Ensure compatibility with Safari for multi-environment ES6 builds#18482
Ensure compatibility with Safari for multi-environment ES6 builds#18482sbc100 merged 3 commits intoemscripten-core:mainfrom
Conversation
Use a different parameter name to prevent the variable from being incorrectly hoisted in JavaScriptCore-based engines (such as Safari). See: https://bugs.webkit.org/show_bug.cgi?id=223533. Regressed for multi-environment ES6 builds since commit ce4c405 (Emscripten 3.1.27). `-sENVIRONMENT=web` is not affected, as that would avoid the emit of this async function altogether. Resolves: emscripten-core#18357.
|
FWIW, as noticed in comment #17915 (comment) this |
emcc.py
Outdated
| %(maybe_async)sfunction(%(EXPORT_NAME)s) { | ||
| %(EXPORT_NAME)s = %(EXPORT_NAME)s || {}; | ||
| %(maybe_async)sfunction(_%(EXPORT_NAME)s) { | ||
| %(EXPORT_NAME)s = _%(EXPORT_NAME)s || {}; |
There was a problem hiding this comment.
Should we completely rename this to something like "params", or "args".
Don't we also then need the var on the next line since its declaring a new var?
There was a problem hiding this comment.
Should we completely rename this to something like "params", or "args".
Ah, that would probably be neater. Given that this object contains Emscripten-specific configuration (such as settings and/or callbacks), how about naming this config instead? I suppose it could still cause issues when someone uses -sEXPORT_NAME=config, but I'm not sure if that's a valid use-case (since values specified in EXPORT_NAME usually start with a uppercase letter).
Don't we also then need the
varon the next line since its declaring a new var?
You're right, let me fix that after the CI finishes on commit ffb8149.
|
Before marking this as non-draft, I just build a example program with: $ emcc -O3 test/hello_world.c -o test.html -sEXPORT_ES6On this PR and published the produced Wasm binary, HTML and JS file in: To test this on Safari, just do: $ git clone https://gist.github.com/08f7e6c2c978e5581f52a40a04225ab0.git pr-18482-test
$ cd pr-18482-test/
$ emrun --port 8080 --no_browser .
# Visit http://localhost:8080/test.html in SafariCan anyone verify that this works correctly in Safari? If it works correctly (i.e. the |
|
Would be great to get confirmation, but also I think we can land this change regardless as it seems like an simple improvement anyway. |
Indeed, let me remove the draft status. :) |
3 participants
Use a different parameter name to prevent the variable from being incorrectly hoisted in JavaScriptCore-based engines (such as Safari). See: https://bugs.webkit.org/show_bug.cgi?id=223533.
Regressed for multi-environment ES6 builds since commit ce4c405 (Emscripten 3.1.27).
-sENVIRONMENT=webis not affected, as that would avoid the emit of this async function altogether.Resolves: #18357.
Opened as a draft because I do not have access to macOS devices nor have I tested this locally with JSC.