Fix:- Improve HOC support and state preservation in React Refresh #30660
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Comparing: ae9017c...e693246 Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
@gaearon did you encounter this error? if yes could you help me out if this fix looks right. |
|
cc @josephsavona |
|
Hey @Biki-das, apologies for a long wait. I think we should find an owner for |
|
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. |
|
@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. |
There was a problem hiding this comment.
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?
|
@hoxyq thanks for the feedback sure i will try to add the required tests and change the PR title as well. |
Biki-das
force-pushed
the
react-refresh-fix
branch
from
65935b0 to
94387b6
Compare
|
@hoxyq let me know if the new tests added are what you expected, and the reset test is running fine. |
Biki-das
changed the title
also nothing new is added to Fresh, these are the changes Added special handling for React.forwardRef and React.memo components |
Biki-das
force-pushed
the
react-refresh-fix
branch
from
8acf0a8 to
ec4517f
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Thanks! |
…0660) ## 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`  DiffTrain build for [de1eaa2](de1eaa2)
…cebook#30660) ## Summary This fixes facebook#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`  DiffTrain build for [de1eaa2](facebook@de1eaa2)
|
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 #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. |
Switching between I am not sure I am following how is the API surface of integration with FR looks like in this case:
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.
Screen.Recording.2025-01-24.at.11.43.11.PM.mov |
|
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. Screen.Recording.2025-01-25.at.12.05.46.AM.movthe web version from the expo client even goes through the same issue Screen.Recording.2025-01-25.at.12.18.53.AM.movcc @gaearon |
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 |
|
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 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 Screen.Recording.2025-01-26.at.7.05.49.PM.movAs 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. |
…resh" (#32214) Reverts #30660 I don’t feel confident in the approach. This part of code is supposed to rely on the module bundler behaving as expected. _Maybe_ this is correct but I need to review it closer — it was intentionally _not_ implemented this way originally. I’ll try to take a closer look some time this week. We don’t have to merge this revert right now but just flagging that I don’t understand the thinking behind the new approach and don’t have confidence in it.
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