-
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
For legacy-javascript advice, suggest excluding some polyfills for "modern" build #10945
Comments
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. 👍 |
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:
Just spit-balling. I think we can continue w/ |
That is definitely a ridiculous Safari bug, but also extraordinarily narrow. On top of using typedarray specifically with arraybuffer initialization you need to pass
SGTM, I imagine we'll need to update web.dev docs with this approach e
I'm definitely less a fan of this approach.
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? |
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:
babel'd to something like:
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.
I'd prefer our first pass only require the user (in order to pass the audit...) to turn on babel preset-env's 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 |
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 agree, just making sure we're on the same page 👍
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 :) |
For LH pov, we start with heavily recommending 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: true
will 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
.toString
in all versions of safari or 2) it is mistakenly not given a constraint for safari hereSee the bottom of #10937 (comment) for why Promises are included.
Actions we can take:
exclude
) type-arrays in the modern bundle, possibly also add a small polyfill to fix thetoString
issue (taking similar approach asbabel/preset-modules
and attempt to upstream to preset-env). Would be good to understand failure modes for this bug in older browsers.thenable w/ error
bug. (EDIT: actually this module would still get included, because other features like promise.finnaly, allSettled, etc pull in the promise module too ...)Related: GoogleChrome/web.dev#3129 cc @housseindjirdeh
Context: #10937 (comment)
The text was updated successfully, but these errors were encountered: