fix: Promise polyfill for zone #1178
Conversation
Fixes evaluateAsync behavior when [zone.js](https://github.com/angular/zone.js) has mucked with Promise. Adds zone.js to the smoke tests so regression won't happen again. Addresses #1177.
ebidel
left a comment
ebidel
left a comment
There was a problem hiding this comment.
Should we bring in zones via npm?
lighthouse-core/gather/driver.js
Outdated
|
|
||
| this.sendCommand('Runtime.evaluate', { | ||
| // We need to wrap the raw expression for several purposes | ||
| // We need to expliticly wrap the raw expression for several purposes: |
There was a problem hiding this comment.
oops, done :)
Looks like you didn't have to modify @patrickhulce, so this could work this time? |
|
yeah true I didn't this time around, will fix |
| const thirdPartyPath = '../../../lighthouse-core/third_party'; | ||
| absoluteFilePath = path.join(__dirname, `${thirdPartyPath}/promise-polyfill/promise.js`); | ||
| // We bring in an aggressive Promise polyfill (zone) to ensure we don't still fail. | ||
| const zonePath = '../../../node_modules/zone.js'; |
There was a problem hiding this comment.
@ebidel not sure what you're requesting here, to change zonePath to nodeModule path?
There was a problem hiding this comment.
OH nvm. This is probably where it gets installed. Just looks funny from a reviewer's perspective :)
| .then(_ => ${expression}) | ||
| .catch(${wrapRuntimeEvalErrorInBrowser.toString()}) | ||
| .then(resolve); | ||
| }); |
There was a problem hiding this comment.
I tried a few permutations of this and it looks like this is about the only decent one that works :) Reason in #1173 (comment).
For anyone following along, the nice thing about this form is that it lets the polyfill handle resolving to a value (or error), and then all we have to assume is that our __nativePromise constructor is intact, not our earlier assumption that its prototype chain had also not been altered
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
Looks great. Nice detective work.
…Chrome#1178) * Fix `driver.evaluateAsync` behavior when [zone.js](https://github.com/angular/zone.js) has mucked with Promise. * Add zone.js to the smoke tests so regression won't happen again. * Remove old testing promise-polyfill
3 participants
R: @brendankenny
Fixes evaluateAsync behavior when zone.js has mucked with Promise.
Adds zone.js to the smoke tests so regression won't happen again.
Addresses #1173