-
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
fix: evaluateAsync behavior #1037
Changes from all commits
46b8301
11ddfa7
ca5091d
c8e14c7
1343e61
a97a2ea
7c5995f
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 |
---|---|---|
|
@@ -293,5 +293,6 @@ <h2>Do better web tester page</h2> | |
} | ||
</script> | ||
|
||
<script src="/promise_polyfill.js"></script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,30 +129,44 @@ 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 | ||
* Evaluate an expression in the context of the current page. | ||
* Returns a promise that resolves on the expression's value. | ||
* @param {string} expression | ||
* @return {!Promise<*>} | ||
*/ | ||
evaluateAsync(asyncExpression) { | ||
evaluateAsync(expression) { | ||
return new Promise((resolve, reject) => { | ||
// If this gets to 60s and it hasn't been resolved, reject the Promise. | ||
const asyncTimeout = setTimeout( | ||
(_ => reject(new Error('The asynchronous expression exceeded the allotted time of 60s'))), | ||
60000 | ||
); | ||
|
||
this.sendCommand('Runtime.evaluate', { | ||
expression: asyncExpression, | ||
// We need to wrap the raw expression for several purposes | ||
// 1. Ensure that the expression will be a native Promise and not a polyfill/non-Promise. | ||
// 2. Ensure that errors captured in the Promise are converted into plain-old JS Objects | ||
// so that they can be serialized properly b/c JSON.stringify(new Error('foo')) === '{}' | ||
expression: `(function wrapInNativePromise() { | ||
const __nativePromise = window.__nativePromise || Promise; | ||
return __nativePromise.resolve() | ||
.then(_ => ${expression}) | ||
.catch(${wrapRuntimeEvalErrorInBrowser.toString()}); | ||
}())`, | ||
includeCommandLineAPI: true, | ||
awaitPromise: true, | ||
returnByValue: true | ||
}).then(result => { | ||
clearTimeout(asyncTimeout); | ||
const value = result.result.value; | ||
|
||
if (result.exceptionDetails) { | ||
reject(result.exceptionDetails.exception.value); | ||
// An error occurred before we could even create a Promise, should be *very* rare | ||
reject(new Error('an unexpected driver error occurred')); | ||
} if (value && value.__failedInBrowser) { | ||
reject(Object.assign(new Error(), value)); | ||
} else { | ||
resolve(result.result.value); | ||
resolve(value); | ||
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. we can save for another issue (maybe this is also #941), but the fact that there are three cases here
makes it feel like we shouldn't be conflating the last two, but I'm not exactly sure of an elegant way to do this, or even what gatherers should do (catch or re-throw) if we do differentiate by type 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 think the differentiation is just done by what error message and stack trace results. Unless we think driver error should be fatal? 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. yeah, I think @paulirish is addressing this in his latest comments in #941. We basically need a way to say "this is an error I expected (fetch rejected on offline request or whatever)" vs "whoooops". For this, the caller of |
||
} | ||
}).catch(err => { | ||
clearTimeout(asyncTimeout); | ||
|
@@ -746,4 +760,23 @@ function captureJSCallUsage(funcRef, set) { | |
}; | ||
} | ||
|
||
/** | ||
* The `exceptionDetails` provided by the debugger protocol does not contain the useful | ||
* information such as name, message, and stack trace of the error when it's wrapped in a | ||
* promise. Instead, map to a successful object that contains this information. | ||
* @param {string|Error} err The error to convert | ||
* istanbul ignore next | ||
*/ | ||
function wrapRuntimeEvalErrorInBrowser(err) { | ||
err = err || new Error(); | ||
const fallbackMessage = typeof err === 'string' ? err : 'unknown error'; | ||
|
||
return { | ||
__failedInBrowser: true, | ||
name: err.name || 'Error', | ||
message: err.message || fallbackMessage, | ||
stack: err.stack || (new Error()).stack, | ||
}; | ||
} | ||
|
||
module.exports = Driver; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,10 @@ const path = require('path'); | |
* C. GatherRunner.setupDriver() | ||
* i. assertNoSameOriginServiceWorkerClients | ||
* ii. beginEmulation | ||
* iii. cleanAndDisableBrowserCaches | ||
* iiii. clearDataForOrigin | ||
* iii. enableRuntimeEvents | ||
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill | ||
* v. cleanAndDisableBrowserCaches | ||
* vi. clearDataForOrigin | ||
* | ||
* 2. For each pass in the config: | ||
* A. GatherRunner.beforePass() | ||
|
@@ -90,6 +92,7 @@ class GatherRunner { | |
return driver.assertNoSameOriginServiceWorkerClients(options.url) | ||
.then(_ => driver.beginEmulation(options.flags)) | ||
.then(_ => driver.enableRuntimeEvents()) | ||
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;')) | ||
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. need to doc this in outline at top 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. done |
||
.then(_ => driver.cleanAndDisableBrowserCaches()) | ||
.then(_ => driver.clearDataForOrigin(options.url)); | ||
} | ||
|
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.
this feels like it could be simplified, but are there reasons for everything? Why wrap in a promise constructor and try/catch and Promise.resolve() instead of doing a single promise wrapper?
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.
should also update the function docs
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.
yes each has a purpose
try/catch - for errors that happen outside of promises
Promise.resolve - to enable sync executions
new Promise - to ensure the promise returned is indeed a native promise + avoid inconsistent error handling between sync and async paths
but as I'm typing this just remembering that we opted for
Promise.resolve().then( => )
, will fixThere 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.
wanna add a comment to document this? we're definitely gonna be headscratching if we need to touch this code again. :)
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.
done :)