facebook
Fix:- fixed HMR issue
Summary
This fixes #30659 , the issue was how the state was preserved and needed special cases for the forward and memo, have also added tests related to the same.
How did you test this change?
yarn test packages/react-refresh/src/__tests__/ReactFresh-test.js
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| react-compiler-playground | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jan 7, 2025 0:43am |
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
Comparing: ae9017ceabb2a36a04c249ad5342e0b1af3e1a54...e6932468e2f2a278cf54a0761e724e40e4acabd3
Critical size changes
Includes critical production bundles, as well as any change greater than 2%:
Significant size changes
Includes any change greater than 0.2%:
Expand to show
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| oss-experimental/react-refresh/cjs/react-refresh-runtime.development.js | +11.69% | 12.36 kB | 13.80 kB | +6.77% | 2.98 kB | 3.18 kB |
| oss-stable-semver/react-refresh/cjs/react-refresh-runtime.development.js | +11.69% | 12.36 kB | 13.80 kB | +6.77% | 2.98 kB | 3.18 kB |
| oss-stable/react-refresh/cjs/react-refresh-runtime.development.js | +11.69% | 12.36 kB | 13.80 kB | +6.77% | 2.98 kB | 3.18 kB |
| facebook-www/ReactFreshRuntime-dev.classic.js | +11.68% | 12.37 kB | 13.82 kB | +6.68% | 2.99 kB | 3.19 kB |
| facebook-www/ReactFreshRuntime-dev.modern.js | +11.68% | 12.37 kB | 13.82 kB | +6.68% | 2.99 kB | 3.19 kB |
Generated by :no_entry_sign: dangerJS against ec4517f9fedfc2942d1f99a150fd8b94beeb0d9e
@gaearon did you encounter this error? if yes could you help me out if this fix looks right.
cc @josephsavona Might be something to fix. would love to know your thoughts
Hey @Biki-das, apologies for a long wait. I think we should find an owner for react-refresh, I am not sure when was the last time react-refresh was actually updated.
hi @hoxyq 😅 no worries, i understand the team have other priorities and i guess dan was requested for review while ago, but since he is no more actively a part of the react team, so might not be something that gets attention, yeah the last change was long back, but this seems to be an important bug that shall be fixed as this occurs in Nextjs as well. yeah we should find someone who knows about the react refresh code.
@hoxyq any update on this,
may be @poteto
Hey, @Biki-das, no updates from me, I am currently on leave and will be back in December.
@hoxyq whenever you return and if you are a bit less busy, could you forward this further to someone who might have idea about react refresh, feel like this fix could be important.
@eps1lon any thoughts on who can review this, i see react refresh has not been touched for a long time? could we get someone to review this if you know someone.
@hoxyq thanks for the feedback sure i will try to add the required tests and change the PR title as well.
@hoxyq let me know if the new tests added are what you expected, and the reset test is running fine.
Overall looks good, and changes make sense, few points:
- Could you please retitle this PR to something that describes what's being added to Fresh?
- Could you please add tests that will check that state is not preserved when switching between different wrappers?
also nothing new is added to Fresh, these are the changes
Added special handling for React.forwardRef and React.memo components Enhanced state preservation logic between updates Added checks for component type signatures and React component types Improved family registration for HOCs
Why is this necessary? What set of tooling does that reproduce with?
These are supposed to be handled by the module bundler. If we switch between a function and memo, I would expect “is exporting only functions” to switch from true to false. From what I remember, in that case the modules that depend on this code are expected to start using the new versions directly (without trying to proxy the implementations). So it should “just work”.
I’m not convinced this is a correct or necessary fix. I would need to read more closely through the code to say for sure. But I would encourage to check the React Native (Metro) or the webpack plugin behavior instead of testing this in isolation. If they handle it with the existing code (which I assume they do — I tested these cases when working on FR), I don’t think this change is needed.
If CodeSandbox or Next.js aren’t handling this correctly, I would first assume that this is a problem with their integrations of FR. We need to verify whether this works in the original Metro one.
Sorry, i should’ve explicitly marked this as “needs revision” instead of stalling on the review. I sent a revert in https://github.com/facebook/react/pull/32214. We don’t have to merge it right now but I think this needs a more careful analysis.
@gaearon i happened to discover this even using vitebundler with React, so i assumed react refresh to be the issue and just played with the same, tried to see why it is breaking, apologize if this wasn't the correct way since i had limited understanding.
We need to verify whether this works in the original Metro one.
Switching between forwardRef and memo wrappers works for me with Metro runtime. Thanks for the explanation.
I am not sure I am following how is the API surface of integration with FR looks like in this case:
I would expect “is exporting only functions” to switch from true to false. From what I remember, in that case the modules that depend on this code are expected to start using the new versions directly (without trying to proxy the implementations)
I can look up a bit later how Metro does that and check if there is anything special.
thanks for the detailed explanation i tried using CRA as it uses webpack and it seems to have the same issue reproduced. and as i understood what dan meant is module bundlers are expected to handle the case which i introduced, rather than the runtime itself.
the component is not a function error log is the same i saw with vite as well.
https://github.com/user-attachments/assets/6bb4a7df-0157-4464-b135-3f52ea196e66
another interesting observation tried the same for react native as of now using expo and i see the same behaviour, @hoxyq did you did the way i did? i seemed to reproduce and see the same error.
the only way i managed to see the different wrapper being rendered below is hitting reload as in the browser refreshing it and here reloading the bundler, else it does not seem to update even after hitting save.
https://github.com/user-attachments/assets/8f64459a-b6eb-42ea-be40-8cbceb2630ec
the web version from the expo client even goes through the same issue
https://github.com/user-attachments/assets/f05d28d9-39de-4eb8-8656-a9f8a55d8fb3
cc @gaearon
@hoxyq did you did the way i did?
Actually, no. With the same code that is in your demo, I can also reproduce this issue. Both with default and named exports.
Previously I would just wrap Component in React.memo or React.forwardRef and export it, but looks like what you actually need is to preserve a name of what you are exporting, and switch it from plain function to wrappers with memo or forwardRef, like what you are doing in your demo with Component.
Yeah that's the exact issue, while editing code, the changes should reflect without any issue!
Have verified the changes that i made in the refresh file whether they actually work or not, as test in isolation might not tell the true story, it does work, for CRA i tried configuring it with crago and pulled up my react-refresh-runtime.development.js file which has the fixes to be used. and there were no errors as we currently face.
i have wrapped up a repo to try and test the same, feel free to checkout and let me know if i did something wrong and this is not the way to test the same.
https://github.com/Biki-das/React-refresh-fix-check
https://github.com/user-attachments/assets/8dcca8d2-946d-40f5-baf1-44e2b93ce854
As Dan said, these might not be the way to fix this, so we need to investigate this further. i am still confused on the part where these should be handled by the bundlers and not the refresh runtime.
cc @hoxyq @gaearon