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

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

Merged
merged 4 commits into from
Jan 4, 2022

Conversation

toSayNothing
Copy link
Contributor

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

in vite-react project, when import a worker which contains 'react' word and export function name's FirstLetter is capital, the react/plugin will inject the hmr code to the worker.
reproduction : issue #6148 's repo.

// worker.js
// react
export function Thing() { return 1 }

in packages\plugin-react\src\index.ts file

  1. check the 'react' keyword
  2. inside the isRefreshBoundary fn will check the worker's export function name whether isComponentLikeName

before
image
after add the isWorker check
image

patak-dev
patak-dev previously approved these changes Dec 30, 2021
Copy link
Member

@aleclarson aleclarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we improved the code.includes("react") check so it doesn't have false positives

@toSayNothing
Copy link
Contributor Author

toSayNothing commented Dec 31, 2021

i compare the webpack version(create-react-app) and vite:
in webpack , the react-refresh-webpack-plugin will generate each JS-LIKE module
https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/loader/utils/getRefreshModuleRuntime.js
but vite will transform if it's isJSX or code includes 'react'

const isReactModule = isJSX || code.includes('react')

the util.js transformed result by webpack/vite (simplified version):

// util.js with 'react' keyword
export function Test() {
  console.log("Capital fn");
}
export function normal() {
  console.log("normal"); // react words here
}
// transform by webpack
function Test() {
  console.log("Capital fn");
}
_c = Test;
function normal() {
  console.log("normal"); // react words here
}
var _c;
// 1. regist
__webpack_require__.$Refresh$.register(_c, "Test");
// 2. react refresh
$ReactRefreshModuleRuntime$($ReactRefreshCurrentExports$);
// transform by vite
if (import.meta.hot) {
  window.$RefreshSig$ = RefreshRuntime.createSignatureFunctionForTransform;
}
export function Test() {
  console.log("Capital fn");
}
_c = Test;
export function normal() {
  console.log("normal"); // react words here
}
var _c;
// 1. regist
$RefreshReg$(_c, "Test");
if (import.meta.hot) {
  // 2. react refresh
  RefreshRuntime.performReactRefresh();
}

but if there are no 'react' in util.js, vite wouldn't transform the file.
maybe because it is difficult to know exactly whether a JS-LIKE module is a react component, so react-refresh-webpack-plugin just transform each JS-LIKE module(and webpack can config worker-loader which won't insert refresh code into workerFile to handle worker import).
so i think maybe we could do the same , inject hmr to each JS-LIKE module, but ignore the workerFile.
like this:

let useFastRefresh = false
if (!skipFastRefresh && !ssr && !isNodeModules) {
  // Modules with .js or .ts extension must import React.
  // const isReactModule = isJSX || code.includes('react')
  // if (isReactModule && filter(id)) {
  // ---Replace isReactModule with isWorker condition here---
  if (!isWorker && filter(id)) {
    useFastRefresh = true
    plugins.push([
      await loadPlugin('react-refresh/babel.js'),
      { skipEnvCheck: true }
    ])
  }
}

@aleclarson what do you think ? 😅

@aleclarson
Copy link
Member

aleclarson commented Jan 4, 2022

Vite requires .js modules to import React explicitly. We don't want to change that

Could you update the code.includes("react") check to use importReactRE.test?

const importReactRE = /(^|\n)import\s+(\*\s+as\s+)?React(,|\s+)/

edit: Nevermind, I've got it

@aleclarson aleclarson changed the title fix: reactPlugin skip inject react hmr code to workerFile (fix #6148) fix(plugin-react): check for import React statement in .js files Jan 4, 2022
@patak-dev patak-dev merged commit bd9e97b into vitejs:main Jan 4, 2022
aleclarson added a commit to aleclarson/vite that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client hmr injection heuristics break workers that are not using react
4 participants