From ea3ac586693014e882655728fc8396ecb1d6cf6e Mon Sep 17 00:00:00 2001 From: Timothy Yung Date: Thu, 31 Oct 2024 13:24:45 -0700 Subject: [PATCH] Fix Ref Lifecycles in Hidden Subtrees (#31379) ## Summary We're seeing certain situations in React Native development where ref callbacks in `` are sometimes invoked exactly once with `null` without ever being called with a "current" value. This violates the contract for refs because refs are expected to always attach before detach (and to always eventually detach after attach). This is *particularly* bad for refs that return cleanup functions, because refs that return cleanup functions expect to never be invoked with `null`. This bug causes such refs to be invoked with `null` (because since `safelyAttachRef` was never called, `safelyDetachRef` thinks the ref does not return a cleanup function and invokes it with `null`). This fix makes use of `offscreenSubtreeWasHidden` in `commitDeletionEffectsOnFiber`, similar to how https://github.com/facebook/react/commit/ec52a5698e2dfea7050a0b015f0b79abfb2d81b7 did this for `commitDeletionEffectsOnFiber`. ## How did you test this change? We were able to isolate the repro steps to isolate the React Native experimental changes. However, the repro steps depend on Fast Refresh. ``` function callbackRef(current) { // Called once with `current` as null, upon triggering Fast Refresh. } ; ``` Ideally, we would have a unit test that verifies this behavior without Fast Refresh. (We have evidence that this bug occurs without Fast Refresh in real product implementations. However, we have not successfully deduced the root cause, yet.) This PR currently includes a unit test that reproduces the Fast Refresh scenario, which is also demonstrated in this CodeSandbox: https://codesandbox.io/p/sandbox/hungry-darkness-33wxy7 Verified unit tests pass: ``` $ yarn $ yarn test # Run with `-r=www-classic` for `enableScopeAPI` tests. $ yarn test -r=www-classic ``` Verified on the internal React Native development branch that the bug no longer repros. --------- Co-authored-by: Rick Hanlon --- .../src/ReactFiberCommitWork.js | 24 ++-- .../__tests__/ReactFreshIntegration-test.js | 125 ++++++++++++++++++ 2 files changed, 140 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 89aa35d760..9384f5cbd5 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -1325,7 +1325,9 @@ function commitDeletionEffectsOnFiber( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } } recursivelyTraverseDeletionEffects( finishedRoot, @@ -1335,7 +1337,9 @@ function commitDeletionEffectsOnFiber( return; } case OffscreenComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } if (disableLegacyMode || deletedFiber.mode & ConcurrentMode) { // If this offscreen component is hidden, we already unmounted it. Before // deleting the children, track that it's already unmounted so that we @@ -1572,7 +1576,7 @@ function recursivelyTraverseMutationEffects( lanes: Lanes, ) { // Deletions effects can be scheduled on any fiber type. They need to happen - // before the children effects hae fired. + // before the children effects have fired. const deletions = parentFiber.deletions; if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { @@ -1637,7 +1641,7 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Ref) { - if (current !== null) { + if (!offscreenSubtreeWasHidden && current !== null) { safelyDetachRef(current, current.return); } } @@ -1660,7 +1664,7 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Ref) { - if (current !== null) { + if (!offscreenSubtreeWasHidden && current !== null) { safelyDetachRef(current, current.return); } } @@ -1745,7 +1749,7 @@ function commitMutationEffectsOnFiber( commitReconciliationEffects(finishedWork); if (flags & Ref) { - if (current !== null) { + if (!offscreenSubtreeWasHidden && current !== null) { safelyDetachRef(current, current.return); } } @@ -1961,7 +1965,7 @@ function commitMutationEffectsOnFiber( } case OffscreenComponent: { if (flags & Ref) { - if (current !== null) { + if (!offscreenSubtreeWasHidden && current !== null) { safelyDetachRef(current, current.return); } } @@ -2074,10 +2078,12 @@ function commitMutationEffectsOnFiber( // TODO: This is a temporary solution that allowed us to transition away // from React Flare on www. if (flags & Ref) { - if (current !== null) { + if (!offscreenSubtreeWasHidden && current !== null) { safelyDetachRef(finishedWork, finishedWork.return); } - safelyAttachRef(finishedWork, finishedWork.return); + if (!offscreenSubtreeIsHidden) { + safelyAttachRef(finishedWork, finishedWork.return); + } } if (flags & Update) { const scopeInstance = finishedWork.stateNode; diff --git a/packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js b/packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js index cc9deb6236..093d164e70 100644 --- a/packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js +++ b/packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js @@ -305,6 +305,131 @@ describe('ReactFreshIntegration', () => { } }); + // @gate __DEV__ && enableActivity + it('ignores ref for class component in hidden subtree', async () => { + const code = ` + import {unstable_Activity as Activity} from 'react'; + + // Avoid creating a new class on Fast Refresh. + global.A = global.A ?? class A extends React.Component { + render() { + return
; + } + } + const A = global.A; + + function hiddenRef() { + throw new Error('Unexpected hiddenRef() invocation.'); + } + + export default function App() { + return ( + + + + ); + }; + `; + + await render(code); + await patch(code); + }); + + // @gate __DEV__ && enableActivity + it('ignores ref for hoistable resource in hidden subtree', async () => { + const code = ` + import {unstable_Activity as Activity} from 'react'; + + function hiddenRef() { + throw new Error('Unexpected hiddenRef() invocation.'); + } + + export default function App() { + return ( + + + + ); + }; + `; + + await render(code); + await patch(code); + }); + + // @gate __DEV__ && enableActivity + it('ignores ref for host component in hidden subtree', async () => { + const code = ` + import {unstable_Activity as Activity} from 'react'; + + function hiddenRef() { + throw new Error('Unexpected hiddenRef() invocation.'); + } + + export default function App() { + return ( + +
+ + ); + }; + `; + + await render(code); + await patch(code); + }); + + // @gate __DEV__ && enableActivity + it('ignores ref for Activity in hidden subtree', async () => { + const code = ` + import {unstable_Activity as Activity} from 'react'; + + function hiddenRef(value) { + throw new Error('Unexpected hiddenRef() invocation.'); + } + + export default function App() { + return ( + + +
+ + + ); + }; + `; + + await render(code); + await patch(code); + }); + + // @gate __DEV__ && enableActivity && enableScopeAPI + it('ignores ref for Scope in hidden subtree', async () => { + const code = ` + import { + unstable_Activity as Activity, + unstable_Scope as Scope, + } from 'react'; + + function hiddenRef(value) { + throw new Error('Unexpected hiddenRef() invocation.'); + } + + export default function App() { + return ( + + +
+ + + ); + }; + `; + + await render(code); + await patch(code); + }); + it('reloads HOCs if they return functions', async () => { if (__DEV__) { await render(`