Skip to content

Framework for including JS polyfills in emscripten output #15938

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

Closed
sbc100 opened this issue Jan 10, 2022 · 9 comments
Closed

Framework for including JS polyfills in emscripten output #15938

sbc100 opened this issue Jan 10, 2022 · 9 comments

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Jan 10, 2022

We currently have one polyfill that I know of in the form of src/promise_polyfill.js. I was proposing adding a second as part of #15822, and the discussion prompted me to open this issue.

It seems we lack a policy if how/when to include polyfill code in JS. This issue is opened to discuss what policy we should have.

Open questions:

  1. Should we prefer to polyfill the JS library or provide implementations under our own namespace?
  2. Should we allow the user to disable the output of all polyfills (i.e. maybe they want to provide their own centralized polyfills outside of emscripten-generated code).
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 10, 2022

I suggest that the answer is yes in both cases.

We should have a way to disable polyfills, but when needed, we I think we should polyfill in such a way that our library code reads in an idiomatic way. e.g. we write Promise rather then esPromise and Object.assign rather than esObjAssign.

@kripken
Copy link
Member

kripken commented Jan 11, 2022

We also have some Math polyfills I believe, and wasm2js includes a partial polyfill of the WebAssembly object too.

Regarding disabling polyfills, do you mean something more than the user not using a flag that requires a polyfill? Our default builds target new-enough browsers that no polyfills are needed, in particular. Or were you thinking that there would be a new flag "no polyfills" that would error if we try to polyfill anything?

I do agree that idiomatic code is more readable. But that does lead to a problem. If we do

if (typeof Promise == 'undefined') {
  Promise = ...

then we are modifying the global scope. Or, if we do

if (typeof Promise == 'undefined') {
  var Promise = ...

then that doesn't work as the var creates a scope and we always end up using the polyfill. So we do have the choice of either messing with the global scope - dangerous - or using a different name,

var ourPromise;
if (typeof Promise == 'undefined') {
  ourPromise = ...
} else {
  ourPromise = Promise;
}
// .. use ourPromise which is sadly not idiomatic

I wonder what the best practices for this are in the JS ecosystem. Maybe @RReverser and/or @curiousdannii have thoughts?

Note: This relates to #12203 and one reason I've not started on that is because I'm not sure how best to answer these questions.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 11, 2022

I'm suggesting a new flag such as "NO_POLYFILLS" which means "I will take care of polyfilling myself". When this option is enabled we would not output any polyfills. Users of this option be in charge of ensuring the polyfills were present via some other means.

This would be inline with what the closure compiler does which includes polyfills, as needed, but has an options to disable them (--rewrite_polyfills=false): https://github.com/google/closure-compiler/wiki/Polyfills.

@curiousdannii
Copy link
Contributor

I think polyfills should be opt-in, but using MIN_X_VERSION would count as opting in.

If you need any polyfills for current browsers, then they should probably be namespaced rather than implemented in the global scope. But to be safe you could do that for everything. But is that technically actually a "polyfill"? More like a shim? If you namespace them then they wouldn't need to be perfect implementations either, you could simpler smaller ones. I guess that could determine the answer to question 1: if you can save several KB by namespacing, then it's probably worth it.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 12, 2022

I think its important that we treat the polyfill case the exception... users opt into it by asking for older browser support.. if they would rather provide their own polyfills they can of course opt out of ours, but our code should use the idomatic/spec compliant names things (e.g. Promise, and not emPromise).

@RReverser
Copy link
Collaborator

then we are modifying the global scope.

If we're going to include full spec-compliant polyfills, then it shouldn't be a big deal and is pretty reasonable. However, if we want minimal shims, then approach of using local variable like var emPromise = typeof Promise !== 'undefined' ? Promise : customPolyfill would be the correct choice.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 12, 2022

I think we should include full spec compilant polyfills (In fact I think we should just use the closure-compiler ones directly where possible).

I don't think we should worry about trying to provide half-baked solution in order to save a few bytes for a very few users who want to opt into old browser support. If a user wants to opt into old browser support AND cares a lot about code size then can use NO_POLYFILLS and try to write smaller ones to match their needs.

@kripken
Copy link
Member

kripken commented Jan 14, 2022

Good points!

Yes, maybe we shouldn't care that much about code size for polyfills - we should focus on code size for the non-polyfill case. That means we might as well use idiomatic code. And when we do polyfill we can use a full spec-compliant one, so it's ok to modify the global scope. For both those reasons we we may as well modify the global scope, then.

And a new flag to disable polyfills sounds good, matching closure, for people that want more control.

(For #12203 I'm still not sure how to proceed. But perhaps the issue more relevant to there is shimming and not polyfilling to use the term you used @curiousdannii . Worth more thought there I guess.)

sbc100 added a commit that referenced this issue Jan 15, 2022
sbc100 added a commit that referenced this issue Jan 15, 2022
sbc100 added a commit that referenced this issue Jan 18, 2022
sbc100 added a commit that referenced this issue Feb 1, 2022
This gives a small code size saving and it makes our codebase
more readable.

The reason for this change is that I ran into an issue where I was
trying to use `objAssign` in more places and it became apparent
that MINIMAL_RUNTIME didn't define the `objAssign` helper.

Also, move the defintion of the polyfill inside of `#if POLYFILL` so
it can be disabled using `-sNO_POLYFILL`.

See #15823 #15938
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 9, 2025

Thankfully we have removed all but on of our polyfills now! The only remaining one is bigint64array.js

@sbc100 sbc100 closed this as completed Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants