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: use typed-query-selector for native querySelector #11990

Merged
merged 2 commits into from
Jan 23, 2021
Merged

misc: use typed-query-selector for native querySelector #11990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
927098a
misc: use typed-query-selector for native querySelector
brendankenny Jan 22, 2021
4cac615
rm readerTarget
brendankenny Jan 22, 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: 6 additions & 8 deletions clients/extension/scripts/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,11 @@ const STRINGS = BROWSER_BRAND === 'chrome' ? CHROME_STRINGS : FIREFOX_STRINGS;
/**
* Guaranteed context.querySelector. Always returns an element or throws if
* nothing matches query.
* @param {string} query
* @template {string} T
* @param {T} query
* @param {ParentNode=} context
* @return {HTMLElement}
*/
function find(query, context = document) {
/** @type {?HTMLElement} */
const result = context.querySelector(query);
if (result === null) {
throw new Error(`query ${query} not found`);
Expand Down Expand Up @@ -108,10 +107,9 @@ function fillDevToolsShortcut() {
function readSettingsFromDomAndPersist() {
const optionsEl = find('.section--options');
// Save settings when options page is closed.
const checkboxes = /** @type {NodeListOf<HTMLInputElement>} */
(optionsEl.querySelectorAll('.options__categories input:checked'));
const checkboxes = optionsEl.querySelectorAll('.options__categories input:checked');
const selectedCategories = Array.from(checkboxes).map(input => input.value);
const device = /** @type {HTMLInputElement} */ (find('input[name="device"]:checked')).value;
const device = find('input[name="device"]:checked').value;

const settings = {
selectedCategories,
Expand Down Expand Up @@ -155,8 +153,8 @@ async function initPopup() {

const mainEl = find('main');
const optionsEl = find('.button--configure');
const generateReportButton = /** @type {HTMLButtonElement} */ (find('.button--generate'));
const configureButton = /** @type {HTMLButtonElement} */ (find('.button--configure'));
const generateReportButton = find('button.button--generate');
const configureButton = find('button.button--configure');
const psiDisclaimerEl = find('.psi-disclaimer');
const errorMessageEl = find('.errormsg');
const optionsFormEl = find('.options__form');
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/gather/fetcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ class Fetcher {
*/
/* c8 ignore start */
function injectIframe(src) {
/** @type {HTMLIFrameElement} */
const iframe = document.createElement('iframe');
// Try really hard not to affect the page.
iframe.style.display = 'none';
Expand Down
10 changes: 2 additions & 8 deletions lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,10 @@ async function collectTagsThatBlockFirstPaint() {
try {
/** @type {Array<LinkTag>} */
const linkTags = [...document.querySelectorAll('link')]
.filter(/** @return {tag is HTMLLinkElement} */ tag => {
if (tag.tagName !== 'LINK') return false;

.filter(linkTag => {
Copy link
Member Author

Choose a reason for hiding this comment

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

removing the lines in this file is real nice

// Filter stylesheet/HTML imports that block rendering.
// https://www.igvita.com/2012/06/14/debunking-responsive-css-performance-myths/
// https://www.w3.org/TR/html-imports/#dfn-import-async-attribute
const linkTag = /** @type {HTMLLinkElement} */ (tag);
const blockingStylesheet = linkTag.rel === 'stylesheet' &&
window.matchMedia(linkTag.media).matches && !linkTag.disabled;
const blockingImport = linkTag.rel === 'import' && !linkTag.hasAttribute('async');
Expand All @@ -87,10 +84,7 @@ async function collectTagsThatBlockFirstPaint() {

/** @type {Array<ScriptTag>} */
const scriptTags = [...document.querySelectorAll('head script[src]')]
.filter(/** @return {tag is HTMLScriptElement} */ tag => {
if (tag.tagName !== 'SCRIPT') return false;

const scriptTag = /** @type {HTMLScriptElement} */ (tag);
.filter(scriptTag => {
return (
!scriptTag.hasAttribute('async') &&
!scriptTag.hasAttribute('defer') &&
Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/report/html/renderer/category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class CategoryRenderer {
/**
* @param {LH.ReportResult.Category} category
* @param {Record<string, LH.Result.ReportGroup>} groupDefinitions
* @return {Element}
* @return {DocumentFragment}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this just remain Element? I don't see benefit in making the return type more specific.

Copy link
Member Author

@brendankenny brendankenny Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can this just remain Element? I don't see benefit in making the return type more specific.

It would be a Node, not an Element, so it had to change either way

*/
renderCategoryHeader(category, groupDefinitions) {
const tmpl = this.dom.cloneTemplate('#tmpl-lh-category-header', this.templateContext);
Expand All @@ -184,7 +184,7 @@ class CategoryRenderer {
this.dom.find('.lh-category-header__description', tmpl).appendChild(descEl);
}

return /** @type {Element} */ (tmpl.firstElementChild);
return tmpl;
Copy link
Member Author

@brendankenny brendankenny Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

not a querySelector simplification, but you can appendChild a DocumentFragment. Not sure why we bothered with passing back the first child here since this is equivalent.

}

/**
Expand Down Expand Up @@ -336,8 +336,7 @@ class CategoryRenderer {
// Cast `null` to 0
const numericScore = Number(category.score);
const gauge = this.dom.find('.lh-gauge', tmpl);
/** @type {?SVGCircleElement} */
const gaugeArc = gauge.querySelector('.lh-gauge-arc');
const gaugeArc = this.dom.find('circle.lh-gauge-arc', gauge);

if (gaugeArc) this._setGaugeArc(gaugeArc, numericScore);

Expand Down
2 changes: 0 additions & 2 deletions lighthouse-core/report/html/renderer/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,6 @@ class DOM {
* @param {ParentNode} context
*/
find(query, context) {
/** @type {?import('typed-query-selector/parser').ParseSelector<T>} */
const result = context.querySelector(query);
if (result === null) {
throw new Error(`query ${query} not found`);
Expand All @@ -233,7 +232,6 @@ class DOM {
* @param {ParentNode} context
*/
findAll(query, context) {
/** @type {Array<import('typed-query-selector/parser').ParseSelector<T>>} */
const elements = Array.from(context.querySelectorAll(query));
return elements;
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/report/html/renderer/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ class Logger {
* @param {Element} element
*/
constructor(element) {
/** @type {Element} */
this.el = element;
this._id = undefined;
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/report/html/renderer/report-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class ReportRenderer {
const el = this._dom.cloneTemplate('#tmpl-lh-heading', this._templateContext);
const domFragment = this._dom.cloneTemplate('#tmpl-lh-scores-wrapper', this._templateContext);
const placeholder = this._dom.find('.lh-scores-wrapper-placeholder', el);
/** @type {HTMLDivElement} */ (placeholder.parentNode).replaceChild(domFragment, placeholder);
placeholder.replaceWith(domFragment);
return el;
}

Expand Down
20 changes: 9 additions & 11 deletions lighthouse-core/report/html/renderer/report-ui-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,7 @@ class ReportUIFeatures {
];

// Get all tables with a text url column.
/** @type {Array<HTMLTableElement>} */
const tables = Array.from(this._document.querySelectorAll('.lh-table'));
const tables = Array.from(this._document.querySelectorAll('table.lh-table'));
const tablesWithUrls = tables
.filter(el =>
el.querySelector('td.lh-table-column--url, td.lh-table-column--source-location'))
Expand Down Expand Up @@ -335,8 +334,7 @@ class ReportUIFeatures {
for (const rowEl of rowEls) {
if (rowEl.classList.contains('lh-sub-item-row')) continue;

/** @type {HTMLElement|null} */
const urlItem = rowEl.querySelector('.lh-text__url');
const urlItem = rowEl.querySelector('div.lh-text__url');
if (!urlItem) continue;

const datasetUrl = urlItem.dataset.url;
Expand Down Expand Up @@ -834,11 +832,11 @@ class DropDown {

/**
* @param {Array<Node>} allNodes
* @param {?Node=} startNode
* @returns {Node}
* @param {?HTMLElement=} startNode
* @returns {HTMLElement}
*/
_getNextSelectableNode(allNodes, startNode) {
const nodes = allNodes.filter((node) => {
const nodes = allNodes.filter(/** @return {node is HTMLElement} */ (node) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

also not querySelector related, but this was already checking instanceof HTMLElement, so it seems good

if (!(node instanceof HTMLElement)) {
return false;
}
Expand All @@ -865,21 +863,21 @@ class DropDown {
}

/**
* @param {?Element=} startEl
* @param {?HTMLElement=} startEl
* @returns {HTMLElement}
*/
_getNextMenuItem(startEl) {
const nodes = Array.from(this._menuEl.childNodes);
return /** @type {HTMLElement} */ (this._getNextSelectableNode(nodes, startEl));
return this._getNextSelectableNode(nodes, startEl);
}

/**
* @param {?Element=} startEl
* @param {?HTMLElement=} startEl
* @returns {HTMLElement}
*/
_getPreviousMenuItem(startEl) {
const nodes = Array.from(this._menuEl.childNodes).reverse();
return /** @type {HTMLElement} */ (this._getNextSelectableNode(nodes, startEl));
return this._getNextSelectableNode(nodes, startEl);
}
}

Expand Down
5 changes: 2 additions & 3 deletions lighthouse-treemap/app/src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,11 @@ class TreemapUtil {
/**
* Guaranteed context.querySelector. Always returns an element or throws if
* nothing matches query.
* @param {string} query
* @template {string} T
* @param {T} query
* @param {ParentNode=} context
* @return {HTMLElement}
*/
static find(query, context = document) {
/** @type {?HTMLElement} */
const result = context.querySelector(query);
if (result === null) {
throw new Error(`query ${query} not found`);
Expand Down
10 changes: 4 additions & 6 deletions lighthouse-viewer/app/src/lighthouse-report-viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
/**
* Guaranteed context.querySelector. Always returns an element or throws if
* nothing matches query.
* @param {string} query
* @template {string} T
* @param {T} query
* @param {ParentNode} context
* @return {HTMLElement}
*/
function find(query, context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

we apparently have four copies of find() :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

soon to be 5 with #11832

I often copy Dom in personal projects because the interface is so nice.

/** @type {?HTMLElement} */
const result = context.querySelector(query);
if (result === null) {
throw new Error(`query ${query} not found`);
Expand Down Expand Up @@ -67,7 +66,7 @@ class LighthouseReportViewer {
gistUrlInput.addEventListener('change', this._onUrlInputChange);

// Hidden file input to trigger manual file selector.
const fileInput = find('#hidden-file-input', document);
const fileInput = find('input#hidden-file-input', document);
fileInput.addEventListener('change', e => {
if (!e.target) {
return;
Expand Down Expand Up @@ -283,8 +282,7 @@ class LighthouseReportViewer {
return new Promise((resolve, reject) => {
const reader = new FileReader();
reader.onload = function(e) {
const readerTarget = /** @type {?FileReader} */ (e.target);
const result = /** @type {?string} */ (readerTarget && readerTarget.result);
const result = /** @type {?string} */ (e.target && e.target.result);
if (!result) {
reject('Could not read file');
return;
Expand Down
3 changes: 3 additions & 0 deletions types/externs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
import _Crdp from 'devtools-protocol/types/protocol';
import _CrdpMappings from 'devtools-protocol/types/protocol-mapping'

// Import for side effects improving types of querySelector/querySelectorAll.
import 'typed-query-selector';

declare global {
// Augment Intl to include
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/getCanonicalLocales
Expand Down