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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f146e32
misc: tighten querySelector types
brendankenny Jan 27, 2021
87e0f51
feedback
brendankenny Jan 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
* Otherwise, minification will mangle the variable names and break usage.
*/

/** @typedef {HTMLElementTagNameMap & {[id: string]: HTMLElement}} HTMLElementByTagName */
/**
* `typed-query-selector`'s CSS selector parser.
* @template {string} T
* @typedef {import('typed-query-selector/parser').ParseSelector<T>} ParseSelector
*/

/* global window document Node ShadowRoot HTMLElement */

Expand Down Expand Up @@ -98,19 +102,21 @@ function checkTimeSinceLastLongTask() {
* @template {string} T
* @param {T} selector Optional simple CSS selector to filter nodes on.
* Combinators are not supported.
* @return {Array<HTMLElementByTagName[T]>}
* @return {Array<ParseSelector<T>>}
*/
function getElementsInDocument(selector) {
const realMatchesFn = window.__ElementMatches || window.Element.prototype.matches;
/** @type {Array<HTMLElement>} */
/** @type {Array<ParseSelector<T>>} */
const results = [];

/** @param {NodeListOf<HTMLElement>} nodes */
/** @param {NodeListOf<Element>} nodes */
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 :)

results.push(el);
}
Copy link
Member Author

@brendankenny brendankenny Jan 27, 2021 •
edited
Loading

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 :)


// If the element has a shadow root, dig deeper.
if (el.shadowRoot) {
_findAllElements(el.shadowRoot.querySelectorAll('*'));
Expand Down
8 changes: 7 additions & 1 deletion types/externs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import _Crdp from 'devtools-protocol/types/protocol';
import _CrdpMappings from 'devtools-protocol/types/protocol-mapping'

// Import for side effects improving types of querySelector/querySelectorAll.
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
import 'typed-query-selector';
import {ParseSelector} from 'typed-query-selector/parser';

declare global {
// Augment Intl to include
Expand Down Expand Up @@ -392,4 +392,10 @@ declare global {
// Not defined in tsc yet: https://github.com/microsoft/TypeScript/issues/40807
requestIdleCallback(callback: (deadline: {didTimeout: boolean, timeRemaining: () => DOMHighResTimeStamp}) => void, options?: {timeout: number}): number;
}

// Stricter querySelector/querySelectorAll using typed-query-selector.
interface ParentNode {
querySelector<S extends string>(selector: S): ParseSelector<S> | null;
querySelectorAll<S extends string>(selector: S): NodeListOf<ParseSelector<S>>;
}
}