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

core: better type checking of evaluate() code #12795

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions build/build-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const terser = require('terser');
const {minifyFileTransform} = require('./build-utils.js');
const {LH_ROOT} = require('../root.js');

const pageFunctions = require('../lighthouse-core/lib/page-functions.js');

const COMMIT_HASH = require('child_process')
.execSync('git rev-parse HEAD')
.toString().trim();
Expand Down Expand Up @@ -152,6 +154,8 @@ async function minifyScript(filePath) {
keep_classnames: true,
// Runtime.evaluate errors if function names are elided.
keep_fnames: true,
// Preserve page-function function names for references in Runtime.evaluate.
mangle: {reserved: Object.keys(pageFunctions)},
sourceMap: DEBUG && {
content: JSON.parse(fs.readFileSync(`${filePath}.map`, 'utf-8')),
url: path.basename(`${filePath}.map`),
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/driver/execution-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class ExecutionContext {
return new Promise(function (resolve) {
return Promise.resolve()
.then(_ => ${expression})
.catch(${pageFunctions.wrapRuntimeEvalErrorInBrowserString})
.catch(${pageFunctions.wrapRuntimeEvalErrorInBrowser.toString()})
Copy link
Collaborator

Choose a reason for hiding this comment

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

why call toString explicitly when all objects in a template string are coerced via .toString() automatically?

.then(resolve);
});
}())`,
Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
*/
'use strict';

/* global window, document, getNodeDetails */
/* global window, document */

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const axeLibSource = require('../../lib/axe.js').source;
const pageFunctions = require('../../lib/page-functions.js');
const {getNodeDetails} = require('../../lib/page-functions.js');

/**
* This is run in the page, not Lighthouse itself.
Expand Down Expand Up @@ -84,7 +84,6 @@ function createAxeRuleResultArtifact(result) {
const nodes = result.nodes.map(node => {
const {target, failureSummary, element} = node;
// TODO: with `elementRef: true`, `element` _should_ always be defined, but need to verify.
// @ts-expect-error - getNodeDetails put into scope via stringification
const nodeDetails = getNodeDetails(/** @type {HTMLElement} */ (element));

return {
Expand Down Expand Up @@ -135,7 +134,7 @@ class Accessibility extends FRGatherer {
useIsolation: true,
deps: [
axeLibSource,
pageFunctions.getNodeDetailsString,
getNodeDetails,
createAxeRuleResultArtifact,
],
});
Expand Down
14 changes: 5 additions & 9 deletions lighthouse-core/gather/gatherers/anchor-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@
*/
'use strict';

/* global getNodeDetails */

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const dom = require('../driver/dom.js');
const pageFunctions = require('../../lib/page-functions.js');
const {getElementsInDocument, getNodeDetails} = require('../../lib/page-functions.js');
Copy link
Collaborator

Choose a reason for hiding this comment

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

(you probably already know this, just wanted to make it explicit)

FWIW this is what breaks terser. this is basically

const getElementsInDocument = require('../../lib/page-functions.js').getElementsInDocument;

which allows terser to do w/e it wants with the name here since it is just local.


/* eslint-env browser, node */

Expand Down Expand Up @@ -39,9 +37,9 @@ function collectAnchorElements() {
return onclick.slice(0, 1024);
}

// Manually widen type to include SVG. See https://github.com/GoogleChrome/lighthouse/issues/12011.
/** @type {Array<HTMLAnchorElement|SVGAElement>} */
// @ts-expect-error - put into scope via stringification
const anchorElements = getElementsInDocument('a'); // eslint-disable-line no-undef
const anchorElements = getElementsInDocument('a');

return anchorElements.map(node => {
if (node instanceof HTMLAnchorElement) {
Expand All @@ -54,7 +52,6 @@ function collectAnchorElements() {
text: node.innerText, // we don't want to return hidden text, so use innerText
rel: node.rel,
target: node.target,
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(node),
};
}
Expand All @@ -67,7 +64,6 @@ function collectAnchorElements() {
text: node.textContent || '',
rel: '',
target: node.target.baseVal || '',
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(node),
};
});
Expand Down Expand Up @@ -107,8 +103,8 @@ class AnchorElements extends FRGatherer {
args: [],
useIsolation: true,
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getNodeDetailsString,
getElementsInDocument,
getNodeDetails,
],
});
await session.sendCommand('DOM.enable');
Expand Down
14 changes: 7 additions & 7 deletions lighthouse-core/gather/gatherers/dobetterweb/domstats.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
* and total number of elements used on the page.
*/

/* global getNodeDetails document */
/* global document */

'use strict';

const FRGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
const pageFunctions = require('../../../lib/page-functions.js');
const {getNodeDetails} = require('../../../lib/page-functions.js');

/**
* Calculates the maximum tree depth of the DOM.
Expand All @@ -24,11 +24,13 @@ const pageFunctions = require('../../../lib/page-functions.js');
*/
/* c8 ignore start */
function getDOMStats(element = document.body, deep = true) {
let deepestElement = null;
/** @type {Element|ShadowRoot} */
let deepestElement = element;
let maxDepth = -1;
let maxWidth = -1;
let numElements = 0;
let parentWithMostChildren = null;
/** @type {Element|ShadowRoot} */
let parentWithMostChildren = element;

/**
* @param {Element|ShadowRoot} element
Expand Down Expand Up @@ -63,12 +65,10 @@ function getDOMStats(element = document.body, deep = true) {
return {
depth: {
max: result.maxDepth,
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(deepestElement),
},
width: {
max: result.maxWidth,
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(parentWithMostChildren),
},
totalBodyElements: result.numElements,
Expand All @@ -93,7 +93,7 @@ class DOMStats extends FRGatherer {
const results = await driver.executionContext.evaluate(getDOMStats, {
args: [],
useIsolation: true,
deps: [pageFunctions.getNodeDetailsString],
deps: [getNodeDetails],
});
await driver.defaultSession.sendCommand('DOM.disable');
return results;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
*/
'use strict';

/* global document ClipboardEvent getNodeDetails */
/* global document ClipboardEvent */

const FRGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
const pageFunctions = require('../../../lib/page-functions.js');
const {getNodeDetails} = require('../../../lib/page-functions.js');

/**
* @return {LH.Artifacts['PasswordInputsWithPreventedPaste']}
Expand All @@ -22,7 +22,6 @@ function findPasswordInputsWithPreventedPaste() {
)
)
.map(passwordInput => ({
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(passwordInput),
}));
}
Expand All @@ -41,7 +40,7 @@ class PasswordInputsWithPreventedPaste extends FRGatherer {
getArtifact(passContext) {
return passContext.driver.executionContext.evaluate(findPasswordInputsWithPreventedPaste, {
args: [],
deps: [pageFunctions.getNodeDetailsString],
deps: [getNodeDetails],
});
}
}
Expand Down
65 changes: 34 additions & 31 deletions lighthouse-core/gather/gatherers/form-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,37 @@
*/
'use strict';

/* global getNodeDetails */

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');
const {getElementsInDocument, getNodeDetails} = require('../../lib/page-functions.js');

/* eslint-env browser, node */

/* c8 ignore start */
/**
* @param {HTMLFormElement} formElement
* @return {LH.Artifacts.Form}
*/
function createFormElementArtifact(formElement) {
return {
attributes: {
id: formElement.id,
name: formElement.name,
autocomplete: formElement.autocomplete,
},
node: getNodeDetails(formElement),
inputs: [],
labels: [],
};
}
/* c8 ignore stop */

/**
* @return {LH.Artifacts['FormElements']}
*/
/* c8 ignore start */
function collectFormElements() {
// @ts-expect-error - put into scope via stringification
const formChildren = getElementsInDocument('textarea, input, label, select'); // eslint-disable-line no-undef
/** @type {Map<HTMLFormElement|string, LH.Artifacts.Form>} */
const formChildren = getElementsInDocument('textarea, input, label, select');
/** @type {Map<HTMLFormElement, LH.Artifacts.Form>} */
const forms = new Map();
/** @type {LH.Artifacts.Form} */
const formlessObj = {
Expand All @@ -32,23 +48,14 @@ function collectFormElements() {
(child.type === 'submit' || child.type === 'button');
if (isButton) continue;

let formObj = formlessObj;

const parentFormElement = child.form;
const hasForm = !!parentFormElement;
if (hasForm && !forms.has(parentFormElement)) {
const newFormObj = {
attributes: {
id: parentFormElement.id,
name: parentFormElement.name,
autocomplete: parentFormElement.autocomplete,
},
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(parentFormElement),
inputs: [],
labels: [],
};
forms.set(parentFormElement, newFormObj);
if (parentFormElement) {
formObj = forms.get(parentFormElement) || createFormElementArtifact(parentFormElement);
forms.set(parentFormElement, formObj);
}
const formObj = forms.get(parentFormElement) || formlessObj;

if (child instanceof HTMLInputElement || child instanceof HTMLTextAreaElement
|| child instanceof HTMLSelectElement) {
formObj.inputs.push({
Expand All @@ -61,27 +68,22 @@ function collectFormElements() {
attribute: child.getAttribute('autocomplete'),
prediction: child.getAttribute('autofill-prediction'),
},
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(child),
});
}
if (child instanceof HTMLLabelElement) {
formObj.labels.push({
for: child.htmlFor,
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(child),
});
}
}

const formElements = [...forms.values()];
if (formlessObj.inputs.length > 0 || formlessObj.labels.length > 0) {
forms.set('formless', {
node: formlessObj.node,
inputs: formlessObj.inputs,
labels: formlessObj.labels,
});
formElements.push(formlessObj);
}
return [...forms.values()];
return formElements;
}
/* c8 ignore stop */

Expand All @@ -102,8 +104,9 @@ class FormElements extends FRGatherer {
args: [],
useIsolation: true,
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getNodeDetailsString,
createFormElementArtifact,
getElementsInDocument,
getNodeDetails,
],
});
return formElements;
Expand Down
9 changes: 4 additions & 5 deletions lighthouse-core/gather/gatherers/full-page-screenshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
*/
'use strict';

/* globals window document getBoundingClientRect */
/* globals window document */

const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const emulation = require('../../lib/emulation.js');
const pageFunctions = require('../../lib/page-functions.js');
const {getBoundingClientRect, getMaxTextureSize} = require('../../lib/page-functions.js');

// JPEG quality setting
// Exploration and examples of reports using different quality settings: https://docs.google.com/document/d/1ZSffucIca9XDW2eEwfoevrk-OTl7WQFeMf0CgeJAA8M/edit#
Expand All @@ -34,7 +34,7 @@ class FullPageScreenshot extends FRGatherer {
* @see https://bugs.chromium.org/p/chromium/issues/detail?id=770769
*/
async getMaxScreenshotHeight(context) {
return await context.driver.executionContext.evaluate(pageFunctions.getMaxTextureSize, {
return await context.driver.executionContext.evaluate(getMaxTextureSize, {
args: [],
useIsolation: true,
deps: [],
Expand Down Expand Up @@ -104,7 +104,6 @@ class FullPageScreenshot extends FRGatherer {

const lhIdToElements = window.__lighthouseNodesDontTouchOrAllVarianceGoesAway;
for (const [node, id] of lhIdToElements.entries()) {
// @ts-expect-error - getBoundingClientRect put into scope via stringification
const rect = getBoundingClientRect(node);
nodes[id] = rect;
}
Expand All @@ -119,7 +118,7 @@ class FullPageScreenshot extends FRGatherer {
return context.driver.executionContext.evaluate(resolveNodes, {
args: [],
useIsolation,
deps: [pageFunctions.getBoundingClientRectString],
deps: [getBoundingClientRect],
});
}

Expand Down
Loading