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

Migrate evaluateAsync to Runtime.evaluate(awaitPromise) #793

Merged
merged 1 commit into from
Oct 20, 2016
Merged
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
55 changes: 17 additions & 38 deletions lighthouse-core/gather/drivers/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,50 +144,29 @@ class Driver {
});
}

/**
* Evaluate an expression in the context of the current page. Expression must
* evaluate to a Promise. Returns a promise that resolves on asyncExpression's
* resolved value.
* @param {string} asyncExpression
* @return {!Promise<*>}
*/
evaluateAsync(asyncExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

While you're in here, would you mind adding a doc string? Maybe something like

/**
 * Evaluate an expression in the context of the current page. Expression must
 * evaluate to a Promise. Returns a promise that resolves on asyncExpression's
 * resolved value.
 * @param {string} asyncExpression
 * @return {!Promise<*>}
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return new Promise((resolve, reject) => {
let asyncTimeout;

// Inject the call to capture inspection.
const expression = `(function() {
const __inspect = inspect;
const __returnResults = function(results) {
__inspect(JSON.stringify(results));
};
${asyncExpression}
})()`;

const inspectHandler = value => {
if (asyncTimeout !== undefined) {
clearTimeout(asyncTimeout);
}

// If the returned object doesn't meet the expected pattern, bail with an undefined.
if (value === undefined ||
value.object === undefined ||
value.object.value === undefined) {
return resolve();
}

return resolve(JSON.parse(value.object.value));
};

// COMPAT: Chrome 52 is when Runtime.inspectRequested became available
// https://codereview.chromium.org/1866213002
// Previously, a similar-looking Inspector.inspect event was available, but unfortunately
// it will not fire in this scenario.
this.once('Runtime.inspectRequested', inspectHandler);

this.sendCommand('Runtime.evaluate', {
expression,
includeCommandLineAPI: true
}).catch(reject);

// If this gets to 60s and it hasn't been resolved, reject the Promise.
asyncTimeout = setTimeout(
const asyncTimeout = setTimeout(
(_ => reject(new Error('The asynchronous expression exceeded the allotted time of 60s'))),
60000
);
this.sendCommand('Runtime.evaluate', {
expression: asyncExpression,
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true
}).then(result => {
clearTimeout(asyncTimeout);
resolve(result.result.value);
}).catch(reject);
});
}

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 @@ -16,7 +16,7 @@
*/
'use strict';

/* global document, __returnResults */
/* global document */

const Gatherer = require('./gatherer');
const fs = require('fs');
Expand All @@ -27,9 +27,8 @@ const axe = fs.readFileSync(
// This is run in the page, not Lighthouse itself.
/* istanbul ignore next */
function runA11yChecks() {
axe.a11yCheck(document, function(results) {
// __returnResults is magically inserted by driver.evaluateAsync
__returnResults(results);
return new Promise((resolve, reject) => {
axe.a11yCheck(document, resolve);
});
}

Expand Down
9 changes: 3 additions & 6 deletions lighthouse-core/gather/gatherers/cache-contents.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@
*/
'use strict';

/* global __returnResults, caches */
/* global caches */

const Gatherer = require('./gatherer');

// This is run in the page, not Lighthouse itself.
/* istanbul ignore next */
function getCacheContents() {
// Get every cache by name.
caches.keys()
return caches.keys()

// Open each one.
.then(cacheNames => Promise.all(cacheNames.map(cacheName => caches.open(cacheName))))
Expand All @@ -39,11 +39,8 @@ function getCacheContents() {
requests.push(...reqs.map(r => r.url));
});
})).then(_ => {
// __returnResults is magically inserted by driver.evaluateAsync
__returnResults(requests);
return requests;
});
}).catch(_ => {
__returnResults(undefined);
});
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/gather/gatherers/content-width.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@

const Gatherer = require('./gatherer');

/* global window, __returnResults */
/* global window */

/* istanbul ignore next */
function getContentWidth() {
// window.innerWidth to get the scrollable size of the window (irrespective of zoom)
// window.outerWidth to get the size of the visible area
__returnResults({
return Promise.resolve({
scrollWidth: window.innerWidth,
viewportWidth: window.outerWidth
});
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/gather/gatherers/geolocation-on-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const Gatherer = require('./gatherer');
* @author Paul Lewis
*/

/* global navigator, window, __returnResults */
/* global navigator, window */

/* istanbul ignore next */
function overrideGeo() {
Expand All @@ -39,7 +39,7 @@ function overrideGeo() {
}

function collectGeoState() {
__returnResults(window.__didNotCallGeo);
return Promise.resolve(window.__didNotCallGeo);
}

class GeolocationOnStart extends Gatherer {
Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/gather/gatherers/html-without-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@

const Gatherer = require('./gatherer');

/* global document, __returnResults */
/* global document */

/* istanbul ignore next */
function getBodyText() {
// note: we use innerText, not textContent, because textContent includes the content of <script> elements!
const body = document.querySelector('body');
// __returnResults is magically inserted by driver.evaluateAsync
__returnResults(body ? body.innerText : '');
return Promise.resolve(body ? body.innerText : '');
}

class HTMLWithoutJavaScript extends Gatherer {
Expand Down