[Float][Fiber] properly cleanup instances when switching between instance and Resource modes #29628
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
React Core Team
| // This must come at the very end of the complete phase. | ||
| bubbleProperties(workInProgress); | ||
| return null; |
There was a problem hiding this comment.
Previously bubbleProperties was called before preloadInstance/preloadResource but these functions mutate flags and so I suspenct that was incorrect all along
|
Comparing: 4ec6a6f...cc13e23 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
|
gnoff
force-pushed
the
fix-hoistable-statenode
branch
2 times, most recently
from
7966459 to
da1d573
Compare
| <link rel="stylesheet" href="b" data-precedence="default" /> | ||
| <link rel="stylesheet" href="c" data-precedence="default" /> |
There was a problem hiding this comment.
If we'd render <link rel="stylesheet" href="b" precedence="default" /> again, we'd see
<link rel="stylesheet" href="b" data-precedence="default" />
<link rel="stylesheet" href="c" data-precedence="default" />
<link rel="stylesheet" href="b" data-precedence="default" />here?
There was a problem hiding this comment.
No, we should never load the same stylesheet more than once
| @@ -5230,6 +5230,65 @@ body { | |||
| ); | |||
| }); | |||
|
|
|||
| it('can update link tags from Resource to instance and back', async () => { | |||
There was a problem hiding this comment.
Where is the and back part in this test? I thought the first render renders a link tag to Resource, the second render to instance but I'm not seeing a third render.
There was a problem hiding this comment.
I think this is meant to be read as "can update link tags from Resource to instance and vice versa"?
There was a problem hiding this comment.
Yeah, that was my original poorly worded intent but I think it's probably worth another round trip give how finicky the codepaths here are
There was a problem hiding this comment.
Thanks for pointing this out btw, found a bug when going "back again" :)
…ance and Resource modes HostHoistable is unfortunately complicated because any give fiber may be an Instance or a Resource and the lifetime of the fiber does not align with the lifetime of the underyling mode. Conceptually it'd be better if Resources were keyed intrinsically rather than using the key in JSX so we could have reconciliation deal with disposal of unmounted fibers. Maybe this is worth pursuing. However the current implementation models the inner identity during the render and commit phase so it needs to do some bookkeeping of things like the stateNode where other fiber types don't need to @sokra pointed out some suspciously broken looking code in commit work where we assign to the stateNode. It looks like an oversight in the original implementation but on further investigation there are ohter ways in which we don't properly clean up on identity transition during update. This change adds extra logic to do proper clean. If you go from instance to Resource or vice versa we need to better model the unmount semantics and ensure there is no lingering stateNode
gnoff
force-pushed
the
fix-hoistable-statenode
branch
from
da1d573 to
cc13e23
Compare
|
Closing in favor of #29693 |
HostHoistable is unfortunately complicated because any give fiber may be an Instance or a Resource and the lifetime of the fiber does not align with the lifetime of the underyling mode. Conceptually it'd be better if Resources were keyed intrinsically rather than using the key in JSX so we could have reconciliation deal with disposal of unmounted fibers. Maybe this is worth pursuing. However the current implementation models the inner identity during the render and commit phase so it needs to do some bookkeeping of things like the stateNode where other fiber types don't need to
@sokra pointed out some suspiciously broken looking code in commit work where we assign to the stateNode. It looks like an oversight in the original implementation but on further investigation there are other ways in which we don't properly clean up on identity transition during update.
This change adds extra logic to do proper clean. If you go from instance to Resource or vice versa we need to better model the unmount semantics and ensure there is no lingering stateNode