Skip to content

Prevent infinite re-renders in StrictMode + Offscreen #25203

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
merged 4 commits into from
Sep 9, 2022
Merged

Prevent infinite re-renders in StrictMode + Offscreen #25203

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3523be2
Prevent infinite re-render in StrictMode + Offscreen
sammy-SC Sep 7, 2022
fc298f5
Only fire effects for Offscreen when it is revealed
sammy-SC Sep 9, 2022
618b38c
Move setting debug fiber into if branch
sammy-SC Sep 9, 2022
3b01085
Move settings of debug fiber out of if branch
sammy-SC Sep 9, 2022
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
Next Next commit
Prevent infinite re-render in StrictMode + Offscreen
  • Loading branch information
@sammy-SC
sammy-SC committed Sep 8, 2022
commit 3523be2e402af8b8c0ccdac43fa14d98e56866a2
6 changes: 5 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ import {
LayoutMask,
PassiveMask,
PlacementDEV,
Visibility,
} from './ReactFiberFlags';
import {
NoLanes,
Expand Down Expand Up @@ -3184,9 +3185,12 @@ function doubleInvokeEffectsInDEV(
) {
const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;

if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
setCurrentDebugFiberInDEV(fiber);
if (isInStrictMode) {
const hasOffscreenVisibilityFlag =
fiber.tag !== OffscreenComponent || fiber.flags & Visibility;
if (isInStrictMode && hasOffscreenVisibilityFlag) {
Copy link
Collaborator

@acdlite acdlite Sep 9, 2022

Choose a reason for hiding this comment

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

Can you explain why this fixes it? Is it because we shouldn't fire effects in a hidden offscreen tree?

Copy link
Contributor Author

@sammy-SC sammy-SC Sep 9, 2022

Choose a reason for hiding this comment

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

As far as I understand, Visibility flag is set on Offscreen when its visibility changes.

We only want to fire effects on Offscreen when it is visible and only when its visibility changes. This prevents the infinite loop. Without it, it kept firing effects unconditionally if any effect causes synchronous re-render.

Copy link
Collaborator

@acdlite acdlite Sep 9, 2022

Choose a reason for hiding this comment

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

We only want to fire effects on Offscreen when it is visible and only when its visibility changes.

I don't think that's true, the strict mode double invoke logic happens for all newly mounted trees, not just Offscreen ones. (Consider that this behavior is already in open source and offscreen doesn't exist.)

In fact, I don't think we need any Offscreen specific logic at all. Do you remember why you added the fiber.tag === OffscreenComponent check originally? None of the tests seem to fail if you remove it. And it also fixes the regression test.

Copy link
Collaborator

@acdlite acdlite Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tried running the regression test on main and it passes even without any changes, so we need to fix that Nvm was running in the wrong release channel

Copy link
Contributor Author

@sammy-SC sammy-SC Sep 9, 2022

Choose a reason for hiding this comment

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

Do you remember why you added the fiber.tag === OffscreenComponent check originally?

It is needed to prevent double invoking on components that are hidden by OffscreenComponent.

This is the test specifically for that behaviour: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js#L70

Without the special condition for Offscreen, <Component label="A" /> will have its effects invoked.

I think it would be unexpected for our users to see mount/unmount on a component that is hidden by Offscreen.

Copy link
Member

@rickhanlonii rickhanlonii Sep 11, 2022

Choose a reason for hiding this comment

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

What if you change code in a parent that would break an offscreen subtree, but you don't know because you don't navigate to that offscreen component in development (because you're not working on that tab/modal, for example). It seems like these type of bugs are something you eagerly want to know about with StrictMode.

Curious what @gaearon thinks as well.

Copy link
Contributor Author

@sammy-SC sammy-SC Sep 12, 2022

Choose a reason for hiding this comment

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

What if you change code in a parent that would break an offscreen subtree, but you don't know because you don't navigate to that offscreen component in development (because you're not working on that tab/modal, for example).

I agree, there is definitely a value in double invoking effects even if Offscreen is hidden. On the other hand, it will cause confusion. Suddenly you would see effects triggering for something you wouldn't expect. This will be even more confusing with pre rendering where a screen that engineer might not even know about has its effects executed. I think there is already expectation with StrictMode that component needs to be used to trigger double invoking. It aligns well with Offscreen suppressing when hidden.

Copy link
Member

@rickhanlonii rickhanlonii Sep 12, 2022

Choose a reason for hiding this comment

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

Suddenly you would see effects triggering for something you wouldn't expect.

Isn't this also the case for double rendering? Is it confusing that we double render those hidden trees but don't double fire the effects?

Copy link
Contributor Author

@sammy-SC sammy-SC Sep 12, 2022

Choose a reason for hiding this comment

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

I don't think so. Take pre-rendering as an example. User expects render functions to be executed even if the subtree is hidden. This is true for pre-rendering example or for possible virtualised list implementations.
I do see value in double invoking effects in hidden subtree as well. I don't hold strong opinions on this. I'm just explaining how I was thinking about this when I implemented it.

Copy link
Member

@rickhanlonii rickhanlonii Sep 12, 2022

Choose a reason for hiding this comment

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

Discussed offline, the other concern here is that to do this right, you also need to hide the currently visible content, as though you fully switch to the hidden tab. And when you switch to the hidden tab, you may not have the props you need to do a proper render. For example, if you're getting the userId from the route, then you don't have an ID to test the offscreen tree with, so you end up testing behavior that doesn't occur in the wild.

So it's easier to test making visible content hidden than it is testing hidden/pre-rendered content visible without more information. The best practice is to test navigating to the offscreen trees.

disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
Expand Down
6 changes: 5 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ import {
LayoutMask,
PassiveMask,
PlacementDEV,
Visibility,
} from './ReactFiberFlags';
import {
NoLanes,
Expand Down Expand Up @@ -3184,9 +3185,12 @@ function doubleInvokeEffectsInDEV(
) {
const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
const isInStrictMode = parentIsInStrictMode || isStrictModeFiber;

if (fiber.flags & PlacementDEV || fiber.tag === OffscreenComponent) {
setCurrentDebugFiberInDEV(fiber);
if (isInStrictMode) {
const hasOffscreenVisibilityFlag =
fiber.tag !== OffscreenComponent || fiber.flags & Visibility;
if (isInStrictMode && hasOffscreenVisibilityFlag) {
disappearLayoutEffects(fiber);
disconnectPassiveEffect(fiber);
reappearLayoutEffects(root, fiber.alternate, fiber, false);
Expand Down
42 changes: 42 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,21 @@ describe('ReactOffscreenStrictMode', () => {

log = [];

act(() => {
ReactNoop.render(
<React.StrictMode>
<Offscreen mode="hidden">
<Component label="A" />
<Component label="B" />
</Offscreen>
</React.StrictMode>,
);
});

expect(log).toEqual(['A: render', 'A: render', 'B: render', 'B: render']);

log = [];

act(() => {
ReactNoop.render(
<React.StrictMode>
Expand All @@ -92,4 +107,31 @@ describe('ReactOffscreenStrictMode', () => {
'A: useEffect mount',
]);
});

it('should not cause infinite render loop when StrictMode is used with Suspense and synchronous set states', () => {
// This is a regression test, see https://github.com/facebook/react/pull/25179 for more details.
function App() {
const [state, setState] = React.useState(false);

React.useLayoutEffect(() => {
setState(true);
}, []);

React.useEffect(() => {
// Empty useEffect with empty dependency array is needed to trigger infinite render loop.
}, []);

return state;
}

act(() => {
ReactNoop.render(
<React.StrictMode>
<React.Suspense>
<App />
</React.Suspense>
</React.StrictMode>,
);
});
});
});