-
Couldn't load subscription status.
- Fork 356
[ php-wasm ] Add intl dynamic extension to @php-wasm/web
#2591
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
base: trunk
Are you sure you want to change the base?
Conversation
ce5ac2f to
9915b90
Compare
|
Little summary :
I finally found out the resulting
- var specialHTMLTargets = [0, document, window];
+ var specialHTMLTargets = [0, typeof document != 'undefined' ? document : 0, typeof window != 'undefined' ? window : 0];
/** @suppress {duplicate } */
var findEventTarget = (target) => {
target = maybeCStringToJsString(target);
- var domElement = specialHTMLTargets[target] || document.querySelector(target);
+ var domElement = specialHTMLTargets[target] || (typeof document != 'undefined' ? document.querySelector(target) : null);
return domElement;
};And running
|
|
I had in mind to also try to add |
Good find! It should be fine as long as we're not breaking loading that script on a regular web page (not in a worker). And if we are breaking it, that may still be fine, but let's acknowledge that and discuss any consequences. |
Let's just use a specific E2E testing setup. I've tried that in the past in this repo and |
f244496 to
f54d3c5
Compare
the
I implemented a I also managed to run the tests in JSPI and Asyncify separately by using |
|
I ended up only removing the I tried to add the |
|
@adamziel That's it I think! The first full dynamic extension with its associated tests in Node, Web and Playground. I will clean up the old artifacts from static Intl and Playground CLI in the next pull request, to keep this one clean. Should I leave |
| }, | ||
| }, | ||
| { | ||
| name: 'ignore-lib-imports', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated – can we move the extension module to packages/vite-extensions/ and import it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of this :
To fuse ignore-wasm-imports, ignore-lib-imports and ignore-data-imports into ignore-binary-imports and add it in the /vite-extensions directory. Load it in php-wasm-web vite.config.ts. Remove playground/ignore-wasm-imports.ts and playground/ignore-data-imports and use ignore-binary-imports from vite-extensions instead.
And as this was some modifications in multiple projects I wanted to make those modifications in a follow-up pull request. Is it ok for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single, configurable plugin sounds lovely. Let's just make sure we can still granularly configure each aspect of it for modules that need just one or two of these options (assuming we have any). A follow-up PR sounds lovely.
packages/php-wasm/web/vite.config.ts
Outdated
| * and not | ||
| * import("shared/icudt74l.js") | ||
| * | ||
| * The slice(-2) will ensure the 'public/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice(-6)? Or is the -6 wrong? It sounds like it would preserve more path segments than just shared/icudt74l.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icudt74l.js is in /public/shared while the intl.so files are in /public/php/{mode}/extensions/intl/{php_version} directories this is why it has to slice 6 times instead of 2 for icudt74l.js
| ): Promise<EmscriptenOptions> { | ||
| const extensionName = 'intl.so'; | ||
| const extensionPath = (await getIntlExtensionModule(version)).default; | ||
| const extension = await (await fetch(extensionPath)).arrayBuffer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be executed every time loadWebRuntime() is called. Let's cache the array response and reuse it when available instead of always calling fetch(). Doing it at the service worker level makes the most sense – it already handles invalidation and makes those files available in the offline mode. Perhaps it's even already happening and we just need to confirm that. In either case, let's clarify the behavior in a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mho22 this comment is still relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it
8370eeb to
e8d2988
Compare
|
Currently, only |
|
Thank you @mho22! |
|
I had to upgrade |
|
I found two issues to fix :
|
a7cba1e to
7469996
Compare
Yeah let's log it and leave it for another PR, we'll need to find the right option – probably |
…to version <= 1.55.0 needed to run tests with JSPI
7469996 to
4788d0c
Compare
|
All the tests seem to pass here - yay! Are we missing anything unrelated to test coverage? |
|
Ow. I haven't recompiled PHP.wasm web with the new emscripten version yet. I just finally succeeded to rebase the project with trunk. I am currently recompiling locally 🤞 . |
| RUN rm -rf /root/emsdk/upstream/emscripten && \ | ||
| git clone https://github.com/sbc100/emscripten.git /root/emsdk/upstream/emscripten && \ | ||
| cd /root/emsdk/upstream/emscripten && \ | ||
| git checkout main_module_static_v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a fixed commit reference to avoid any surprises
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you suggested earlier, we are cloning sbc100 own fork of the emscripten repository, so we (but who or how?) should probably also fork its fork since he could delete its own later, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also try to patch our emscripten version with : https://patch-diff.githubusercontent.com/raw/emscripten-core/emscripten/pull/25522.patch ? But we're on version 4.0.5 while the latest version is 4.0.18. Maybe I should try to update the version and apply the patch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That also makes sense 👍
|
It looks like |
|
OK, this is due to the |
| # Prevents "document is not defined" errors in Web Workers when using MAIN_MODULE. | ||
| # By default, Emscripten assumes MAIN_MODULE runs only in a web environment, | ||
| # so it injects WebGL and other browser-only APIs. Explicitly adding "worker" | ||
| # ensures the runtime is compatible with Web Workers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great comment
| export JSPI_ADD_IMPORTS=",fd_close"; \ | ||
| export JSPI_ADD_EXPORTS=",fd_close"; \ | ||
| fi; \ | ||
| export ASYNCIFY_FLAGS=" -s ASYNCIFY=2 -sSUPPORT_LONGJMP=wasm -fwasm-exceptions -sJSPI_IMPORTS=js_open_process,js_fd_read,js_waitpid,js_process_status,js_create_input_device,wasm_setsockopt,wasm_shutdown,wasm_close,wasm_recv,__syscall_fcntl64,js_flock,js_release_file_locks,js_waitpid$JSPI_ADD_IMPORTS -sJSPI_EXPORTS=php_wasm_init,wasm_sleep,wasm_read,emscripten_sleep,wasm_sapi_handle_request,wasm_sapi_request_shutdown,wasm_poll_socket,wrap_select,__wrap_select,select,php_pollfd_for,fflush,wasm_popen,wasm_read,wasm_php_exec,run_cli,wasm_recv$JSPI_ADD_EXPORTS -s EXTRA_EXPORTED_RUNTIME_METHODS=ccall,PROXYFS,wasmExports,_malloc,setErrNo "; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove _malloc and setErrNo exports? The latter one is used in the JS library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the new emscripten version I cloned. It crashed because it used the deprecated EXTRA_EXPORTED_RUNTIME_METHODS instead of the new EXPORTED_RUNTIME_METHODS and after that change it crashed because of _malloc and setErrNo :
11.15 error: /root/emsdk/upstream/emscripten/src/postlibrary.js: undefined exported symbol: "_malloc" in EXPORTED_RUNTIME_METHODS
11.15 error: /root/emsdk/upstream/emscripten/src/postlibrary.js: undefined exported symbol: "setErrNo" in EXPORTED_RUNTIME_METHODS
| description: 'The platform to build for', | ||
| }, | ||
| WITH_DEBUG: { | ||
| JSPI: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any chance this name wasn't updated somewhere and we're getting a mixed asyncify/jspi build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test-e2e-php-wasm-web-jspi and test-e2e-php-wasm-web-asyncify are passing so I looked elsewhere.
Inline comments aside, I also wonder if it's a result of having wasm files built with different emscripten versions 🤔 |
Yep! I replaced my previous Dockerfile implementation with the copy patching, and the crash stopped : - # NOTE: Temporary use PR #25522 version of Emscripten
- # to build php.wasm with MAIN_MODULE=2 ensurnig
- # correct build without increasing binary size.
- # RUN rm -rf /root/emsdk/upstream/emscripten && \
- git clone https://github.com/sbc100/emscripten.git /root/emsdk/upstream/emscripten && \
- cd /root/emsdk/upstream/emscripten && \
- git checkout main_module_static_v2
- # NOTE: Temporary use PR #25522 version of Emscripten
- # to build php.wasm with MAIN_MODULE=2 ensurnig
- # correct build without increasing binary size.
- # running bootstrap.py to install npm packages.
- RUN python3 /root/emsdk/upstream/emscripten/bootstrap.py
+ # NOTE: Use of Emscripten PR #25522 patch
+ # to build php.wasm with MAIN_MODULE=2 ensuring
+ # correct build without increasing binary size.
+ # npm required to run emscripten/bootstrap.py
+ COPY ./main_module_without_relocatable.patch /root/main_module_without_relocatable.patch
+ RUN cd /root/emsdk/upstream/emscripten && \
+ git apply /root/main_module_without_relocatable.patchWhile the patch is stored in I'll recompile PHP.wasm Web JSPI again and cross fingers. |
|
It was too easy to be true. The wasm file went back to being huge. I have to investigate. |
|
I think I've seen MAIN_MODULE somewhere, should it always be MAIN_MODULE=2 now? |
|
@adamziel Yes! The next step is |
5e0f913 to
4933318
Compare



Motivation for the change, related issues
This is a pull request to dynamically load Intl in PHP.wasm Web.
Related issues and pull requests
Issues
Pull requests
intldynamic extension to @php-wasm/node ASYNCIFY #2501 #2557intldynamic extension to @php-wasm/node JSPI #2501intlextension #2187Implementation details
MAIN_MODULEin node and webworkerto the [web] environmentignore-lib-importsVite pluginwasm-feature-detectto simulate JSPI mode enabled based on Cypress ENVTesting Instructions (or ideally a Blueprint)
CI
🧪 test-e2e-php-wasm-web-jspi
🧪 test-e2e-php-wasm-web-asyncify
Next steps