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

@vitest/web-worker does not structurally clone its postMessage message argument #2399

Closed
6 tasks done
jasondark opened this issue Nov 30, 2022 · 0 comments · Fixed by #2431
Closed
6 tasks done

@vitest/web-worker does not structurally clone its postMessage message argument #2399

jasondark opened this issue Nov 30, 2022 · 0 comments · Fixed by #2431

Comments

@jasondark
Copy link

Describe the bug

First, thanks for this package -- it's been super helpful for me.

Second, when passing values to a worker's postMessage(), the specification states that they are copied via a structural clone algorithm. (Spec). This only really matters when passing non-primitive values, e.g. { prop: value }.

At present, this wrapper does not clone the data, meaning that if the Worker mutates the data directly, it gets reflected in the main thread's data. The fix is relatively simple:
https://github.com/vitest-dev/vitest/blob/main/packages/web-worker/src/pure.ts#L35

Instead of

return (this.callbacks[event] || []).map(fn => fn(...data))

I imagine something like this would do the trick:

const clone = structuredClone(data);
return (this.callbacks[event] || []).map(fn => fn(...clone))

(there is also a whole thing about transfers of the properties, but that shouldn't be necessary in normal testing scenarios).

structuredClone is available in Node 17, but it seems Vite's compatibility starts with Node 14? I'm not an expert in making build systems play nice with polyfills (otherwise I would have just filed the PR...): https://developer.mozilla.org/en-US/docs/Web/API/structuredClone

Reproduction

My first time sharing something on stackblitz... hope it works. worker.js is just the barebones worker, and the testsuite illustrates the copying problem.

https://stackblitz.com/edit/vitest-dev-vitest-e5mxcm

System Info

This is just the output from running that command within StackBlitz (this should be irrelevant, but for completeness here it is):

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    vite: latest => 3.2.4 
    vitest: latest => 0.25.3

Used Package Manager

npm

Validations

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

Successfully merging a pull request may close this issue.

2 participants