diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index e7a3e157301be..c67a874119dab 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -66,7 +66,6 @@ import { ChildDeletion, Snapshot, Update, - Callback, Ref, Hydrating, HydratingAndUpdate, @@ -75,6 +74,7 @@ import { MutationMask, LayoutMask, PassiveMask, + Visibility, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import invariant from 'shared/invariant'; @@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber( finishedWork: Fiber, committedLanes: Lanes, ): void { - if ((finishedWork.flags & (Update | Callback)) !== NoFlags) { + if ((finishedWork.flags & LayoutMask) !== NoFlags) { switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { - commitSuspenseComponent(finishedWork); + commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); return; } @@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { - commitSuspenseComponent(finishedWork); + commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); return; } @@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { } break; } - case OffscreenComponent: - case LegacyHiddenComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - hideOrUnhideAllChildren(finishedWork, isHidden); - return; - } } invariant( false, @@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { ); } -function commitSuspenseComponent(finishedWork: Fiber) { +function commitSuspenseCallback(finishedWork: Fiber) { + // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; - - if (newState !== null) { - markCommitTimeOfFallback(); - - if (supportsMutation) { - // Hide the Offscreen component that contains the primary children. TODO: - // Ideally, this effect would have been scheduled on the Offscreen fiber - // itself. That's how unhiding works: the Offscreen component schedules an - // effect on itself. However, in this case, the component didn't complete, - // so the fiber was never added to the effect list in the normal path. We - // could have appended it to the effect list in the Suspense component's - // second pass, but doing it this way is less complicated. This would be - // simpler if we got rid of the effect list and traversed the tree, like - // we're planning to do. - const primaryChildParent: Fiber = (finishedWork.child: any); - hideOrUnhideAllChildren(primaryChildParent, true); - } - } - if (enableSuspenseCallback && newState !== null) { const suspenseCallback = finishedWork.memoizedProps.suspenseCallback; if (typeof suspenseCallback === 'function') { @@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) { } function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { + // TODO: The factoring of this phase could probably be improved. Consider + // switching on the type of work before checking the flags. That's what + // we do in all the other phases. I think this one is only different + // because of the shared reconcilation logic below. const flags = finishedWork.flags; if (flags & ContentReset) { @@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { } } + if (flags & Visibility) { + switch (finishedWork.tag) { + case SuspenseComponent: { + const newState: OffscreenState | null = finishedWork.memoizedState; + if (newState !== null) { + markCommitTimeOfFallback(); + // Hide the Offscreen component that contains the primary children. + // TODO: Ideally, this effect would have been scheduled on the + // Offscreen fiber itself. That's how unhiding works: the Offscreen + // component schedules an effect on itself. However, in this case, the + // component didn't complete, so the fiber was never added to the + // effect list in the normal path. We could have appended it to the + // effect list in the Suspense component's second pass, but doing it + // this way is less complicated. This would be simpler if we got rid + // of the effect list and traversed the tree, like we're planning to + // do. + const primaryChildParent: Fiber = (finishedWork.child: any); + hideOrUnhideAllChildren(primaryChildParent, true); + } + break; + } + case OffscreenComponent: + case LegacyHiddenComponent: { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + hideOrUnhideAllChildren(finishedWork, isHidden); + break; + } + } + } + // The following switch statement is only concerned about placement, // updates, and deletions. To avoid needing to add a case for every possible // bitmap value, we remove the secondary effects from the effect tag and diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index a4ce8c644954d..cbaf8ed8c4984 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -66,7 +66,6 @@ import { ChildDeletion, Snapshot, Update, - Callback, Ref, Hydrating, HydratingAndUpdate, @@ -75,6 +74,7 @@ import { MutationMask, LayoutMask, PassiveMask, + Visibility, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import invariant from 'shared/invariant'; @@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber( finishedWork: Fiber, committedLanes: Lanes, ): void { - if ((finishedWork.flags & (Update | Callback)) !== NoFlags) { + if ((finishedWork.flags & LayoutMask) !== NoFlags) { switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: @@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { - commitSuspenseComponent(finishedWork); + commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); return; } @@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { - commitSuspenseComponent(finishedWork); + commitSuspenseCallback(finishedWork); attachSuspenseRetryListeners(finishedWork); return; } @@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { } break; } - case OffscreenComponent: - case LegacyHiddenComponent: { - const newState: OffscreenState | null = finishedWork.memoizedState; - const isHidden = newState !== null; - hideOrUnhideAllChildren(finishedWork, isHidden); - return; - } } invariant( false, @@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { ); } -function commitSuspenseComponent(finishedWork: Fiber) { +function commitSuspenseCallback(finishedWork: Fiber) { + // TODO: Move this to passive phase const newState: SuspenseState | null = finishedWork.memoizedState; - - if (newState !== null) { - markCommitTimeOfFallback(); - - if (supportsMutation) { - // Hide the Offscreen component that contains the primary children. TODO: - // Ideally, this effect would have been scheduled on the Offscreen fiber - // itself. That's how unhiding works: the Offscreen component schedules an - // effect on itself. However, in this case, the component didn't complete, - // so the fiber was never added to the effect list in the normal path. We - // could have appended it to the effect list in the Suspense component's - // second pass, but doing it this way is less complicated. This would be - // simpler if we got rid of the effect list and traversed the tree, like - // we're planning to do. - const primaryChildParent: Fiber = (finishedWork.child: any); - hideOrUnhideAllChildren(primaryChildParent, true); - } - } - if (enableSuspenseCallback && newState !== null) { const suspenseCallback = finishedWork.memoizedProps.suspenseCallback; if (typeof suspenseCallback === 'function') { @@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) { } function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { + // TODO: The factoring of this phase could probably be improved. Consider + // switching on the type of work before checking the flags. That's what + // we do in all the other phases. I think this one is only different + // because of the shared reconcilation logic below. const flags = finishedWork.flags; if (flags & ContentReset) { @@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { } } + if (flags & Visibility) { + switch (finishedWork.tag) { + case SuspenseComponent: { + const newState: OffscreenState | null = finishedWork.memoizedState; + if (newState !== null) { + markCommitTimeOfFallback(); + // Hide the Offscreen component that contains the primary children. + // TODO: Ideally, this effect would have been scheduled on the + // Offscreen fiber itself. That's how unhiding works: the Offscreen + // component schedules an effect on itself. However, in this case, the + // component didn't complete, so the fiber was never added to the + // effect list in the normal path. We could have appended it to the + // effect list in the Suspense component's second pass, but doing it + // this way is less complicated. This would be simpler if we got rid + // of the effect list and traversed the tree, like we're planning to + // do. + const primaryChildParent: Fiber = (finishedWork.child: any); + hideOrUnhideAllChildren(primaryChildParent, true); + } + break; + } + case OffscreenComponent: + case LegacyHiddenComponent: { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + hideOrUnhideAllChildren(finishedWork, isHidden); + break; + } + } + } + // The following switch statement is only concerned about placement, // updates, and deletions. To avoid needing to add a case for every possible // bitmap value, we remove the secondary effects from the effect tag and diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 29c5fe48c4b2f..c919f15222465 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -9,7 +9,11 @@ import type {Fiber} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.new'; -import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes'; +import type { + ReactScopeInstance, + ReactContext, + Wakeable, +} from 'shared/ReactTypes'; import type {FiberRoot} from './ReactInternalTypes'; import type { Instance, @@ -60,6 +64,7 @@ import { Ref, RefStatic, Update, + Visibility, NoFlags, DidCapture, Snapshot, @@ -320,7 +325,7 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - if ((node.flags & Update) !== NoFlags) { + if ((node.flags & Visibility) !== NoFlags) { // Need to toggle the visibility of the primary children. const newIsHidden = node.memoizedState !== null; if (newIsHidden) { @@ -405,7 +410,7 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - if ((node.flags & Update) !== NoFlags) { + if ((node.flags & Visibility) !== NoFlags) { // Need to toggle the visibility of the primary children. const newIsHidden = node.memoizedState !== null; if (newIsHidden) { @@ -1084,32 +1089,40 @@ function completeWork( } } - if (supportsPersistence) { - // TODO: Only schedule updates if not prevDidTimeout. - if (nextDidTimeout) { - // If this boundary just timed out, schedule an effect to attach a - // retry listener to the promise. This flag is also used to hide the - // primary children. - workInProgress.flags |= Update; - } + const wakeables: Set | null = (workInProgress.updateQueue: any); + if (wakeables !== null) { + // Schedule an effect to attach a retry listener to the promise. + // TODO: Move to passive phase + workInProgress.flags |= Update; } + if (supportsMutation) { - // TODO: Only schedule updates if these values are non equal, i.e. it changed. - if (nextDidTimeout || prevDidTimeout) { - // If this boundary just timed out, schedule an effect to attach a - // retry listener to the promise. This flag is also used to hide the - // primary children. In mutation mode, we also need the flag to - // *unhide* children that were previously hidden, so check if this - // is currently timed out, too. - workInProgress.flags |= Update; + if (nextDidTimeout !== prevDidTimeout) { + // In mutation mode, visibility is toggled by mutating the nearest + // host nodes whenever they switch from hidden -> visible or vice + // versa. We don't need to switch when the boundary updates but its + // visibility hasn't changed. + workInProgress.flags |= Visibility; } } + if (supportsPersistence) { + if (nextDidTimeout) { + // In persistent mode, visibility is toggled by cloning the nearest + // host nodes in the complete phase whenever the boundary is hidden. + // TODO: The plan is to add a transparent host wrapper (no layout) + // around the primary children and hide that node. Then we don't need + // to do the funky cloning business. + workInProgress.flags |= Visibility; + } + } + if ( enableSuspenseCallback && workInProgress.updateQueue !== null && workInProgress.memoizedProps.suspenseCallback != null ) { // Always notify the callback + // TODO: Move to passive phase workInProgress.flags |= Update; } bubbleProperties(workInProgress); @@ -1396,7 +1409,7 @@ function completeWork( prevIsHidden !== nextIsHidden && newProps.mode !== 'unstable-defer-without-hiding' ) { - workInProgress.flags |= Update; + workInProgress.flags |= Visibility; } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index a119a6b975190..59d72380dbeeb 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -9,7 +9,11 @@ import type {Fiber} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.old'; -import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes'; +import type { + ReactScopeInstance, + ReactContext, + Wakeable, +} from 'shared/ReactTypes'; import type {FiberRoot} from './ReactInternalTypes'; import type { Instance, @@ -60,6 +64,7 @@ import { Ref, RefStatic, Update, + Visibility, NoFlags, DidCapture, Snapshot, @@ -320,7 +325,7 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - if ((node.flags & Update) !== NoFlags) { + if ((node.flags & Visibility) !== NoFlags) { // Need to toggle the visibility of the primary children. const newIsHidden = node.memoizedState !== null; if (newIsHidden) { @@ -405,7 +410,7 @@ if (supportsMutation) { // down its children. Instead, we'll get insertions from each child in // the portal directly. } else if (node.tag === SuspenseComponent) { - if ((node.flags & Update) !== NoFlags) { + if ((node.flags & Visibility) !== NoFlags) { // Need to toggle the visibility of the primary children. const newIsHidden = node.memoizedState !== null; if (newIsHidden) { @@ -1084,32 +1089,40 @@ function completeWork( } } - if (supportsPersistence) { - // TODO: Only schedule updates if not prevDidTimeout. - if (nextDidTimeout) { - // If this boundary just timed out, schedule an effect to attach a - // retry listener to the promise. This flag is also used to hide the - // primary children. - workInProgress.flags |= Update; - } + const wakeables: Set | null = (workInProgress.updateQueue: any); + if (wakeables !== null) { + // Schedule an effect to attach a retry listener to the promise. + // TODO: Move to passive phase + workInProgress.flags |= Update; } + if (supportsMutation) { - // TODO: Only schedule updates if these values are non equal, i.e. it changed. - if (nextDidTimeout || prevDidTimeout) { - // If this boundary just timed out, schedule an effect to attach a - // retry listener to the promise. This flag is also used to hide the - // primary children. In mutation mode, we also need the flag to - // *unhide* children that were previously hidden, so check if this - // is currently timed out, too. - workInProgress.flags |= Update; + if (nextDidTimeout !== prevDidTimeout) { + // In mutation mode, visibility is toggled by mutating the nearest + // host nodes whenever they switch from hidden -> visible or vice + // versa. We don't need to switch when the boundary updates but its + // visibility hasn't changed. + workInProgress.flags |= Visibility; } } + if (supportsPersistence) { + if (nextDidTimeout) { + // In persistent mode, visibility is toggled by cloning the nearest + // host nodes in the complete phase whenever the boundary is hidden. + // TODO: The plan is to add a transparent host wrapper (no layout) + // around the primary children and hide that node. Then we don't need + // to do the funky cloning business. + workInProgress.flags |= Visibility; + } + } + if ( enableSuspenseCallback && workInProgress.updateQueue !== null && workInProgress.memoizedProps.suspenseCallback != null ) { // Always notify the callback + // TODO: Move to passive phase workInProgress.flags |= Update; } bubbleProperties(workInProgress); @@ -1396,7 +1409,7 @@ function completeWork( prevIsHidden !== nextIsHidden && newProps.mode !== 'unstable-defer-without-hiding' ) { - workInProgress.flags |= Update; + workInProgress.flags |= Visibility; } } diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index b711d730c3b2f..ce6460b023675 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -82,7 +82,7 @@ export const MutationMask = Ref | Hydrating | Visibility; -export const LayoutMask = Update | Callback | Ref; +export const LayoutMask = Update | Callback | Ref | Visibility; // TODO: Split into PassiveMountMask and PassiveUnmountMask export const PassiveMask = Passive | ChildDeletion; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js new file mode 100644 index 0000000000000..8ed56bd5f2d96 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js @@ -0,0 +1,74 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +'use strict'; + +let React; +let ReactDOM; +let act; + +describe('ReactSuspenseEffectsSemanticsDOM', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactDOM = require('react-dom'); + act = require('jest-react').act; + }); + + it('should not cause a cycle when combined with a render phase update', () => { + let scheduleSuspendingUpdate; + + function App() { + const [value, setValue] = React.useState(true); + + scheduleSuspendingUpdate = () => setValue(!value); + + return ( + <> + + + + + + ); + } + + function ComponentThatCausesBug({value}) { + const [mirroredValue, setMirroredValue] = React.useState(value); + if (mirroredValue !== value) { + setMirroredValue(value); + } + + // eslint-disable-next-line no-unused-vars + const [_, setRef] = React.useState(null); + + return
; + } + + const promise = Promise.resolve(); + + function ComponentThatSuspendsOnUpdate({shouldSuspend}) { + if (shouldSuspend) { + // Fake Suspend + throw promise; + } + return null; + } + + act(() => { + const root = ReactDOM.createRoot(document.createElement('div')); + root.render(); + }); + + act(() => { + scheduleSuspendingUpdate(); + }); + }); +});