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

Client hmr injection heuristics break workers that are not using react #6148

Closed
7 tasks done
Rheeseyb opened this issue Dec 16, 2021 · 1 comment · Fixed by #6320
Closed
7 tasks done

Client hmr injection heuristics break workers that are not using react #6148

Rheeseyb opened this issue Dec 16, 2021 · 1 comment · Fixed by #6320

Comments

@Rheeseyb
Copy link

Describe the bug

If a worker uses a source file that both includes the word "react" somewhere (e.g. in a comment, or somewhere in an import statement), and a PascalCase function, the result is that the client code will be injected into the worker. This breaks the worker because that client code contains a reference to HTMLElement, leading to a very hard to debug Uncaught ReferenceError: HTMLElement is not defined error.

This bug appears to be similar in nature to the previously closed #1841

Reproduction

To reproduce it you can use the following code in a file imported as a worker:

// react
export function Thing() { return 1 }

There is a full repo example available at https://github.com/Rheeseyb/vite-worker-bug-repro, where the bug can be enabled / disabled by commenting this line

System Info

System:
    OS: Linux 5.15 NixOS 21.11 (Porcupine) 21.11 (Porcupine)
    CPU: (8) x64 Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz
    Memory: 10.35 GB / 31.28 GB
    Container: Yes
    Shell: 5.1.8 - /run/current-system/sw/bin/bash
  Binaries:
    Node: 16.13.0 - /nix/store/nxkymgnvww1caiq1g73cn2inn1hraa5w-nodejs-16.13.0/bin/node
    Yarn: 1.22.17 - /nix/store/i310mdb86zvhkrrvv5bqfirwcgk7hxkm-scripts/bin/yarn
    npm: 8.1.0 - /nix/store/nxkymgnvww1caiq1g73cn2inn1hraa5w-nodejs-16.13.0/bin/npm
  Browsers:
    Chromium: 96.0.4664.93
    Firefox: 95.0
  npmPackages:
    @vitejs/plugin-react: ^1.0.7 => 1.1.3 
    vite: ^2.7.2 => 2.7.3

Used Package Manager

pnpm

Logs

No response

Validations

@toSayNothing
Copy link
Contributor

toSayNothing commented Dec 30, 2021

https://github.com/vitejs/vite/blob/main/packages/plugin-react/src/index.ts#L121
https://github.com/vitejs/vite/blob/main/packages/plugin-react/src/index.ts#L242
in plugin/react package, it check 1)there is 'react' keyword and 2) export function name's FirstLetter is Capital ;
so the below code will be inject client code(call by addRefreshWrapper)

// react

export function Thing() { return 1 }

maybe can fix that by skipping the worker file when transforming

@Niputi Niputi linked a pull request Dec 30, 2021 that will close this issue
9 tasks
aleclarson added a commit to toSayNothing/vite that referenced this issue Jan 4, 2022
patak-dev pushed a commit that referenced this issue Jan 4, 2022
* fix: reactPlugin skip inject react hmr code to workerFile (fix #6148)

* Revert "fix: reactPlugin skip inject react hmr code to workerFile (fix #6148)"

This reverts commit 136883d.

* fix(plugin-react): check for import React statement in .js files

…instead of using naïve `code.includes("react")` check

Co-authored-by: Alec Larson <1925840+aleclarson@users.noreply.github.com>
aleclarson added a commit to aleclarson/vite that referenced this issue Jan 6, 2022
…ejs#6320)

* fix: reactPlugin skip inject react hmr code to workerFile (fix vitejs#6148)

* Revert "fix: reactPlugin skip inject react hmr code to workerFile (fix vitejs#6148)"

This reverts commit 136883d.

* fix(plugin-react): check for import React statement in .js files

…instead of using naïve `code.includes("react")` check

Co-authored-by: Alec Larson <1925840+aleclarson@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2022
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.

4 participants