Skip to content

Commit

Permalink
Fix Ref Lifecycles in Hidden Subtrees (#31379)
Browse files Browse the repository at this point in the history
## Summary

We're seeing certain situations in React Native development where ref
callbacks in `<Activity mode="hidden">` 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
ec52a56
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.
}

<Activity mode="hidden">
  <View ref={callbackRef} />;
</Activity>
```

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 <rickhanlonii@fb.com>
  • Loading branch information
yungsters and rickhanlonii authored Oct 31, 2024
1 parent 603e610 commit ea3ac58
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 9 deletions.
24 changes: 15 additions & 9 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,9 @@ function commitDeletionEffectsOnFiber(
}
case ScopeComponent: {
if (enableScopeAPI) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
if (!offscreenSubtreeWasHidden) {
safelyDetachRef(deletedFiber, nearestMountedAncestor);
}
}
recursivelyTraverseDeletionEffects(
finishedRoot,
Expand All @@ -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
Expand Down Expand Up @@ -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++) {
Expand Down Expand Up @@ -1637,7 +1641,7 @@ function commitMutationEffectsOnFiber(
commitReconciliationEffects(finishedWork);

if (flags & Ref) {
if (current !== null) {
if (!offscreenSubtreeWasHidden && current !== null) {
safelyDetachRef(current, current.return);
}
}
Expand All @@ -1660,7 +1664,7 @@ function commitMutationEffectsOnFiber(
commitReconciliationEffects(finishedWork);

if (flags & Ref) {
if (current !== null) {
if (!offscreenSubtreeWasHidden && current !== null) {
safelyDetachRef(current, current.return);
}
}
Expand Down Expand Up @@ -1745,7 +1749,7 @@ function commitMutationEffectsOnFiber(
commitReconciliationEffects(finishedWork);

if (flags & Ref) {
if (current !== null) {
if (!offscreenSubtreeWasHidden && current !== null) {
safelyDetachRef(current, current.return);
}
}
Expand Down Expand Up @@ -1961,7 +1965,7 @@ function commitMutationEffectsOnFiber(
}
case OffscreenComponent: {
if (flags & Ref) {
if (current !== null) {
if (!offscreenSubtreeWasHidden && current !== null) {
safelyDetachRef(current, current.return);
}
}
Expand Down Expand Up @@ -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;
Expand Down
125 changes: 125 additions & 0 deletions packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div />;
}
}
const A = global.A;
function hiddenRef() {
throw new Error('Unexpected hiddenRef() invocation.');
}
export default function App() {
return (
<Activity mode="hidden">
<A ref={hiddenRef} />
</Activity>
);
};
`;

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 (
<Activity mode="hidden">
<link rel="preload" href="foo" ref={hiddenRef} />
</Activity>
);
};
`;

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 (
<Activity mode="hidden">
<div ref={hiddenRef} />
</Activity>
);
};
`;

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 (
<Activity mode="hidden">
<Activity mode="visible" ref={hiddenRef}>
<div />
</Activity>
</Activity>
);
};
`;

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 (
<Activity mode="hidden">
<Scope ref={hiddenRef}>
<div />
</Scope>
</Activity>
);
};
`;

await render(code);
await patch(code);
});

it('reloads HOCs if they return functions', async () => {
if (__DEV__) {
await render(`
Expand Down

0 comments on commit ea3ac58

Please sign in to comment.