Skip to content
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

misc: tighten querySelector types #12013

Merged
merged 2 commits into from
Jan 27, 2021
Merged

misc: tighten querySelector types #12013

merged 2 commits into from
Jan 27, 2021

Conversation

brendankenny
Copy link
Member

Fixes an issue with the new typed querySelector (just this and #12011, I promise :).

The problem is that the typed-query-selector shim that adds stricter types to querySelector/querySelectorAll has a type parameter that's only used in the return type. This is usually recommended against in TypeScript because it means that the contextual type can more often than not become the return type as long as it fits the parameter bounds, rather than the compiler complaining that the return type and the contextual type don't match.

I put more details in g-plane/typed-query-selector#12, but as an example, on master you could currently do

/** @type {HTMLAnchorElement|null} */
const el = document.querySelector('div');

or

/** @type {NodeListOf<HTMLAnchorElement>} */
const els = document.querySelectorAll('div');

without error.

As mentioned in the upstream bug, it's not clear that they'll want to fix the problem as it also adds a feature (explicit type overrides), but using the ParseSelector directly is an intended and documented part of the library, so it seems ok to use our own querySelector/querySelectorAll shims.

This PR also updates pageFunctions.getElementsInDocument to use the better typed returns, because that's what I was doing when I noticed this and #12011 :)

@brendankenny brendankenny requested a review from a team as a code owner January 27, 2021 22:59
@brendankenny brendankenny requested review from patrickhulce and removed request for a team January 27, 2021 22:59
@google-cla google-cla bot added the cla: yes label Jan 27, 2021
Comment on lines 115 to 118
if (!selector || realMatchesFn.call(el, selector)) {
// @ts-expect-error - el is verified as matching above, tsc just can't verify it through the .call().
results.push(el);
}
Copy link
Member Author

@brendankenny brendankenny Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be real fancy to also type Element.prototype.matches like

interface Element {
  matches<S extends string>(selectors: S): this is ParseSelector<S>;
}

and then realMatchesFn.call(el, selector)) would verify that el could be pushed into the results array, but unfortunately augmenting matches like that makes it so the method is overloaded, and the old non-specific form (matches(selectors: string): boolean) is the form that wins when realMatchesFn.call picks one of the overloads to use as a type (while calling it directly would pick the above, more specific version...overloads are annoying).

As a result, there's no type guard and so el remains an Element, which we already knew :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool beans! LGTM

types/externs.d.ts Outdated Show resolved Hide resolved
const _findAllElements = nodes => {
for (let i = 0, el; el = nodes[i]; ++i) {
if (!selector || realMatchesFn.call(el, selector)) {
// @ts-expect-error - el is verified as matching above, tsc just can't verify it through the .call().
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In personal projects I've started only ever adding @ts-expect-error to lines that are const thing: Bar = foo based on the number of times we've been screwed by ignoring the wrong errors :) I'm curious what your stance is on that.

I don't think this case is particularly dangerous, it just made me think of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you're definitely right it's better, and maybe we should establish that as the pattern...the only error being suppressed would be the type change of el, while when it's on this line, it could also be results has the wrong type or whatever.

My only hesitation is how annoying jsdoc is for this, turning two lines into three :) If only there was a // @ts-expect-error-line or something...

if (!selector || realMatchesFn.call(el, selector)) {
  /** @type {ParseSelector<T>} */
  // @ts-expect-error - el is verified as matching above, tsc just can't verify it through the .call().
  const matchedEl = el;
  results.push(matchedEl);
}

worth it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth it?

(yes)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, sure. I was mostly asking for future reviews if you're onboard with that policy too, but here is good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants