vitejs / vite

Next generation frontend tooling. It's fast!
http://vite.dev
MIT License
68.23k stars 6.15k forks source link

Incomplete web worker #17950

Closed axelra82 closed 2 months ago

axelra82 commented 2 months ago

Describe the bug

When running PNext three loader in a Vite bundled app (see Vite app example in reproduction repo) there are issues with the web worker. Specifically the adaptive point type is not updated in the render loop, thus creating a fixed point type since the shader is not getting proper data.

As far as we (me and @fredrik-hjelmaeus) can tell, the issue stems from how Vite deals with web workers when doing its "Vite magic".

Possible Culprit

Looking closer at the final builds (i.e. this issue extends from dev to prod as well) we have come to the conclusion that their must be something with how Vite is handling the import of the web worker found in apps/vite-app/src/pnext-loader/loading2/worker-pool.ts.

In createWorker we've tried 2 ways of importing the web worker (both involve changing the require reference for proper Vite import handling):

  1. import DecoderWorker from './decoder.worker.js?worker&inline'; (NOTE Removing inline does not solve the issue. Using it helps us identify the difference in results between the Vite and Rollup bundlers)
  2. return new Worker(new URL('./decoder.worker.js', import.meta.url),{ type: 'module' });

We've tried to identify when/where Vite deals with the imported worker. Currently we've been looking at webWorkerPlugin in the build source code of Vite. Our working theory is that there has to be something in the "wrapping abstractions" in webWorkerPlugin that is causing the "final state" to differ in the Vite app from the Rollup/Webpack apps.

Reproduction

https://github.com/axelra82/vite-pnext-web-worker-issue/tree/master

Steps to reproduce

Steps

Incorrect

Working

To see issue, click Load on each page. What you will see is the pointcloud loading with fixed point type, when it should be adaptive. The way you can distinguish the two is:

This is simplified but should provide an adequate explanation of what to look for. By running the provided apps and comparing them, side-by-side, the difference will become clear (if their are any doubts).

System Info

System:
    OS: macOS 14.6.1
    CPU: (12) arm64 Apple M2 Max
    Memory: 51.55 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.19.0 - /opt/homebrew/opt/nvm/versions/node/v18.19.0/bin/node
    Yarn: 1.22.22 - /opt/homebrew/bin/yarn
    npm: 10.2.3 - /opt/homebrew/opt/nvm/versions/node/v18.19.0/bin/npm
    pnpm: 9.1.4 - ~/Library/pnpm/pnpm
  Browsers:
    Brave Browser: 120.1.61.109
    Chrome: 127.0.6533.120
    Safari: 17.6

Used Package Manager

pnpm

Logs

No response

Validations

sapphi-red commented 2 months ago

It worked if I change useDefineForClassFields to false to align the tsconfig with rollup's one. https://github.com/axelra82/vite-pnext-web-worker-issue/blob/0fa3cb4a7285ccf41a6d1b5e1ff84a407e9ac9ab/apps/vite-app/tsconfig.json#L32 (The default value is false for some options.)

Closing as this is not a bug in Vite.

axelra82 commented 2 months ago

For posterity sake.

I was somewhat confused about the useDefineForClassFields key in TS config (since I didn't recognize it. @fredrik-hjelmaeus was the one who added it for testing).

The example Vite app is only a small reproduction setup. Our production repo has a TSConfig monorepo package with multiple TS-config files depending on app.

We were able to identify that this useDefineForClassFields is default true when TS-config compilerOptions.target is >= es2022 (reference).

Since we had the following in our base.json TS-config:

{
  ...
  compilerOptions: {
    ...
    target: "esnext",
  },
}

Changing target to "es2020" appears to solve the issue. Again, our use case is a bit more nested than the reproduction repo and we are currently evaluating if this completely solves it for us, but as far as we can tell this is NOT a Vite issue but solely a TS-config issue.

A big thanks to @sapphi-red for finding this somewhat obscure solve and @fredrik-hjelmaeus for adding useDefineForClassFields in the first place (otherwise we would still be scratching our heads as to why it worked in the reproduction app but not our production repo).