-
Notifications
You must be signed in to change notification settings - Fork 9.7k
fix: evaluateAsync behavior #1037
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
Changes from 1 commit
46b8301
11ddfa7
ca5091d
c8e14c7
1343e61
a97a2ea
7c5995f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,16 +145,15 @@ class Driver { | |
| ); | ||
|
|
||
| this.sendCommand('Runtime.evaluate', { | ||
| // 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() { | ||
|
Contributor
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. 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?
Contributor
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. should also update the function docs
Collaborator
Author
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. yes each has a purpose try/catch - for errors that happen outside of promises but as I'm typing this just remembering that we opted for
Member
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. wanna add a comment to document this? we're definitely gonna be headscratching if we need to touch this code again. :)
Collaborator
Author
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 :) |
||
| const __nativePromise = window.__nativePromise || Promise; | ||
| return new __nativePromise(function(resolve) { | ||
| const wrapError = ${wrapRuntimeEvalErrorInBrowser.toString()}; | ||
| try { | ||
| __nativePromise.resolve(${asyncExpression}).then(resolve, wrapError); | ||
| } catch (e) { | ||
| wrapError(e); | ||
| } | ||
| }); | ||
| return __nativePromise.resolve() | ||
| .then(_ => ${asyncExpression}) | ||
| .catch(${wrapRuntimeEvalErrorInBrowser.toString()}); | ||
| }())`, | ||
| includeCommandLineAPI: true, | ||
| awaitPromise: true, | ||
|
|
@@ -737,16 +736,15 @@ function captureJSCallUsage(funcRef, set) { | |
| * istanbul ignore next | ||
| */ | ||
| function wrapRuntimeEvalErrorInBrowser(err) { | ||
| /* global resolve */ | ||
| err = err || new Error(); | ||
| const fallbackMessage = typeof err === 'string' ? err : 'unknown error'; | ||
|
|
||
| resolve({ | ||
| return { | ||
| __failedInBrowser: true, | ||
| name: err.name || 'Error', | ||
| message: err.message || fallbackMessage, | ||
| stack: err.stack || (new Error()).stack, | ||
| }); | ||
| }; | ||
| } | ||
|
|
||
| module.exports = Driver; | ||
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.
maybe add a comment here for why this is done?
I also feel like this is also going to be a long term resident in
static-server, so a full if statement is probably better so it's a bit quicker mentally parseThere 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