tests: handwrite the bad promise polyfill #11190
Conversation
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
This reverts commit e8196ef.
| @@ -148,9 +148,12 @@ function collectImageElementInfo() { | |||
| */ | |||
| /* istanbul ignore next */ | |||
| function determineNaturalSize(url) { | |||
| return new Promise((resolve, reject) => { | |||
| // @ts-expect-error nativePromise installed by driver.cacheNatives() | |||
There was a problem hiding this comment.
oof, do we lose all typechecking of promise types with this? can we
declare global {
interface Window {
__nativePromise: typeof Promise
}
}
or something to make it work?
There was a problem hiding this comment.
def this, if we don't do what I suggest in my comment above.
| */ | ||
|
|
||
| /** | ||
| * Lighthouse backstory: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -46,7 +46,7 @@ function installMediaListener() { | |||
| */ | |||
| /* istanbul ignore next */ | |||
| function collectTagsThatBlockFirstPaint() { | |||
| return new Promise((resolve, reject) => { | |||
| return new __nativePromise((resolve, reject) => { | |||
There was a problem hiding this comment.
This is really awful DX, and ripe for errors (proof is that you found 5 cases where we missed this!)
What if, in Driver.evaluateAsync, we did something like...
(() => {
var Promise = __nativePromise;
var Error = __nativeError;
(${pageFunCode})();
})();
ie. create a scope where we define Promise, Error, etc. vars, so any code we pass in will resolve to these variables over the global objects?
(not in this PR, but I could follow up...)
wdyt?
There was a problem hiding this comment.
we already do this for URL, I'm not sure we should split into another PR when this is the first PR that even starts using these outside driver in the first place.
looks like @paulirish already agrees this is a good idea, how do you feel about reverting these changes and typedefs in favor of connor's suggestion? (if they're necessary to land I guess we can't split, but if they were just gravy on top, revert only?)
There was a problem hiding this comment.
Patrick sorted out the native Promise stuff in #12381
This also completes the colocation of all the cached native definition and usage (except ElementMatches), so no need to reference window.__nativePromise ever again :D
| @@ -148,9 +148,12 @@ function collectImageElementInfo() { | |||
| */ | |||
| /* istanbul ignore next */ | |||
| function determineNaturalSize(url) { | |||
| return new Promise((resolve, reject) => { | |||
| // @ts-expect-error nativePromise installed by driver.cacheNatives() | |||
There was a problem hiding this comment.
def this, if we don't do what I suggest in my comment above.
…und-4 -- KEEPING CONFLICTS
…esponseCompression
aggressive-polyfil was the cause for both of those
|
Inspired by the changes and comment in #12932 I resurrected this badboy and addressed all comments |
|
We'll need an alternative to |
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
| the `prepareStackTrace` API. https://github.com/GoogleChrome/lighthouse/issues/1194 | ||
| However, since https://github.com/GoogleChrome/lighthouse/pull/5509, | ||
| we haven't used that API. So, no need to replicate the problem anymore. This comment only remains | ||
| as documentation for the legacy __nativeError. |
There was a problem hiding this comment.
do we even need that? feels like a comment on this PR that removes it could suffice :)
There was a problem hiding this comment.
sg. i moved it to #5509
| @@ -158,7 +158,8 @@ function collectImageElementInfo() { | |||
| function determineNaturalSize(url) { | |||
| return new Promise((resolve, reject) => { | |||
| const img = new Image(); | |||
| img.addEventListener('error', _ => reject(new Error('determineNaturalSize failed img load'))); | |||
| img.addEventListener('error', _ => | |||
|
There's a lot of deltas in the sample artifacts json / lhr that relied on that giant file to provide some results. And so without it, we're left with a lot of passing audits where it was formerly failing. In other words, we lose some bit of coverage here. Any thing in our node_modules ( |
discussion here: https://github.com/GoogleChrome/lighthouse/pull/11043/files#r448038184
aggressive-promise-polyfill is annoying because of its third_party-ness and no good place to put it.
so we're replacing it with a handwritten one.
i updated the Lighthouse Lore of this in the comments of the polyfill file.
Also, I found 5 cases where we were using potentially-modified version of Promise/Error constructors. We're unaware of any problems arising from that, but it was an easy fix to use the safe versions.
edit (sept 8 2021): see also the polyfill changes from #12932