-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rustdoc: clean up a bunch of ts-expected-error declarations in main #136263
Conversation
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
1a99bc5
to
cf3ce0a
Compare
This comment has been minimized.
This comment has been minimized.
cf3ce0a
to
db13f16
Compare
This comment has been minimized.
This comment has been minimized.
db13f16
to
85f7423
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Two questions, r=me when resolved in one way or another
const sidebar = document.querySelector(".sidebar"); | ||
if (!resizer || !sidebar) { | ||
// If this page has no sidebar at all, bail out. | ||
if (!(resizer instanceof HTMLElement) || !(sidebar instanceof HTMLElement)) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not /** @type {Element | null} */
for both resizer
and sidebar
and adding the check !resizer || !sidebar
to the guarded return? This would eliminate the two new //@ts-expect-error
s, wouldn't it?
(And it would eliminate the seeming tautology @type HTMLElement
and !(resizer instanceof HTMLElement)
. I'm slightly surprised that querySelector
doesn't unconditionally return Element
if the param is string
but I guess that's convenience over safety (ofc in practice none of this matters lol)?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this code uses offsetTop, which is defined on HTMLElement, not Element.
And querySelector returns Element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And querySelector returns Element.
As far as I can tell, (the relevant overload of) querySelector
returns any caller-provided E
that extends Element
which allows you to instantiate it with HTMLElement
-- if it was Element
you wouldn't've been able to cast it to HTMLElement
which is more specific than Element
.
Unfortunately, this code uses offsetTop, which is defined on HTMLElement, not Element.
Yes, resizer
and sidebar
would start out as Element
s but due to the guarded returns of the form if (!(… instanceof HTMLElement)) { return; }
the type is automatically narrowed to HTMLElement
following the if statement as part of flow-sensitive typing / control-flow based type analysis which should permit you to access offsetTop
on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// lib.dom.d.ts
querySelector<E extends Element = Element>(selectors: string): E | null;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I was misunderstanding it. I admit, I'm not actually that familiar with TypeScript, beyond intro-level docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries
// @ts-expect-error | ||
getSettingsButton().onclick = event => { | ||
const settingsButton = getSettingsButton(); | ||
if (settingsButton) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the unlikely event that the settings button cannot be located, I think it would be better to fail-fast, rather than silently ignore it, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we went ahead and let it crash, it would still just drop an error in the browser console, probably never to be noticed by anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, I suppose.
@@ -1813,7 +1811,7 @@ href="https://doc.rust-lang.org/${channel}/rustdoc/read-documentation/search.htm | |||
// from settings.js, which uses a separate function. It's done here because | |||
// the minimum sidebar size is rather uncomfortable, and it must pass | |||
// through that size when using the shrink-to-nothing gesture. | |||
function hideSidebar() { | |||
const hideSidebar = function hideSidebar() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const hideSidebar = function hideSidebar() { | |
const hideSidebar = function() { |
Similarly for all other occurrences of this kind of change.
I guess this is just drive-by cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is just drive-by cleanup?
No, it prevents the function definitions from being hoisted above the null check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with new nitpick addressed and with the three last commits squashed
This mostly consists of handling potentially-null input and adding more global functions to the list of globals.
85aca23
to
2ea95f8
Compare
@bors r=fmease rollup |
…=fmease rustdoc: clean up a bunch of ts-expected-error declarations in main This mostly consists of handling potentially-null input and adding more global functions to the list of globals. Follow-up for rust-lang#136161
Rollup of 9 pull requests Successful merges: - rust-lang#128045 (#[contracts::requires(...)] + #[contracts::ensures(...)]) - rust-lang#136263 (rustdoc: clean up a bunch of ts-expected-error declarations in main) - rust-lang#136375 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 1)) - rust-lang#136392 (bootstrap: add wrapper macros for `feature = "tracing"`-gated `tracing` macros) - rust-lang#136405 (rustdoc-book: Clean up section on `--output-format`) - rust-lang#136497 (Report generic mismatches when calling bodyless trait functions) - rust-lang#136502 (Mark `std::fmt::from_fn` as `#[must_use]`) - rust-lang#136509 (Add tests for nested macro_rules edition behavior) - rust-lang#136526 (mir_build: Rename `thir::cx::Cx` to `ThirBuildCx` and remove `UserAnnotatedTyHelpers`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 8 pull requests Successful merges: - rust-lang#128045 (#[contracts::requires(...)] + #[contracts::ensures(...)]) - rust-lang#136263 (rustdoc: clean up a bunch of ts-expected-error declarations in main) - rust-lang#136375 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 1)) - rust-lang#136392 (bootstrap: add wrapper macros for `feature = "tracing"`-gated `tracing` macros) - rust-lang#136396 (rustdoc-json-types: Document that crate name isn't package name.) - rust-lang#136405 (rustdoc-book: Clean up section on `--output-format`) - rust-lang#136502 (Mark `std::fmt::from_fn` as `#[must_use]`) - rust-lang#136509 (Add tests for nested macro_rules edition behavior) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#136263 - notriddle:notriddle/typescript2, r=fmease rustdoc: clean up a bunch of ts-expected-error declarations in main This mostly consists of handling potentially-null input and adding more global functions to the list of globals. Follow-up for rust-lang#136161
This mostly consists of handling potentially-null input and adding more global functions to the list of globals.
Follow-up for #136161