Revert "Fix:- Improve HOC support and state preservation in React Refresh"

[gaearon]

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.

[react-sizebot]

Comparing: b65afdd...e5d6858

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	+0.05%	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	514.24 kB	514.24 kB	=	91.74 kB	91.73 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	+0.05%	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	557.28 kB	557.28 kB	=	98.97 kB	98.97 kB
facebook-www/ReactDOM-prod.classic.js	=	595.79 kB	595.79 kB	=	104.85 kB	104.85 kB
facebook-www/ReactDOM-prod.modern.js	=	586.21 kB	586.21 kB	=	103.30 kB	103.30 kB
facebook-www/ReactFreshRuntime-dev.classic.js	=	13.82 kB	12.37 kB	=	3.19 kB	2.99 kB
facebook-www/ReactFreshRuntime-dev.modern.js	=	13.82 kB	12.37 kB	=	3.19 kB	2.99 kB
oss-experimental/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB
oss-stable-semver/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB
oss-stable/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactFreshRuntime-dev.classic.js	=	13.82 kB	12.37 kB	=	3.19 kB	2.99 kB
facebook-www/ReactFreshRuntime-dev.modern.js	=	13.82 kB	12.37 kB	=	3.19 kB	2.99 kB
oss-experimental/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB
oss-stable-semver/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB
oss-stable/react-refresh/cjs/react-refresh-runtime.development.js	=	13.80 kB	12.36 kB	=	3.18 kB	2.98 kB

Generated by 🚫 dangerJS against 76a1e72

[rickhanlonii]

sgtm

[Biki-das]

thanks @gaearon for the follow up, happy to collaborate if we are looking forward to revert the changes and understand if its necessary!

[gaearon]

I've taken a closer look at this.

I do believe the original bug is legit. I.e. I can reproduce it in RN. So this is something that does need to be fixed.

Regarding the fix, I'm still not sure and I think it merits more investigation:

• The part of Revert "Fix:- Improve HOC support and state preservation in React Refresh" #32214 about changing canPreserveStateBetween does make sense to me. You can't preserve the state when it changes from a function to an object (already handled by existing code), or between different $$typeof (newly added code). So if we'd like to handle this better, we should.

• The part about nextFamily seems dubious to me and needs very concrete motivation and explanation for why we're doing this (which needs to be more concrete than that it happens to fix the bug). I get that it fixes the bug, but it undermines existing constraints.

  • Families are meant to be a "long-lived" concept. Before Revert "Fix:- Improve HOC support and state preservation in React Refresh" #32214, they used to get created in a single place (look for family = {current: type};). There used to be a guarantee that the same ID always resolves to the same family object, forever. For example, you can observe that allFamiliesByID.set is only called once on family creation and never again — but that was fine because there's never again another family with the same ID. It's the existing family that would get updated. So Revert "Fix:- Improve HOC support and state preservation in React Refresh" #32214 has already introduced an inconsistency because it doesn't respect this constraint and recreates some families without updating allFamiliesByID or allFamiliesByType. My point isn't that we need to update these fields too, but that we're making the concept more complicated than it was, so maybe something else needs rethinking.
  • Another suspicious thing is that in the original code (which runs during init too), the current field of the family object is always set to the type that's being passed. There's no "guessing" about its shape. But in Revert "Fix:- Improve HOC support and state preservation in React Refresh" #32214, the current field is trying to read something specific based on the component type. Why is there a difference? It doesn't seem like it should get initialized differently since it's the same "kind of a thing".
  • This is a bit edge casey but technically memo can be nested more than once due to reexports, or have a forwardRef inside. If there's something wrong with just setting {current: nextType}, then nextType.type is also not enough for the memo case. It's not guaranteed to be terminal.
  • Overall, it is strange to me that we need to "matching the shapes" so much for the case where the tree is supposed to get remounted. It's not really even exactly supposed to "hot reload" in these cases, we just want to make sure the instruction to remount the tree "makes it" to the part that scans for it. I suspect that this fix "patches things up" enough that the reconciler finds the right thing to remount, but what we should really do is figure out why it doesn't find it right now (which is what I understand to be the cause of the error — it didn't remount, so it tries to hot reload, but the shape doesn't match).

My main concern with merging this is that it does the minimal job to fix that particular bug, but it undermines the assumptions of how this thing was meant to work. I think the fix is probably in the wrong place, and if we sit down and describe the exact sequencing of what is going wrong, we'll find a more natural place to apply the fix that doesn't undermine the existing constraints. Or maybe we discover that there's some more fundamental issue with how it was designed.

But we need a concrete writedown of what's going wrong first.

[Biki-das]

Thanks @gaearon for the detailed follow up,makes sense and appreciate your time on this,i feel we can take my changes as a point to do the right fixes.