emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.83k stars 3.31k forks source link

Framework for including JS polyfills in emscripten output #15938

Open sbc100 opened 2 years ago

sbc100 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/emscripten-core/emscripten/issues/12203 and one reason I've not started on that is because I'm not sure how best to answer these questions.

sbc100 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.)