From cc13e23389075629ef2afd18426b18ad639fcc48 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 28 May 2024 10:21:56 -0700 Subject: [PATCH] [Float][Fiber] properly cleanup instances when switching between instance 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 --- .../src/client/ReactFiberConfigDOM.js | 18 +-- .../src/__tests__/ReactDOMFloat-test.js | 97 +++++++++++++ .../src/ReactFiberBeginWork.js | 21 +++ .../src/ReactFiberCommitWork.js | 133 ++++++++++-------- .../src/ReactFiberCompleteWork.js | 27 +--- 5 files changed, 210 insertions(+), 86 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 2fcf5bf9a57f..ebb26a6aed84 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -2573,7 +2573,7 @@ export function acquireResource( hoistableRoot: HoistableRoot, resource: Resource, props: any, -): null | Instance { +): void { resource.count++; if (resource.instance === null) { switch (resource.type) { @@ -2587,7 +2587,7 @@ export function acquireResource( if (instance) { resource.instance = instance; markNodeAsHoistable(instance); - return instance; + return; } const styleProps = styleTagPropsFromRawProps(props); @@ -2604,7 +2604,7 @@ export function acquireResource( insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); resource.instance = instance; - return instance; + return; } case 'stylesheet': { // This typing is enforce by `getResource`. If we change the logic @@ -2621,7 +2621,7 @@ export function acquireResource( resource.state.loading |= Inserted; resource.instance = instance; markNodeAsHoistable(instance); - return instance; + return; } const stylesheetProps = stylesheetPropsFromRawProps(props); @@ -2644,7 +2644,7 @@ export function acquireResource( insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); resource.instance = instance; - return instance; + return; } case 'script': { // This typing is enforce by `getResource`. If we change the logic @@ -2660,7 +2660,7 @@ export function acquireResource( if (instance) { resource.instance = instance; markNodeAsHoistable(instance); - return instance; + return; } let scriptProps = borrowedScriptProps; @@ -2678,10 +2678,10 @@ export function acquireResource( (ownerDocument.head: any).appendChild(instance); resource.instance = instance; - return instance; + return; } case 'void': { - return null; + return; } default: { throw new Error( @@ -2712,7 +2712,7 @@ export function acquireResource( insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot); } } - return resource.instance; + return; } export function releaseResource(resource: Resource): void { diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 43926bfa160f..61f3c7f619a4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -5230,6 +5230,103 @@ body { ); }); + it('can update link tags from Resource to instance and back', async () => { + const root = ReactDOMClient.createRoot(container); + + root.render( +
+ hello + + + +
, + ); + await waitForAll([]); + loadPreloads(); + loadStylesheets(); + await assertLog(['load preload: c', 'load stylesheet: c']); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + +
+
hello
+
+ + , + ); + + root.render( +
+ hello + + + +
, + ); + await waitForAll([]); + loadPreloads(); + loadStylesheets(); + await assertLog(['load preload: d', 'load stylesheet: d']); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + + + +
+
hello
+
+ + , + ); + + root.render( +
+ hello + + + +
, + ); + await waitForAll([]); + loadPreloads(); + loadStylesheets(); + await assertLog(['load preload: i', 'load stylesheet: i']); + + expect(getMeaningfulChildren(document)).toEqual( + + + + + + + + + + + + +
+
hello
+
+ + , + ); + }); + describe('ReactDOM.prefetchDNS(href)', () => { it('creates a dns-prefetch resource when called', async () => { function App({url}) { diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 6eeb7ab37797..f8e7a416ba0d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1705,6 +1705,27 @@ function updateHostHoistable( workInProgress, ); } + } else { + // we're updating + if (current.memoizedState) { + if (resource === null) { + // we must be transitioning to the instance mode and need to create + // an instance + workInProgress.stateNode = createHoistableInstance( + workInProgress.type, + workInProgress.pendingProps, + getRootHostContainer(), + workInProgress, + ); + } + } else { + if (resource) { + // we must be transitioning to the instance mode and need to void + // the previous stateNode. It will be unmounted during the commit + // we just null it on the workInProgress so it doesn't carry over + workInProgress.stateNode = null; + } + } } // Resources never have reconciler managed children. It is possible for diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index a8d37b5e4493..216a28d05033 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -1506,12 +1506,20 @@ function hideOrUnhideAllChildren(finishedWork: Fiber, isHidden: boolean) { while (true) { if ( node.tag === HostComponent || - (supportsResources ? node.tag === HostHoistable : false) || + (supportsResources + ? // HostHoistables don't have stateNodes when they represent + // Resources. We only want to unhide instances, not Resources + // because only instances are semantically bound to the tree + node.tag === HostHoistable && node.stateNode + : false) || (supportsSingletons ? node.tag === HostSingleton : false) ) { if (hostSubtreeRoot === null) { hostSubtreeRoot = node; try { + // If we did not guard HostHoistable above this instance could + // be null. We don't do the check here to avoid adding overhead + // when traversing HostComponent and HostSingleton fibers const instance = node.stateNode; if (isHidden) { hideInstance(instance); @@ -2628,69 +2636,80 @@ function commitMutationEffectsOnFiber( } if (flags & Update) { - const currentResource = - current !== null ? current.memoizedState : null; - const newResource = finishedWork.memoizedState; - if (current === null) { - // We are mounting a new HostHoistable Fiber. We fork the mount - // behavior based on whether this instance is a Hoistable Instance - // or a Hoistable Resource - if (newResource === null) { - if (finishedWork.stateNode === null) { - finishedWork.stateNode = hydrateHoistable( - hoistableRoot, - finishedWork.type, - finishedWork.memoizedProps, - finishedWork, - ); + const resource = finishedWork.memoizedState; + try { + if (current === null) { + // We are mounting a new HostHoistable Fiber. We fork the mount + // behavior based on whether this instance is a Hoistable Instance + // or a Hoistable Resource + if (resource === null) { + if (finishedWork.stateNode === null) { + finishedWork.stateNode = hydrateHoistable( + hoistableRoot, + finishedWork.type, + finishedWork.memoizedProps, + finishedWork, + ); + } else { + mountHoistable( + hoistableRoot, + finishedWork.type, + finishedWork.stateNode, + ); + } } else { - mountHoistable( + acquireResource( hoistableRoot, - finishedWork.type, - finishedWork.stateNode, + resource, + finishedWork.memoizedProps, ); } } else { - finishedWork.stateNode = acquireResource( - hoistableRoot, - newResource, - finishedWork.memoizedProps, - ); - } - } else if (currentResource !== newResource) { - // We are moving to or from Hoistable Resource, or between different Hoistable Resources - if (currentResource === null) { - if (current.stateNode !== null) { - unmountHoistable(current.stateNode); + // We are updating + if (resource) { + // We are a Resource + if (current.stateNode !== null) { + // We used to be an Instance and need to clean up + unmountHoistable(current.stateNode); + } else if (current.memoizedState) { + // We used to be a different resource and need to release + releaseResource(current.memoizedState); + } + + acquireResource( + hoistableRoot, + resource, + finishedWork.memoizedProps, + ); + } else { + // We are an Instance + if (current.memoizedState) { + // we used to be a Resource and need to release + releaseResource(current.memoizedState); + } + + if (current.memoizedState) { + // While this is an update the finishedWork stateNode has not + // yet ever been inserted so we mount it now + mountHoistable( + hoistableRoot, + finishedWork.type, + finishedWork.stateNode, + ); + } else { + // update the instance + commitUpdate( + finishedWork.stateNode, + finishedWork.type, + current.memoizedProps, + finishedWork.memoizedProps, + finishedWork, + ); + } } - } else { - releaseResource(currentResource); - } - if (newResource === null) { - mountHoistable( - hoistableRoot, - finishedWork.type, - finishedWork.stateNode, - ); - } else { - acquireResource( - hoistableRoot, - newResource, - finishedWork.memoizedProps, - ); - } - } else if (newResource === null && finishedWork.stateNode !== null) { - try { - commitUpdate( - finishedWork.stateNode, - finishedWork.type, - current.memoizedProps, - finishedWork.memoizedProps, - finishedWork, - ); - } catch (error) { - captureCommitPhaseError(finishedWork, finishedWork.return, error); } + } catch (error) { + captureCommitPhaseError(finishedWork, finishedWork.return, error); } } return; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index a060d0f00aed..c77c7df77046 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -1039,9 +1039,6 @@ function completeWork( markUpdate(workInProgress); if (nextResource !== null) { // This is a Hoistable Resource - - // This must come at the very end of the complete phase. - bubbleProperties(workInProgress); preloadResourceAndSuspendIfNeeded( workInProgress, nextResource, @@ -1049,36 +1046,26 @@ function completeWork( newProps, renderLanes, ); - return null; } else { // This is a Hoistable Instance - - // This must come at the very end of the complete phase. - bubbleProperties(workInProgress); preloadInstanceAndSuspendIfNeeded( workInProgress, type, newProps, renderLanes, ); - return null; } } else { // We are updating. - const currentResource = current.memoizedState; - if (nextResource !== currentResource) { - // We are transitioning to, from, or between Hoistable Resources - // and require an update - markUpdate(workInProgress); - } if (nextResource !== null) { // This is a Hoistable Resource - // This must come at the very end of the complete phase. + const currentResource = current.memoizedState; - bubbleProperties(workInProgress); + // This must come at the very end of the complete phase. if (nextResource === currentResource) { workInProgress.flags &= ~MaySuspendCommit; } else { + markUpdate(workInProgress); preloadResourceAndSuspendIfNeeded( workInProgress, nextResource, @@ -1087,9 +1074,9 @@ function completeWork( renderLanes, ); } - return null; } else { // This is a Hoistable Instance + // We may have props to update on the Hoistable instance. if (supportsMutation) { const oldProps = current.memoizedProps; @@ -1108,17 +1095,17 @@ function completeWork( ); } - // This must come at the very end of the complete phase. - bubbleProperties(workInProgress); preloadInstanceAndSuspendIfNeeded( workInProgress, type, newProps, renderLanes, ); - return null; } } + // This must come at the very end of the complete phase. + bubbleProperties(workInProgress); + return null; } // Fall through }