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(driver): create eval code using interface #10816

Merged
merged 30 commits into from
Dec 2, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
30 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
77afd88
fix
connorjclark Nov 10, 2020
fd94564
fix nasty types by using tuples
connorjclark Nov 10, 2020
9e7644e
no snapshot
connorjclark Nov 10, 2020
f42ca84
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Nov 12, 2020
b1fb5d3
fix pr
connorjclark Nov 12, 2020
c4d3a58
oops
connorjclark Nov 12, 2020
ee6f930
require empty args array
connorjclark Nov 13, 2020
6be38eb
inline
connorjclark Nov 13, 2020
97b0525
last bits
connorjclark Nov 13, 2020
81ea5b5
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Nov 13, 2020
4843291
Merge remote-tracking branch 'origin/master' into driver-eval-enhanced
connorjclark Dec 1, 2020
3c9f573
tests
connorjclark Dec 2, 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
75 changes: 72 additions & 3 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,17 +418,86 @@ class Driver {
});
}

/**
* Deprecated: renamed to `evaluate`.
* @param {string} expression
* @param {{useIsolation?: boolean}=} options
* @return {Promise<*>}
*/
async evaluateAsync(expression, options = {}) {
return this.evaluate(expression, options);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could drop this. if we like this, I'd follow up with a PR that changes all existing usages to this.

}

/**
* 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.
* See the documentation for `createEvalCode` in `page-functions.js`.
* @template T, R
* @param {string | ((...args: T[]) => R)} expressionOrMainFn
* @param {{useIsolation?: boolean, args?: T[], deps?: (Function|string)[]}=} options
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI I would like to drop the string part eventually.

Copy link
Member

Choose a reason for hiding this comment

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

FYI I would like to drop the string part eventually.

could we drop it now and say "if you still want to use a string use the legacy evaluateAsync()"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

* @return {Promise<R>}
Copy link
Member

Choose a reason for hiding this comment

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

Should we just split these into two eval functions?. Keep evaluateAsync and make a new evaluateFunction or something:

  • js-style "overloading" tends to grow in implementation complications over time and is harder to understand as a caller (e.g. options could be included with a string expression, but would it do anything in that case? Now we have to document that, etc)
  • There seems to be a bug in jsdoc generics where R defaults to any here if expression is a string (so there's no inferred R). In the typescript equivalent R will default to unknown (as of Typescript 3.5). If that gets fixed for JS, all the evaluateAsync(string) calls will have to be changed too. I can't think of a great way to set the return type the right way since you still can't do generic defaults or conditional types in jsdoc (AFAIK).

Copy link
Member

Choose a reason for hiding this comment

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

("fun" fact: evaluateAsync is called evaluateAsync and not evaluate because way back, "evaluate" sounded synchronous and we didn't have types to give IDE clues or type checking to ensure that calling code would handle a promised return value. That does make variations on evaluateAsync a little awkward, though, since the "Async" part seems really unnecessary now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the great feedback!

js-style "overloading" tends to grow in implementation complications over time and is harder to understand as a caller (e.g. options could be included with a string expression, but would it do anything in that case? Now we have to document that, etc)

I took a second pass to incorporate this feedback, and added some docs. Running to a meeting, will respond to the rest later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you prefer splitting this out to evaluate(fn, ...) and evaluateString(code, ...) (alias for evalauteAsync)?

Copy link
Member

Choose a reason for hiding this comment

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

rather than add another evaluate*() hop with _evaluate, what do you think about keeping it called evaluateAsync, so

evaluate(fn, ...) calls evaluateAsync(string, opts) calls _evaluateInContext(string, contextId)

then in the next breaking change we either incorporate the guts of evaluateAsync into evaluate so it's back to only two functions, or we rename evaluateAsync to evaluateString or whatever if we do want to keep it around for folks to use?

I see the reason for doing it the _evaluate way, but the number of evaluate layers doesn't seem worth it when external to driver we still only have evaluate() and evaluateAsync either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused about the purpose of _evaluate. Is it for a possible patch where evaluateAsync is removed but we still need a function for executing short expressions?

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 think I just forgot to delete _evaluate.

*/
async evaluate(expressionOrMainFn, options = {}) {
if (typeof expressionOrMainFn !== 'string') {
expressionOrMainFn = pageFunctions.createEvalCode(expressionOrMainFn, {mode: 'iffe', ...options});
}
return this._evaluate(expressionOrMainFn, options);
}

/**
* Evaluate a statement in the context of a JavaScript object on the current page.
* Returns a promise that resolves on the expression's value.
* See the documentation for `createEvalCode` in `page-functions.js`.
* @template T, R
* @param {string | ((...args: T[]) => R)} statementOrMainFn
* @param {{objectId: string, args?: T[], deps?: (Function|string)[]}} options
* @return {Promise<R>}
*/
async evaluateFunctionOnObject(statementOrMainFn, options) {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
if (typeof statementOrMainFn !== 'string') {
statementOrMainFn = pageFunctions.createEvalCode(statementOrMainFn, {
mode: 'function',
...options,
});
}

const response = await this.sendCommand('Runtime.callFunctionOn', {
objectId: options.objectId,
functionDeclaration: statementOrMainFn,
returnByValue: true,
awaitPromise: true,
});

if (response.exceptionDetails) {
// An error occurred before we could even create a Promise, should be *very* rare.
// Also occurs when the expression is not valid JavaScript.
const errorMessage = response.exceptionDetails.exception ?
response.exceptionDetails.exception.description :
response.exceptionDetails.text;
return Promise.reject(new Error(`Evaluation exception: ${errorMessage}`));
}

// TODO: check if __failedInBrowser happens here too.
if (response && response.result && response.result.value) {
return response.result.value;
}
return Promise.reject();
}

/**
* 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
* @param {{useIsolation?: boolean}} options
* @return {Promise<*>}
*/
async evaluateAsync(expression, options = {}) {
const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined;
async _evaluate(expression, options) {
const contextId =
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined;

try {
// `await` is not redundant here because we want to `catch` the async errors
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/gather/fetcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ class Fetcher {
document.body.appendChild(iframe);
}

await this.driver.evaluateAsync(`${injectIframe}(${JSON.stringify(url)})`, {
await this.driver.evaluate(injectIframe, {
args: [url],
useIsolation: true,
});

Expand Down
16 changes: 6 additions & 10 deletions lighthouse-core/gather/gatherers/link-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const Gatherer = require('./gatherer.js');
const URL = require('../../lib/url-shim.js').URL;
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const LinkHeader = require('http-link-header');
const {getElementsInDocumentString} = require('../../lib/page-functions.js');
const {getElementsInDocument} = require('../../lib/page-functions.js');

/* globals HTMLLinkElement */

Expand Down Expand Up @@ -48,9 +48,7 @@ function getCrossoriginFromHeader(value) {
*/
/* istanbul ignore next */
function getLinkElementsInDOM() {
/** @type {Array<HTMLOrSVGElement>} */
// @ts-ignore - getElementsInDocument put into scope via stringification
const browserElements = getElementsInDocument('link'); // eslint-disable-line no-undef
const browserElements = getElementsInDocument('link');
/** @type {LH.Artifacts['LinkElements']} */
const linkElements = [];

Expand Down Expand Up @@ -81,12 +79,10 @@ class LinkElements extends Gatherer {
static getLinkElementsInDOM(passContext) {
// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize
// the values like access from JavaScript does.
return passContext.driver.evaluateAsync(`(() => {
${getElementsInDocumentString};
${getLinkElementsInDOM};

return getLinkElementsInDOM();
})()`, {useIsolation: true});
return passContext.driver.evaluate(getLinkElementsInDOM, {
useIsolation: true,
deps: [getElementsInDocument],
});
}

/**
Expand Down
39 changes: 25 additions & 14 deletions lighthouse-core/gather/gatherers/meta-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,27 @@
'use strict';

const Gatherer = require('./gatherer.js');
const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len
const {getElementsInDocument} = require('../../lib/page-functions.js');

/* istanbul ignore next */
function collectMetaElements() {
const metas = /** @type {HTMLMetaElement[]} */ (getElementsInDocument('head meta'));
return metas.map(meta => {
/** @param {string} name */
const getAttribute = name => {
const attr = meta.attributes.getNamedItem(name);
if (!attr) return;
return attr.value;
};
return {
name: meta.name.toLowerCase(),
content: meta.content,
property: getAttribute('property'),
httpEquiv: meta.httpEquiv ? meta.httpEquiv.toLowerCase() : undefined,
charset: getAttribute('charset'),
};
});
}

class MetaElements extends Gatherer {
/**
Expand All @@ -18,19 +38,10 @@ class MetaElements extends Gatherer {

// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize
// the values like access from JavaScript does.
return driver.evaluateAsync(`(() => {
${getElementsInDocumentString};

return getElementsInDocument('head meta').map(meta => {
return {
name: meta.name.toLowerCase(),
content: meta.content,
property: meta.attributes.property ? meta.attributes.property.value : undefined,
httpEquiv: meta.httpEquiv ? meta.httpEquiv.toLowerCase() : undefined,
charset: meta.attributes.charset ? meta.attributes.charset.value : undefined,
};
});
})()`, {useIsolation: true});
return driver.evaluate(collectMetaElements, {
useIsolation: true,
deps: [getElementsInDocument],
});
}
}

Expand Down
24 changes: 10 additions & 14 deletions lighthouse-core/gather/gatherers/script-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,15 @@
const Gatherer = require('./gatherer.js');
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const NetworkRequest = require('../../lib/network-request.js');
const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len
const {getElementsInDocument, getNodePath} = require('../../lib/page-functions.js');
const pageFunctions = require('../../lib/page-functions.js');

/* global getNodePath */

/**
* @return {LH.Artifacts['ScriptElements']}
*/
/* istanbul ignore next */
function collectAllScriptElements() {
/** @type {HTMLScriptElement[]} */
// @ts-ignore - getElementsInDocument put into scope via stringification
const scripts = getElementsInDocument('script'); // eslint-disable-line no-undef
function collectScriptElements() {
const scripts = getElementsInDocument('script');

return scripts.map(script => {
return {
Expand All @@ -30,7 +26,6 @@ function collectAllScriptElements() {
async: script.async,
defer: script.defer,
source: /** @type {'head'|'body'} */ (script.closest('head') ? 'head' : 'body'),
// @ts-ignore - getNodePath put into scope via stringification
devtoolsNodePath: getNodePath(script),
content: script.src ? null : script.text,
requestId: null,
Expand Down Expand Up @@ -72,12 +67,13 @@ class ScriptElements extends Gatherer {
const driver = passContext.driver;
const mainResource = NetworkAnalyzer.findMainDocument(loadData.networkRecords, passContext.url);

/** @type {LH.Artifacts['ScriptElements']} */
const scripts = await driver.evaluateAsync(`(() => {
${getElementsInDocumentString}
${pageFunctions.getNodePathString};
return (${collectAllScriptElements.toString()})();
})()`, {useIsolation: true});
const scripts = await driver.evaluate(collectScriptElements, {
useIsolation: true,
deps: [
getElementsInDocument,
pageFunctions.getNodePathString,
],
});

for (const script of scripts) {
if (script.content) script.requestId = mainResource.requestId;
Expand Down
68 changes: 34 additions & 34 deletions lighthouse-core/gather/gatherers/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const tapTargetsSelector = TARGET_SELECTORS.join(',');

/**
* @param {HTMLElement} element
* @returns {boolean}
* @return {boolean}
*/
/* istanbul ignore next */
function elementIsVisible(element) {
Expand All @@ -47,7 +47,7 @@ function elementIsVisible(element) {

/**
* @param {Element} element
* @returns {LH.Artifacts.Rect[]}
* @return {LH.Artifacts.Rect[]}
*/
/* istanbul ignore next */
function getClientRects(element) {
Expand All @@ -69,17 +69,18 @@ function getClientRects(element) {

/**
* @param {Element} element
* @returns {boolean}
* @param {string} tapTargetsSelector
* @return {boolean}
*/
/* istanbul ignore next */
function elementHasAncestorTapTarget(element) {
function elementHasAncestorTapTarget(element, tapTargetsSelector) {
if (!element.parentElement) {
return false;
}
if (element.parentElement.matches(tapTargetsSelector)) {
return true;
}
return elementHasAncestorTapTarget(element.parentElement);
return elementHasAncestorTapTarget(element.parentElement, tapTargetsSelector);
}

/**
Expand Down Expand Up @@ -123,7 +124,7 @@ function hasTextNodeSiblingsFormingTextBlock(element) {
* Makes a reasonable guess, but for example gets it wrong if the element is surrounded by other
* HTML elements instead of direct text nodes.
* @param {Element} element
* @returns {boolean}
* @return {boolean}
*/
/* istanbul ignore next */
function elementIsInTextBlock(element) {
Expand Down Expand Up @@ -177,7 +178,7 @@ function elementCenterIsAtZAxisTop(el, elCenterPoint) {
/**
* Finds all position sticky/absolute elements on the page and adds a class
* that disables pointer events on them.
* @returns {() => void} - undo function to re-enable pointer events
* @return {() => void} - undo function to re-enable pointer events
*/
/* istanbul ignore next */
function disableFixedAndStickyElementPointerEvents() {
Expand All @@ -202,10 +203,11 @@ function disableFixedAndStickyElementPointerEvents() {
}

/**
* @returns {LH.Artifacts.TapTarget[]}
* @param {string} tapTargetsSelector
* @return {LH.Artifacts.TapTarget[]}
*/
/* istanbul ignore next */
function gatherTapTargets() {
function gatherTapTargets(tapTargetsSelector) {
/** @type {LH.Artifacts.TapTarget[]} */
const targets = [];

Expand All @@ -223,7 +225,7 @@ function gatherTapTargets() {
const tapTargetsWithClientRects = [];
tapTargetElements.forEach(tapTargetElement => {
// Filter out tap targets that are likely to cause false failures:
if (elementHasAncestorTapTarget(tapTargetElement)) {
if (elementHasAncestorTapTarget(tapTargetElement, tapTargetsSelector)) {
// This is usually intentional, either the tap targets trigger the same action
// or there's a child with a related action (like a delete button for an item)
return;
Expand Down Expand Up @@ -305,30 +307,28 @@ class TapTargets extends Gatherer {
* @return {Promise<LH.Artifacts.TapTarget[]>} All visible tap targets with their positions and sizes
*/
afterPass(passContext) {
const expression = `(function() {
const tapTargetsSelector = "${tapTargetsSelector}";
${pageFunctions.getElementsInDocumentString};
${disableFixedAndStickyElementPointerEvents.toString()};
${elementIsVisible.toString()};
${elementHasAncestorTapTarget.toString()};
${elementCenterIsAtZAxisTop.toString()}
${truncate.toString()};
${getClientRects.toString()};
${hasTextNodeSiblingsFormingTextBlock.toString()};
${elementIsInTextBlock.toString()};
${getRectArea.toString()};
${getLargestRect.toString()};
${getRectCenterPoint.toString()};
${rectContains.toString()};
${pageFunctions.getNodePathString};
${pageFunctions.getNodeSelectorString};
${pageFunctions.getNodeLabelString};
${gatherTapTargets.toString()};

return gatherTapTargets();
})()`;

return passContext.driver.evaluateAsync(expression, {useIsolation: true});
return passContext.driver.evaluate(gatherTapTargets, {
args: [tapTargetsSelector],
useIsolation: true,
deps: [
pageFunctions.getElementsInDocument,
disableFixedAndStickyElementPointerEvents,
elementIsVisible,
elementHasAncestorTapTarget,
elementCenterIsAtZAxisTop,
truncate,
getClientRects,
hasTextNodeSiblingsFormingTextBlock,
elementIsInTextBlock,
getRectArea,
getLargestRect,
getRectCenterPoint,
rectContains,
pageFunctions.getNodePath,
pageFunctions.getNodeSelectorString,
pageFunctions.getNodeLabelString,
],
});
}
}

Expand Down
Loading