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: replace most usages of evaluateAsync with structured evaluate #11754

Merged
merged 38 commits into from
Dec 22, 2020
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
90018b2
core(driver): create page code using structured interface
connorjclark May 20, 2020
f7e38f5
rename type
connorjclark May 20, 2020
78f386a
pr feedback
connorjclark May 20, 2020
32856e7
no strings
connorjclark May 20, 2020
e2dd63d
test
connorjclark May 20, 2020
f38bf41
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 4, 2020
ce9839b
test
connorjclark Jun 4, 2020
4c903de
restrucutre
connorjclark Jun 5, 2020
0413a4a
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 12, 2020
d4bd2bf
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jun 17, 2020
02dc215
remove obj
connorjclark Jun 17, 2020
ce2827a
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Jul 15, 2020
c81a636
update master
connorjclark Nov 5, 2020
70d8210
rm dead code
connorjclark Nov 5, 2020
df5e8dd
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Nov 10, 2020
4e59476
adam feedback
connorjclark Nov 10, 2020
147b98c
fix tests
connorjclark Nov 10, 2020
a4051a1
fix mangle issues
connorjclark Nov 10, 2020
9eea206
wip
connorjclark Nov 10, 2020
d79b23f
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
b8c76ed
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
2088109
fix
connorjclark Dec 2, 2020
7b01e8c
wip
connorjclark Dec 2, 2020
4cc1eea
fix
connorjclark Dec 2, 2020
aae0a9f
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 2, 2020
77af88d
fix tests
connorjclark Dec 3, 2020
d35fd5e
fix
connorjclark Dec 3, 2020
9c10cda
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced-2
connorjclark Dec 21, 2020
d643baf
update
connorjclark Dec 21, 2020
9638b90
master
connorjclark Dec 21, 2020
c526772
fixtest
connorjclark Dec 21, 2020
b893bb1
fix
connorjclark Dec 22, 2020
606db62
oops
connorjclark Dec 22, 2020
8c98d05
pr
connorjclark Dec 22, 2020
f1c0afe
refactor
connorjclark Dec 22, 2020
05ba735
ya
connorjclark Dec 22, 2020
9545ab4
fix
connorjclark Dec 22, 2020
668d04e
w.e. async you can stay, silly jest
connorjclark Dec 22, 2020
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
2 changes: 1 addition & 1 deletion docs/recipes/custom-gatherer-puppeteer/custom-gatherer.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class CustomGatherer extends Gatherer {
el.type = 'number';
document.body.append(el);
}
await driver.evaluateAsync(`(${makeInput})()`);
await driver.evaluate(makeInput, {args: []});
await new Promise(resolve => setTimeout(resolve, 100));

// Prove that `driver` (Lighthouse) and `page` (Puppeteer) are talking to the same page.
Expand Down
13 changes: 13 additions & 0 deletions lighthouse-core/fraggle-rock/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,19 @@ class Driver {
if (!this._executionContext) throw new Error('Driver not connected to page');
return this._executionContext.evaluateAsync(expression, options);
}

/**
* @template {any[]} T, R
* @param {((...args: T) => R)} mainFn The main function to call.
* @param {{args: T, useIsolation?: boolean, deps?: Array<Function|string>}} options `args` should
* match the args of `mainFn`, and can be any serializable value. `deps` are functions that must be
* defined for `mainFn` to work.
* @return {FlattenedPromise<R>}
*/
evaluate(mainFn, options) {
if (!this._executionContext) throw new Error('Driver not connected to page');
return this._executionContext.evaluate(mainFn, options);
}
}

module.exports = Driver;
7 changes: 6 additions & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,11 @@ class Driver {
}

/**
* Note: Prefer `evaluate` instead.
* Evaluate an expression in the context of the current page. If useIsolation is true, the expression
* will be evaluated in a content script that has access to the page's DOM but whose JavaScript state
* is completely separate.
* Returns a promise that resolves on the expression's value.
* @param {string} expression
* @param {{useIsolation?: boolean}=} options
* @return {Promise<*>}
Expand All @@ -484,7 +489,7 @@ class Driver {
* @param {{args: T, useIsolation?: boolean, deps?: Array<Function|string>}} options `args` should
* match the args of `mainFn`, and can be any serializable value. `deps` are functions that must be
* defined for `mainFn` to work.
* @return {Promise<R>}
* @return {FlattenedPromise<R>}
Copy link
Collaborator Author

@connorjclark connorjclark Dec 2, 2020

Choose a reason for hiding this comment

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

Otherwise results in Promise<Promise<...>>, given some main functions (first arg to evaluate) return promises. see microsoft/TypeScript#27711 (comment) for more

*/
async evaluate(mainFn, options) {
return this._executionContext.evaluate(mainFn, options);
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 @@ -160,7 +160,7 @@ class ExecutionContext {
* @param {{args: T, useIsolation?: boolean, deps?: Array<Function|string>}} options `args` should
* match the args of `mainFn`, and can be any serializable value. `deps` are functions that must be
* defined for `mainFn` to work.
* @return {Promise<R>}
* @return {FlattenedPromise<R>}
*/
evaluate(mainFn, options) {
const argsSerialized = options.args.map(arg => JSON.stringify(arg)).join(',');
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/gather/driver/wait-for-condition.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,15 +213,15 @@ function waitForCPUIdle(session, waitForCPUQuiet) {
let lastTimeout;
let canceled = false;

const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`;
/**
* @param {ExecutionContext} executionContext
* @param {() => void} resolve
* @return {Promise<void>}
*/
async function checkForQuiet(executionContext, resolve) {
if (canceled) return;
const timeSinceLongTask = await executionContext.evaluateAsync(checkForQuietExpression);
const timeSinceLongTask =
await executionContext.evaluate(pageFunctions.checkTimeSinceLastLongTask, {args: []});
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (canceled) return;

if (typeof timeSinceLongTask === 'number') {
Expand Down
14 changes: 8 additions & 6 deletions lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,15 @@ class Accessibility extends Gatherer {
*/
afterPass(passContext) {
const driver = passContext.driver;
const expression = `(function () {
${pageFunctions.getNodeDetailsString};
${axeLibSource};
return (${runA11yChecks.toString()}());
})()`;

return driver.evaluateAsync(expression, {useIsolation: true}).then(returnedValue => {
return driver.evaluate(runA11yChecks, {
args: [],
useIsolation: true,
deps: [
axeLibSource,
pageFunctions.getNodeDetailsString,
],
}).then(returnedValue => {
if (!returnedValue) {
throw new Error('No axe-core results returned');
}
Expand Down
16 changes: 8 additions & 8 deletions lighthouse-core/gather/gatherers/anchor-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,15 @@ class AnchorElements extends Gatherer {
*/
async afterPass(passContext) {
const driver = passContext.driver;
const expression = `(() => {
${pageFunctions.getElementsInDocumentString};
${pageFunctions.getNodeDetailsString};

return (${collectAnchorElements})();
})()`;

/** @type {LH.Artifacts['AnchorElements']} */
const anchors = await driver.evaluateAsync(expression, {useIsolation: true});
const anchors = await driver.evaluate(collectAnchorElements, {
args: [],
useIsolation: true,
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getNodeDetailsString,
],
});
await driver.sendCommand('DOM.enable');

// DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds if the `DOM` domain was enabled before this gatherer, invoke it to be safe.
Expand Down
7 changes: 1 addition & 6 deletions lighthouse-core/gather/gatherers/cache-contents.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,7 @@ class CacheContents extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;

/** @type {Array<string>|void} */
const cacheUrls = await driver.evaluateAsync(`(${getCacheContents.toString()}())`);
if (!cacheUrls || !Array.isArray(cacheUrls)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extra defensive coding, or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the Array.isArray check is necessary, but cacheUrls could still be empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In JS evaluating arrays for truthiness doesn't consider array length. A length check does happen in python, I believe.

throw new Error('Unable to retrieve cache contents');
}

const cacheUrls = await driver.evaluate(getCacheContents, {args: []});
return cacheUrls;
}
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/dobetterweb/doctype.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Doctype extends Gatherer {
*/
afterPass(passContext) {
const driver = passContext.driver;
return driver.evaluateAsync(`(${getDoctype.toString()}())`);
return driver.evaluate(getDoctype, {args: []});
}
}

Expand Down
21 changes: 11 additions & 10 deletions lighthouse-core/gather/gatherers/dobetterweb/domstats.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,35 @@
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
// @ts-nocheck

/**
* @fileoverview Gathers stats about the max height and width of the DOM tree
* and total number of elements used on the page.
*/

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

'use strict';

const Gatherer = require('../gatherer.js');
const pageFunctions = require('../../../lib/page-functions.js');


/**
* Calculates the maximum tree depth of the DOM.
* @param {HTMLElement} element Root of the tree to look in.
* @param {boolean=} deep True to include shadow roots. Defaults to true.
* @return {LH.Artifacts.DOMStats}
*/
/* istanbul ignore next */
function getDOMStats(element, deep = true) {
function getDOMStats(element = document.body, deep = true) {
let deepestElement = null;
let maxDepth = -1;
let maxWidth = -1;
let numElements = 0;
let parentWithMostChildren = null;

/**
* @param {Element} element
* @param {Element|ShadowRoot} element
* @param {number} depth
*/
const _calcDOMWidthAndHeight = function(element, depth = 1) {
Expand Down Expand Up @@ -64,10 +63,12 @@ function getDOMStats(element, 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 @@ -82,12 +83,12 @@ class DOMStats extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;

const expression = `(function() {
${pageFunctions.getNodeDetailsString};
return (${getDOMStats.toString()}(document.body));
})()`;
await driver.sendCommand('DOM.enable');
const results = await driver.evaluateAsync(expression, {useIsolation: true});
const results = await driver.evaluate(getDOMStats, {
args: [],
useIsolation: true,
deps: [pageFunctions.getNodeDetailsString],
});
await driver.sendCommand('DOM.disable');
return results;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,10 @@ class PasswordInputsWithPreventedPaste extends Gatherer {
* @return {Promise<LH.Artifacts['PasswordInputsWithPreventedPaste']>}
*/
afterPass(passContext) {
const expression = `(() => {
${pageFunctions.getNodeDetailsString};
return (${findPasswordInputsWithPreventedPaste.toString()}());
})()`;

return passContext.driver.evaluateAsync(expression);
return passContext.driver.evaluate(findPasswordInputsWithPreventedPaste, {
args: [],
deps: [pageFunctions.getNodeDetailsString],
});
}
}

Expand Down
16 changes: 8 additions & 8 deletions lighthouse-core/gather/gatherers/form-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,14 @@ class FormElements extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;

const expression = `(() => {
${pageFunctions.getElementsInDocumentString};
${pageFunctions.getNodeDetailsString};
return (${collectFormElements})();
})()`;

/** @type {LH.Artifacts['FormElements']} */
const formElements = await driver.evaluateAsync(expression, {useIsolation: true});
const formElements = await driver.evaluate(collectFormElements, {
args: [],
useIsolation: true,
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getNodeDetailsString,
],
});
return formElements;
}
}
Expand Down
40 changes: 25 additions & 15 deletions lighthouse-core/gather/gatherers/full-page-screenshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

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

const Gatherer = require('./gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');
Expand Down Expand Up @@ -93,18 +93,23 @@ class FullPageScreenshot extends Gatherer {

return nodes;
}
const expression = `(function () {
${pageFunctions.getBoundingClientRectString};
return (${resolveNodes.toString()}());
})()`;

/**
* @param {boolean} useIsolation
*/
function evaluateInPage(useIsolation) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding another evaluate function seems unnecessary. I think it's better to just use passContext.driver.evaluate twice.

Copy link
Member

Choose a reason for hiding this comment

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

Adding another evaluate function seems unnecessary. I think it's better to just use passContext.driver.evaluate twice.

Bump, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I normally wouldn't DRY up so aggressively (just two usages here...), but given the unfortunately "boilerplate"ness here, and the fact that it is all local (as in, this is a nest function only used immediately after being defined) it seems ok to me.

my goal was more readability, which I realized could be further improved by not using a boolean param / using the same function name, so I refactored a bit.

return passContext.driver.evaluate(resolveNodes, {
args: [],
useIsolation,
deps: [pageFunctions.getBoundingClientRectString],
});
}

// Collect nodes with the page context (`useIsolation: false`) and with our own, reused
// context (`useIsolation: true`). Gatherers use both modes when collecting node details,
// so we must do the same here too.
const pageContextResult =
await passContext.driver.evaluateAsync(expression, {useIsolation: false});
const isolatedContextResult =
await passContext.driver.evaluateAsync(expression, {useIsolation: true});
const pageContextResult = await evaluateInPage(false);
const isolatedContextResult = await evaluateInPage(true);
return {...pageContextResult, ...isolatedContextResult};
}

Expand Down Expand Up @@ -136,20 +141,25 @@ class FullPageScreenshot extends Gatherer {
// in the LH runner api, which for ex. puppeteer consumers would setup puppeteer emulation,
// and then just call that to reset?
// https://github.com/GoogleChrome/lighthouse/issues/11122
const observedDeviceMetrics = await driver.evaluateAsync(`(function() {

const observedDeviceMetrics = await driver.evaluate(function getObservedDeviceMetrics() {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// Convert the Web API's snake case (landscape-primary) to camel case (landscapePrimary).
const screenOrientationType = /** @type {LH.Crdp.Emulation.ScreenOrientationType} */ (
snakeCaseToCamelCase(window.screen.orientation.type));
return {
width: document.documentElement.clientWidth,
height: document.documentElement.clientHeight,
screenOrientation: {
type: window.screen.orientation.type,
type: screenOrientationType,
angle: window.screen.orientation.angle,
},
deviceScaleFactor: window.devicePixelRatio,
};
})()`, {useIsolation: true});
// Convert the Web API's snake case (landscape-primary) to camel case (landscapePrimary).
observedDeviceMetrics.screenOrientation.type =
snakeCaseToCamelCase(observedDeviceMetrics.screenOrientation.type);
}, {
args: [],
useIsolation: true,
deps: [snakeCaseToCamelCase],
});
await driver.sendCommand('Emulation.setDeviceMetricsOverride', {
mobile: passContext.settings.formFactor === 'mobile',
...observedDeviceMetrics,
Expand Down
17 changes: 8 additions & 9 deletions lighthouse-core/gather/gatherers/iframe-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,14 @@ class IFrameElements extends Gatherer {
async afterPass(passContext) {
const driver = passContext.driver;

const expression = `(() => {
${pageFunctions.getElementsInDocumentString};
${pageFunctions.isPositionFixedString};
${pageFunctions.getNodeDetailsString};
return (${collectIFrameElements})();
})()`;

/** @type {LH.Artifacts['IFrameElements']} */
const iframeElements = await driver.evaluateAsync(expression, {useIsolation: true});
const iframeElements = await driver.evaluate(collectIFrameElements, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yikes! almost missed this. forgot to include useIsolation: true.

args: [],
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.isPositionFixedString,
pageFunctions.getNodeDetailsString,
],
});
return iframeElements;
}
}
Expand Down
Loading