-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Changes from 27 commits
90018b2
f7e38f5
78f386a
32856e7
e2dd63d
f38bf41
ce9839b
4c903de
0413a4a
d4bd2bf
02dc215
ce2827a
c81a636
70d8210
df5e8dd
4e59476
147b98c
a4051a1
9eea206
d79b23f
b8c76ed
2088109
7b01e8c
4cc1eea
aae0a9f
77af88d
d35fd5e
9c10cda
d643baf
9638b90
c526772
b893bb1
606db62
8c98d05
f1c0afe
05ba735
9545ab4
668d04e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra defensive coding, or am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -92,18 +92,23 @@ class FullPageScreenshot extends Gatherer { | |
|
||
return nodes; | ||
} | ||
const expression = `(function () { | ||
${pageFunctions.getBoundingClientRectString}; | ||
return (${resolveNodes.toString()}()); | ||
})()`; | ||
|
||
/** | ||
* @param {boolean} useIsolation | ||
*/ | ||
function evaluateInPage(useIsolation) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Bump, WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: false). Gatherers use both modes when collecting node details, | ||
// context (`useIsolation: false`). 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}; | ||
} | ||
|
||
|
@@ -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.baseArtifacts.TestedAsMobileDevice, // could easily be wrong | ||
...observedDeviceMetrics, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yikes! almost missed this. forgot to include |
||
args: [], | ||
deps: [ | ||
pageFunctions.getElementsInDocumentString, | ||
pageFunctions.isPositionFixedString, | ||
pageFunctions.getNodeDetailsString, | ||
], | ||
}); | ||
return iframeElements; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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 toevaluate
) return promises. see microsoft/TypeScript#27711 (comment) for more