Open connorjclark opened 4 years ago
patrickhulce
commented
4 years ago possibly also add a small polyfill to fix the toString issue (taking similar approach as babel/preset-modules and attempt to upstream to preset-env)
I think this is an awesome idea. Even if it's 10x the time/effort on our part, getting the default module/nomodule build path to not pull in a polyfill for all typedarray behavior will have 100x the impact. 👍
connorjclark
commented
4 years ago connorjclark
commented
4 years ago Maybe going all-in on the module/nomodule pattern as it is today for our recommendation was too much of a half measure. perhaps we could expand upon the pattern:
esmodules: false or w/e.esmodules: true but for polyfills use a much more recent set of browser targets, feature detect at runtime, and asynchronously load what's needed.Just spit-balling. I think we can continue w/ legacy-javascript audit as-is, use it as an introduction to differential loading, and improve upon it later.
cc @philipwalton
patrickhulce
commented
4 years ago See this. bah.
That is definitely a ridiculous Safari bug, but also extraordinarily narrow. On top of using typedarray specifically with arraybuffer initialization you need to pass undefined explicitly as the length and not just omit the argument.
but for polyfills use a much more recent set of browser targets, feature detect at runtime, and asynchronously load what's needed.
SGTM, I imagine we'll need to update web.dev docs with this approach e
come up with a nice way to generate multiple bundles of varying browser targets and the necessary detection on the server for picking which to serve to a useragent. We have been avoiding this due to the complexity of involving the server, but perhaps it's unavoidable.
I'm definitely less a fan of this approach.
I think we can continue w/ legacy-javascript audit as-is, use it as an introduction to differential loading, and improve upon it later.
Agreed. Does this mean that you're suggesting still flagging TypedArray polyfills and saying "figure out how to not load this your own way" or that you want to hide them for now until we have a plan to surface them later?
connorjclark
commented
4 years ago SGTM, I imagine we'll need to update web.dev docs with this approach e
The issue is that "feature detect at runtime, and asynchronously load what's needed" is not supported by core-js. And the detection is complex; it goes beyond just "is X defined", as you can see in the typed array case. Same for Promise.
you write:
await import('core-js/async') // doesn't exist
app.init()babel'd to something like:
const corejs = await import('core-js/async') // doesn't exist
const missingModules = corejs.checkFor([...list of features...]) // doesn't exist
await Promise.all(missingModules.map(import)) // core-js polyfills share many internal modules so this is still lacking ...
app.init()but as I said feature detection in core-js is coupled with the actual polyfilling so this isn't possible today.
I haven't looked at alternative polyfill libraries much yet. I did look at polyfill.io. Turns out they also ship TypedArrays (if requested) to all browsers ... but that seems like a bug. anyway, polyfill.io still requires modern browsers to do a request so I don't want to go that route.
Agreed. Does this mean that you're suggesting still flagging TypedArray polyfills and saying "figure out how to not load this your own way" or that you want to hide them for now until we have a plan to surface them later?
I'd prefer our first pass only require the user (in order to pass the audit...) to turn on babel preset-env's esmoudles, bugfixes and employ the module/nomodule pattern.
It's not clear to me how a site using typed arrays and needing to support Safari can avoid that polyfill. would certainly want to test some sites using TypedArrays on those browsers first to confirm if that bug is as rare as it seems
patrickhulce
commented
4 years ago The automagic version of this doesn't exist for sure, but you can manually do this sort of thing. Documenting how to go about doing it yourself or that 80KiB is simply because of two annoying safari bugs you probably won't run into seems worthwhile.
I'd prefer our first pass only require the user (in order to pass the audit...) to turn on babel preset-env's esmoudles, bugfixes and employ the module/nomodule pattern.
I agree, just making sure we're on the same page 👍
would certainly want to test some sites using TypedArrays on those browsers first to confirm if that bug is as rare as it seems
I'm definitely interested in finding other solutions here. Static analysis would be enough of a first pass, IMO. If anything invokes a TypedArray constructor with 3 arguments then they're potentially exposed. Typescript would know for sure :)
paulirish
commented
4 years ago For LH pov, we start with heavily recommending @babel/preset-env + esmodules: true + bugfixes: true.
Ideally we can resolve the promises/typedarrays polyfilling from within the babel-preset-env bugfixes path. An alternative is recommending some exclude patterns, but it's not guaranteed to be totally fine.
As of today,
@babel/preset-env+esmodules: true+bugfixes: truewill apply these polyfills and transforms:That means, if you follow our planned module/nomodule strategy, the modern bundle will still ship Promise and TypedArrays.
Polyfilling just the typed-arrays and Promise is about 85KB:
AFAICT, typed-arrays are included either because 1) a bug in
.toStringin all versions of safari or 2) it is mistakenly not given a constraint for safari hereSee the bottom of https://github.com/GoogleChrome/lighthouse/pull/10937#issuecomment-641744844 for why Promises are included.
Actions we can take:
1) Do nothing, be ok with large polyfills in "modern" bundles 2) Suggest excluding (via babel/preset-env
exclude) type-arrays in the modern bundle, possibly also add a small polyfill to fix thetoStringissue (taking similar approach asbabel/preset-modulesand attempt to upstream to preset-env). Would be good to understand failure modes for this bug in older browsers. 3) Suggest excluding Promise in the modern bundle. Again, maybe we can create a smaller polyfill to fix thethenable w/ errorbug. (EDIT: actually this module would still get included, because other features like promise.finnaly, allSettled, etc pull in the promise module too ...)Related: https://github.com/GoogleChrome/web.dev/issues/3129 cc @housseindjirdeh Context: https://github.com/GoogleChrome/lighthouse/pull/10937#issuecomment-642320005