Skip to content

fix(hmr): skip HMR for JSX files with hooks #480

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

Merged
merged 3 commits into from
May 22, 2025
Merged

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented May 18, 2025

The current HMR implementation was trying to all HMR files that contains either hooks or components, but this was working only for components and lead to HMR invalidation for JSX files containing hooks.

The best solution would have been to support HMR for hooks, but in my testing it was sometimes leading to stale updates. So this simple and reliable solution is to skip HMR for these files and have the components handle the updates, like any other hooks file.

Fixes #289
Fixes #354
Fixes #411

Copy link
Collaborator

@hi-ogawa hi-ogawa left a comment

Choose a reason for hiding this comment

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

Does this only remove warnings for non component files? It looks like the test case here and the repor in #354 is working in terms of HMR without this change.

Another interesting case is that the module exports only hook, but internally there's a non-exported components. Transform injects $RefreshReg$(_c, "Button") for this case, so refreshContentRE matches and thus warning still appears (but HMR still works).

// useButtonHook.tsx

export function useButtonHook() {
  return <Button />
}

function Button() {
  const [count, setCount] = useState(0)
  return (
    <button onClick={() => setCount((count) => count + 1)}>
      count is {count}
    </button>
  )
}

The change seems good, but I just want to confirm what this change is fixing.

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented May 19, 2025

Current HMR of hooks is "always working", but it as a strange behaviour that makes the DX not great:

  • If the file is hooks.ts, updates propagate to the component(s) that import those hooks and everything works.
  • If the file is hooks.tsx, file try to self-update, then invalidate with a warning, then component(s) updates.

This is because we only apply React refresh transform on JSX files & the current logic says that if the file contains $RefreshReg$ (has component with refresh wrapper) or $RefreshSig$ (has hooks with refresh wrapper), add self update hmr footer. The issue is that this footer expect exports to be components and doesn't handle hooks.

I updated the code to have files with only hooks do the refresh self update, but without success, it lead to some stale updates. Plus this would requires extra complexity and running the react refresh transformation on all .js/.ts files to be consistent, which would be a big change.

This change is safe and makes hooks.tsx behave like hooks.ts so that you don't get annoying warnings

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

Copy link
Member

Choose a reason for hiding this comment

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

The file name seems to be inconsistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

oups bad copy paste ahah

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented May 19, 2025

Thanks for clarifying it, but I'm not sure I got the answer 😅 My question was, in terms of user visible difference, does it only remove warnings for hooks exports only jsx/tsx files?
(If that's the case, change log doesn't seem to convey that to me, so should we adjust that?)

@ArnaudBarre
Copy link
Member Author

@hi-ogawa Yes you are right this was too much and too technical information, I changed to only tell the user facing change and people can read the comment above to better understand the change if needed

@hi-ogawa
Copy link
Collaborator

Thanks! What's left is just renaming class-components.spec.ts?

@sapphi-red sapphi-red merged commit 02b1ede into main May 22, 2025
11 checks passed
@sapphi-red sapphi-red deleted the fix-jsx-hook-hmr branch May 22, 2025 11:01
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request May 24, 2025
| datasource | package              | from  | to    |
| ---------- | -------------------- | ----- | ----- |
| npm        | @vitejs/plugin-react | 4.4.1 | 4.5.0 |


## [v4.5.0](https://github.com/vitejs/vite-plugin-react/blob/HEAD/packages/plugin-react/CHANGELOG.md#450-2025-05-23)

##### Add `filter` for rolldown-vite [#470](vitejs/vite-plugin-react#470)

Added `filter` so that it is more performant when running this plugin with rolldown-powered version of Vite.

##### Skip HMR for JSX files with hooks [#480](vitejs/vite-plugin-react#480)

This removes the HMR warning for hooks with JSX.
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request May 24, 2025
| datasource | package              | from  | to    |
| ---------- | -------------------- | ----- | ----- |
| npm        | @vitejs/plugin-react | 4.4.1 | 4.5.0 |


## [v4.5.0](https://github.com/vitejs/vite-plugin-react/blob/HEAD/packages/plugin-react/CHANGELOG.md#450-2025-05-23)

##### Add `filter` for rolldown-vite [#470](vitejs/vite-plugin-react#470)

Added `filter` so that it is more performant when running this plugin with rolldown-powered version of Vite.

##### Skip HMR for JSX files with hooks [#480](vitejs/vite-plugin-react#480)

This removes the HMR warning for hooks with JSX.
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request May 25, 2025
| datasource | package              | from  | to    |
| ---------- | -------------------- | ----- | ----- |
| npm        | @vitejs/plugin-react | 4.4.1 | 4.5.0 |


## [v4.5.0](https://github.com/vitejs/vite-plugin-react/blob/HEAD/packages/plugin-react/CHANGELOG.md#450-2025-05-23)

##### Add `filter` for rolldown-vite [#470](vitejs/vite-plugin-react#470)

Added `filter` so that it is more performant when running this plugin with rolldown-powered version of Vite.

##### Skip HMR for JSX files with hooks [#480](vitejs/vite-plugin-react#480)

This removes the HMR warning for hooks with JSX.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants