Skip to content
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

web-worker search path regression with ES6 wasm module #5704

Open
6 tasks done
elalish opened this issue May 11, 2024 · 5 comments
Open
6 tasks done

web-worker search path regression with ES6 wasm module #5704

elalish opened this issue May 11, 2024 · 5 comments

Comments

@elalish
Copy link

elalish commented May 11, 2024

Describe the bug

I have a WASM library I built as an ES module with Emscripten, so I have manifold.js and manifold.wasm in my built directory. In a worker I call it like import Module from './built/manifold'; the offending line

This works with npm test in vitest 0.33, but not in 0.34. The error I get in 0.34 is:

stderr | worker.test.js > Examples > Tetrahedron Puzzle
failed to asynchronously prepare wasm: Error: ENOENT: no such file or directory, open '/built/manifold.wasm'
Aborted(Error: ENOENT: no such file or directory, open '/built/manifold.wasm')
RuntimeError: Aborted(Error: ENOENT: no such file or directory, open '/built/manifold.wasm'). Build with -sASSERTIONS for more info.
    at abort (/Users/elalish/Code/manifold/bindings/wasm/examples/built/manifold.js:8:19924)
    at /Users/elalish/Code/manifold/bindings/wasm/examples/built/manifold.js:8:21399

And I believe I'm getting an equivalent error in 1.6.0:

stderr | worker.test.js > Examples > Tetrahedron Puzzle
TypeError: Invalid URL
    at new URL (node:internal/url:787:36)
    at Module.default (/Users/elalish/Code/manifold/bindings/wasm/examples/built/manifold.js:8:20330)
    at /Users/elalish/Code/manifold/bindings/wasm/examples/worker.ts:17:16
    at InlineWorkerRunner.runModule (file:///Users/elalish/Code/manifold/bindings/wasm/examples/node_modules/vite-node/dist/client.mjs:362:5)
    at InlineWorkerRunner.directRequest (file:///Users/elalish/Code/manifold/bindings/wasm/examples/node_modules/vite-node/dist/client.mjs:346:5)
    at InlineWorkerRunner.cachedRequest (file:///Users/elalish/Code/manifold/bindings/wasm/examples/node_modules/vite-node/dist/client.mjs:189:14)
    at InlineWorkerRunner.executeFile (file:///Users/elalish/Code/manifold/bindings/wasm/examples/node_modules/vite-node/dist/client.mjs:161:12) {
  code: 'ERR_INVALID_URL',
  input: '/built/manifold.wasm'
}

You mention in common errors that the base path can be a problem, which this certainly seems related to, but I haven't set anything special. Even though manifold.js is calling manifold.wasm in its own directory, it can't seem to resolve the URL anymore. Is there something special I need to do to help it understand these files are both in built/?

Reproduction

I made a PR where you can see our CI (which runs vitest in the wasm action) passes and then fails based on updating vitest.

System Info

System:
    OS: macOS 14.4.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 139.25 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.6.0 - /opt/homebrew/bin/node
    npm: 10.2.4 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 124.0.6367.201
    Safari: 17.4.1
  npmPackages:
    @vitest/ui: ^1.6.0 => 1.6.0 
    @vitest/web-worker: ^1.6.0 => 1.6.0 
    vite: ^5.2.11 => 5.2.11 
    vitest: ^1.6.0 => 1.6.0

Used Package Manager

npm

Validations

@elalish elalish changed the title web-worker search path regression ES6 wasm module web-worker search path regression with ES6 wasm module May 11, 2024
@hi-ogawa
Copy link
Contributor

I made a simpler emscripten EXPORT_ES6 setup to investigate the issue https://github.com/hi-ogawa/reproductions/tree/main/vitest-5704-emscripten-esm-worker

It looks like what's happening is that new URL(..., import.meta.url) is heavily used by EXPORT_ES6 format, but Vitest started to replace it with new URL(..., self.location) #4258 for web transform (including web worker simulation provided by @vitest/web-worker) and it's actually undefined here, so the url becomes invalid.

A quick trick I found is to use a following plugin to prevent Vitest's transform from kicking in:
https://github.com/hi-ogawa/reproductions/blob/a72885fedbf70ddb6d48294d99d26d24e79105b9/vitest-5704-emscripten-esm-worker/vitest.config.ts#L6C1-L21C5

Can you try this on your repo to see if it works?

{
  name: "keep-import-meta-url",
  enforce: "pre",
  transform(code, id, _options) {
    // prevent NormalizeURLPlugin from replacing import.meta.url with self.location
    // https://github.com/vitest-dev/vitest/blob/d8304bb4fbe16285d014f63aa71ef9969865c691/packages/vitest/src/node/plugins/normalizeURL.ts#L11
    // since it breaks `new URL(..., import.meta.url)` used by emscripten EXPORT_ES6 output
    // https://github.com/emscripten-core/emscripten/blob/228af1a7de1672b582e1448d4573c20c5d2a5b5a/src/shell.js#L242
    if (id.endsWith("/dist/lib.js")) {
      return code.replace(/\bimport\.meta\.url\b/g, `String(import.meta.url)`);
    }
  },
}

Technically @vitest/web-worker is probably intended to be used on a web environment (such as jsdom/happy-dom), so not sure if it should be called a bug/fixable or it's imply out of the scope to support this use case.

Personally, just testing out emscripten's EXPORT_ES6 for the first time, it's mostly working nice with Vite / Vitest in general, so it would be cool if Vitest can support it without friction.

@elalish
Copy link
Author

elalish commented May 22, 2024

Thanks for the investigation - I just pushed your vitest.config.js to my above PR, but it didn't make a difference (I'm assuming this config is picked up automatically by name?).

As to use cases, indeed we filed a related bug on Emscripten a couple years back. They had also been thinking of WASM libraries as being built either for web or for Node, but not both. We convinced them that the "both" use case is really important, testing being a key reason. If we build a computational library that we intend to work on the web, it's still really convenient to test it headless, particularly on a Github CI, so Node is ideal for that.

Let me know if there's anything else I can try to help narrow this down.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented May 23, 2024

If we build a computational library that we intend to work on the web,

I haven't fully understand your use case, but personally, your testing strategy might be a little too convoluted for what it achives. I'm assuming that it's a js/wasm binding of pure computation library and it's not tied to platform api (e.g. filesystem, pthread, web window api, etc..). If that's case, wouldn't it be simpler to test your Module directly without going through web worker?

I understand your actual consumption on web sample would obviously require off-loading computation on Web worker, but I just want to make sure what exactly your objective of integrating Vitest (or any testing) there.

If you want to exercise actual web worker communication, then what @vitest/web-worker provides is a simulation on NodeJS and that looks a bit tricky to make it work and doesn't seem worth the effort to get around as it's still a simulation. If you go with this path, then it might be better to use Vitest browser mode https://vitest.dev/guide/browser.html (though it's also possible that some Vite / Vitest transform would mess up emscripten output) or going with full browser e2e testing like playwright.

On the other hand, If the purpose is to exercise only your wasm binding (+ model io library integration e.g. gltf?), then simpler (and more direct) testing approach could be to somehow extract that logic out of your bindings/wasm/examples/worker.ts and test directly as pure js/wasm library without WebWorker.

I get that if Vitest would somehow fix the regression, then you don't have to change any code and that's obviously better, but I just wanted to share my perspective in terms of overall testing strategy.
(Also because of this perspective, it's unlikely that I'll look into further on your reproduction to investigate this specific use case unless you can minimize reproduction to point out the issue.)

@elalish
Copy link
Author

elalish commented May 23, 2024

Fair point, my earlier argument didn't address why I'm testing a worker. In my case it's because I'm testing ManifoldCAD.org, which uses a web worker to keep all the calculation off the main thread. It also forms a more compact API: you hand it a script of the geometry operations you want to do and it packs the result up as a GLB 3D model and hands it back. This makes for a very convenient and compact surface for end-to-end tests, testing every example on ManifoldCAD.org, but by extension, testing a huge variety of operations on the underlying Manifold library as well.

That said, at the time I built this I didn't realize the difficulties workers posed to testing on Node (it happened to just work on the version of vitest I started with, so it never occurred to me to do anything else). Is there some kind of pattern for making a worker just a lightweight wrapper for its business logic code, so that the testing can hook to the business logic only and skip the worker? Perhaps that would be a better approach.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented May 23, 2024

Thanks for providing the context. As I'm very much interested in your project, I'm happy to help if I can, but I don't have a time to get into your code base and actually understand something at that moment. (Well, I'm starting to read a few bits, but I would imagine your project is pretty non trivial for me to suggest something quickly).

Please feel free to leave this issue open for now. I might be able to get back to this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants