-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
feat: add wasm support for test and SSR #21102
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: main
Are you sure you want to change the base?
Conversation
sapphi-red
left a comment
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.
Thanks, but this isn't working for build.
When I run vite build --ssr src/app.js in playground/ssr-wasm, I expect the wasm files to be copied (or inlined depending on the option) to dist. But that didn't happen.
| return result | ||
| } | ||
| } | ||
| if (relative && !ssr) { |
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 have to admit I have no idea why this condition was written this way before. I can see it was initially added in #8762 but couldn't figure out why it was there. Changing this also doesn't seem to fail any test, so I suppose it isn't very critical.
I think it makes more sense for SSR to always use relative path for building. While client code can use either absolute path or relative path, server code almost definitely need relative path, since files have to be accessed through filesystem rather than network request, and the current directory the process starts from is often not reliable for resolving paths.
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 think this is to generate the same path in both client build and SSR build. That is the expected behavior by meta-frameworks. The failure in the downstream project looks like this: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/19456693621/job/55671884733#step:7:508
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.
Ah, I see. This is quite challenging. There are three potential solutions I have in mind:
- Only use relative if
ssrEmitAssetsis set. - Only use relative if the file is a
.wasmfile. - A combination of both (use relative if both emitting is enabled and the file is a
.wasmfile).
The main conflict here is probably that WebAssembly files, while can be considered as assets given they are binary files, also involve in computation, unlike most other assets where the code really just need a reference to pass to the browser.
That being said, there could still be cases where ssr code wants to read from assets, for example, a page that renders background color based on an image asset. Those are probably more edge cases, though.
I'm leaning towards the first option so that we don't treat wasm too specially, and if one decides that ssr should emit assets, it could be a good signal that the code may want to read from it rather than just passing a reference to browser.
WDYT? Do you have any other options in mind?
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.
Actually, some meta-frameworks uses ssrEmitAssets: false to avoid processing the assets twice while expecting the URL to reference the asset's URL (which I think needs to be fixed in the future though).
I think we need to treat this PR's case specially for now. We need to keep the normal wasm asset URLs as-is for compatibility.
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.
By “if ssrEmitAssets is set”, I mean ssrEmitAssets === true, since otherwise the wasm file wouldn't even present.
I can look into treating it specially and see how it goes.
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.
By “if
ssrEmitAssetsis set”, I meanssrEmitAssets === true, since otherwise the wasm file wouldn't even present.
Some meta-frameworks uses ssrEmitAssets: true, so that doesn't work as well.
|
@sapphi-red Thanks for your review! I have looked into the issue, and now build should be working correctly. The test should now be covering the build case properly as well, and I've add some tests to verify the build result. Also I have done some manual testing locally and confirmed that it works the way I'd expect. |
|
Hi @sapphi-red, it has been a week since the last update of this PR, would you mind taking another look at it? Is there anything I can do to help moving it forward? |
|
/ecosystem-ci run |
commit: |
|
📝 Ran ecosystem CI on
✅ histoire, ladle, vite-environment-examples, vite-plugin-react, quasar, vite-plugin-svelte, vite-plugin-pwa, vitest, vitepress, vite-setup-catalogue, rakkas, vite-plugin-vue, nuxt, laravel |
This PR intends to add WASM support for Vitest and SSR. It should fix #8882 as well as vitest-dev/vitest#6723.
Implementation
The main changes are in the wasm plugin, that provide different helper code based on whether the consumer is server or client. As I'm new to this project, it's not clear to me whether this is the right approach.
I also looked into
vite-plugin-wasmfor how it handles SSR and vitest. What it does is:This approach doesn't feel great to me.
Testing
There are two tests I added:
ssr-wasmwhich serves SSR with WASM loaded.wasme2e test that checks that WASM can be imported and run within tests.It's not clear to me whether those tests are in the right location. Please let me know if there are better places for them.
Regarding Vitest
Checking
consumeractually doesn't seem to be a reliable way to work in Vitest. Vitest'senvironmentcan specifyviteEnvironment. Within its builtin environments,jsdomandhappy-domspecifiesclientas their vite environment which, on the Vite side, usesconsumer: 'client', even though the tests are still running in Node.js.I'm going to leave it there given that Vitest's default environment is
node, so this mechanism should just work most of the time, and also because I failed to find a way to add test for this behavior.I tried adding
// @vitest-environment jsdomto a spec file, but esbuild complains aboutwhich is probably because jsdom doesn't have the proper implementation for it currently. So I'm not going to dig deeper on this rabbit hole for now.
One trick I found is that some plugins like
vite-plugin-solidimplicitly changes the test environment, which could potentially cause confusion.If someone wants to address this part in the future, one possible approach is from
vite-plugin-wasmthat goes through plugins to findvitest, then combine it with the consumer check to decide which way the file is passed and obtained. It's hacky but I can confirm it works locally.