Skip to content

Fix Ref Lifecycles in Hidden Subtrees #31379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yungsters merged 10 commits into from
Oct 31, 2024
Merged

Fix Ref Lifecycles in Hidden Subtrees #31379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bdfe9fe
Avoid Detaching Refs on Hidden Subtrees
yungsters Oct 28, 2024
a2c9e7a
Add fast refresh test
rickhanlonii Oct 29, 2024
2101de0
Isolate `!offscreenSubtreeWasHidden` checks from `flags` checks
yungsters Oct 29, 2024
1426f6d
Hoist `__DEV__` check into `@gate` pragma
yungsters Oct 29, 2024
52c0e97
Check `offscreenSubtreeIsHidden` for attaching ref of `Scope`
yungsters Oct 29, 2024
a0685eb
Simplify test code (in preparation for adding more)
yungsters Oct 30, 2024
4036c91
Add more test cases
yungsters Oct 30, 2024
1b7c7ee
Drop changes to `ScopeComponent` in `commitMutationEffectsOnFiber`
yungsters Oct 30, 2024
dc0c3a5
Fix missing `Activity` import in test case
yungsters Oct 30, 2024
82bea69
Fix `ScopeComponent` detach/attach behavior
yungsters Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
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);
}
Comment on lines +1340 to +1342
Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the unit test for "ignores ref for Activity in hidden subtree", I discovered that this and the call from commitMutationEffectsOnFiber are triggered, causing X to be incorrectly invoked twice! 🤯

<Activity mode="hidden">
  <Activity mode="visible" ref={X}>
    <div />
  </Activity>
</Activity>

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);
}
Comment on lines +2081 to +2086
Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kassens, I think this is the correct behavior.

You were right that !offscreenSubtreeWasHidden should only be scoped to safelyDetachRef, and we were missing a !offscreenSubtreeIsHidden check around safelyAttachRef.

Copy link
Copy Markdown
Member

@rickhanlonii rickhanlonii Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this behavior? We don't check for Offscreen anywhere else that we attach.

Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't check for Offscreen anywhere else that we attach.

We actually do, but it's implicit.

Every other type calls safelyAttachRef from reappearLayoutEffects, which is called only by recursivelyTraverseReappearLayoutEffects. These occur during:

In both of these, we prune traversal when we encounter a hidden Offscreen. (See the links above for where this happens.)

Are you sure about this behavior?

I'm not sure why ScopeComponent requires safelyAttachRef to be here, but I assume it has something to do with the comment above:

// TODO: This is a temporary solution that allowed us to transition away
// from React Flare on www.

I don't plan to change this as part of fixing refs, though. I considered an alternative in which I leave ScopeComponent alone, but I think this is actually the correct fix.

Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why ScopeComponent requires safelyAttachRef to be here, [...]

I traced the origin of that comment (which was harder than I expected due to the reconciler forking and unforking that happened) and found this: #19264

To unblock internal progression on replacing React Flare, let's instead move resolution of Scope API refs to the mutation phase, so we can attach event listeners to scopes before the layout phase begins.

I don't fully understand the context, but I figured that it would be helpful to know its origin.

Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~~After writing unit tests for this bug, I've confirmed that these changes are actually not needed to prevent <Scope ref={X}> from invoking X in <Activity mode="hidden">.

I've dropped these changes for now.~~

Oh… just kidding, I was not running yarn test with -r=www-classic (which is where enableScopeAPI is enabled). These changes are needed after all, will update.

Copy link
Copy Markdown
Contributor Author

@yungsters yungsters Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that Scope was previously completely ignoring any parent Activity that declared the subtree as hidden.

}
if (flags & Update) {
const scopeInstance = finishedWork.stateNode;
Expand Down
125 changes: 125 additions & 0 deletions packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js
View file
Open in desktop
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