-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove legacy support for polyfilling Math.clz32, imul, trunc #5837
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
Conversation
…ath.fround; we now assume browsers are modern enough to have it
Added a test fix, and verified this passes all the tests locally, should be good to go. |
These are needed for IE11, which we still support at Wikipedia for ogv.js media playback. I could package the polyfills separately in a pinch though. |
I see, thanks. Well, we could easily add an option for this, |
A LEGACY_BROWSER_SUPPORT flag would be perfect for us. :) |
This reverts commit d820bc1.
…etc. when ASSERTIONS, warn at runtime if a polyfill was needed without that option set
Ok, added |
We're interested in using this option for Kiwix JS. But enabling this flag disables the output of a wasm binary when building with We ideally want a single compile that will support Firefox, Chrome, Edge and IE11 with this flag set, where the browsers that are capable of loading the binary will do so, and the others (which may include versions running in browser extensions) will load asm. Obviously we can work around this with browser capability detection, but that seems to mean having two different compiles. |
Ah yes, the LEGACY_VM_SUPPORT option now forces WASM off (now that it's default) whereas previously it didn't affect it... might be better to make it agnostic again. @kripken what do you think? @Jaifroid, beware that the single-build binaryen fallback method is a little flaky and is officially not recommended; I bit the bullet and went with two separate builds and runtime detection for ogv.js. It's a bit unsatisfying and complicates the build and deployment process, though. |
Thanks, @Brion , at the moment a dual build may be our only option if we want the extra speed of wasm on the latest browsers while still supporting the widest range of browsers in a single app. Like Wikipedia, Kiwix can't fully drop IE11 yet given the number of Windows 7 boxes still around, especially as Kiwix generally targets areas of the world where internet access may be flakey, expensive or controlled. I can't see an obvious reason why the legacy support option should stop the wasm side from building (especially with no warning about it): the math polyfill should only need to apply to the fallback, so shouldn't affect browsers which support wasm, no? |
Yeah, a dual build is best for this, see https://kripken.github.io/emscripten-site/docs/compiling/WebAssembly.html#codegen-effects - a single build with fallbacks has big compromises. @Brion Perhaps it makes sense to allow legacy support in a dual build, yeah... In other words, in a dual build it would not force wasm off. That seems reasonable, although I worry about the extra complexity somewhat. Thoughts? |
Many thanks @kripken . We're leaning towards dual build over at Kiwix JS. What I would suggest at least is that emcc should spit out a message if using the LEGACY_VM_SUPPORT flag together with a set of wasm options (e.g. WASM=1) to say that it's reverting to asm only. It does this in other instances of such conflict. |
Good point @Jaifroid . Actually thinking more on this, I believe we should go even further - a warning would not be shown for people not specifying wasm and just assuming they'd get it by default, as it is the default now, and that is even more confusing... so I suggest that we just throw an error if wasm is on and legacy support is enabled, and the error can explain that it must be used without wasm. I opened #6635 for discussion. |
These have been present in modern browsers for quite some time, see compatibility sections in
There is some risk in removing these polyfills, though, as IE doesn't support them (Edge of course does), and possibly very old mobile browsers don't (I couldn't find an online resource on that). If there are concerns, this could be behind a flag.