@samitouri / QOS-React / commits / d8cfeaf221

Fix context propagation for offscreen/fallback trees (#23095)

* Failing test for Context.Consumer in suspended Suspense See issue #19701. * Fix context propagation for offscreen trees * Address nits * Specify propagation root for Suspense too * Pass correct propagation root * Harden test coverage This test will fail if we remove propagation, or if we propagate with a root node like fiber.return or fiber.return.return. The additional DEV-only error helps detect a different kind of mistake, like if the thing being passed hasn't actually been encountered on the way up. However, we still leave the actual production loop to check against null so that there is no way we loop forever if the propagation root is wrong. * Remove superfluous warning Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>

Dan Abramov committed Jan 19, 2022 at 16:30 UTC d8cfeaf221563f3828fe7bf07833f3824accfa39
6 files changed +253 -22
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+9 -5
@@ -174,7 +174,7 @@ import {
174 checkIfContextChanged,
175 readContext,
176 prepareToReadContext,
177 - scheduleWorkOnParentPath,
177 + scheduleContextWorkOnParentPath,
178 } from './ReactFiberNewContext.new';
179 import {
180 renderWithHooks,
@@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
2754 }
2755 }
2756
2757 -function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
2757 +function scheduleSuspenseWorkOnFiber(
2758 + fiber: Fiber,
2759 + renderLanes: Lanes,
2760 + propagationRoot: Fiber,
2761 +) {
2762 fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
2763 const alternate = fiber.alternate;
2764 if (alternate !== null) {
2765 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
2766 }
2763 - scheduleWorkOnParentPath(fiber.return, renderLanes);
2767 + scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
2768 }
2769
2770 function propagateSuspenseContextChange(
@@ -2776,7 +2780,7 @@ function propagateSuspenseContextChange(
2780 if (node.tag === SuspenseComponent) {
2781 const state: SuspenseState | null = node.memoizedState;
2782 if (state !== null) {
2779 - scheduleWorkOnFiber(node, renderLanes);
2783 + scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
2784 }
2785 } else if (node.tag === SuspenseListComponent) {
2786 // If the tail is hidden there might not be an Suspense boundaries
@@ -2784,7 +2788,7 @@ function propagateSuspenseContextChange(
2788 // list itself.
2789 // We don't have to traverse to the children of the list since
2790 // the list will propagate the change when it rerenders.
2787 - scheduleWorkOnFiber(node, renderLanes);
2791 + scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
2792 } else if (node.child !== null) {
2793 node.child.return = node;
2794 node = node.child;
packages/react-reconciler/src/ReactFiberBeginWork.old.js
+9 -5
@@ -174,7 +174,7 @@ import {
174 checkIfContextChanged,
175 readContext,
176 prepareToReadContext,
177 - scheduleWorkOnParentPath,
177 + scheduleContextWorkOnParentPath,
178 } from './ReactFiberNewContext.old';
179 import {
180 renderWithHooks,
@@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
2754 }
2755 }
2756
2757 -function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
2757 +function scheduleSuspenseWorkOnFiber(
2758 + fiber: Fiber,
2759 + renderLanes: Lanes,
2760 + propagationRoot: Fiber,
2761 +) {
2762 fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
2763 const alternate = fiber.alternate;
2764 if (alternate !== null) {
2765 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
2766 }
2763 - scheduleWorkOnParentPath(fiber.return, renderLanes);
2767 + scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
2768 }
2769
2770 function propagateSuspenseContextChange(
@@ -2776,7 +2780,7 @@ function propagateSuspenseContextChange(
2780 if (node.tag === SuspenseComponent) {
2781 const state: SuspenseState | null = node.memoizedState;
2782 if (state !== null) {
2779 - scheduleWorkOnFiber(node, renderLanes);
2783 + scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
2784 }
2785 } else if (node.tag === SuspenseListComponent) {
2786 // If the tail is hidden there might not be an Suspense boundaries
@@ -2784,7 +2788,7 @@ function propagateSuspenseContextChange(
2788 // list itself.
2789 // We don't have to traverse to the children of the list since
2790 // the list will propagate the change when it rerenders.
2787 - scheduleWorkOnFiber(node, renderLanes);
2791 + scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
2792 } else if (node.child !== null) {
2793 node.child.return = node;
2794 node = node.child;
packages/react-reconciler/src/ReactFiberNewContext.new.js
+37 -6
@@ -138,9 +138,10 @@ export function popProvider(
138 }
139 }
140
141 -export function scheduleWorkOnParentPath(
141 +export function scheduleContextWorkOnParentPath(
142 parent: Fiber | null,
143 renderLanes: Lanes,
144 + propagationRoot: Fiber,
145 ) {
146 // Update the child lanes of all the ancestors, including the alternates.
147 let node = parent;
@@ -157,12 +158,26 @@ export function scheduleWorkOnParentPath(
158 ) {
159 alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
160 } else {
160 - // Neither alternate was updated, which means the rest of the
161 + // Neither alternate was updated.
162 + // Normally, this would mean that the rest of the
163 // ancestor path already has sufficient priority.
164 + // However, this is not necessarily true inside offscreen
165 + // or fallback trees because childLanes may be inconsistent
166 + // with the surroundings. This is why we continue the loop.
167 + }
168 + if (node === propagationRoot) {
169 break;
170 }
171 node = node.return;
172 }
173 + if (__DEV__) {
174 + if (node !== propagationRoot) {
175 + console.error(
176 + 'Expected to find the propagation root when scheduling context work. ' +
177 + 'This error is likely caused by a bug in React. Please file an issue.',
178 + );
179 + }
180 + }
181 }
182
183 export function propagateContextChange<T>(
@@ -246,7 +261,11 @@ function propagateContextChange_eager<T>(
261 if (alternate !== null) {
262 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
263 }
249 - scheduleWorkOnParentPath(fiber.return, renderLanes);
264 + scheduleContextWorkOnParentPath(
265 + fiber.return,
266 + renderLanes,
267 + workInProgress,
268 + );
269
270 // Mark the updated lanes on the list, too.
271 list.lanes = mergeLanes(list.lanes, renderLanes);
@@ -284,7 +303,11 @@ function propagateContextChange_eager<T>(
303 // because we want to schedule this fiber as having work
304 // on its children. We'll use the childLanes on
305 // this fiber to indicate that a context has changed.
287 - scheduleWorkOnParentPath(parentSuspense, renderLanes);
306 + scheduleContextWorkOnParentPath(
307 + parentSuspense,
308 + renderLanes,
309 + workInProgress,
310 + );
311 nextFiber = fiber.sibling;
312 } else {
313 // Traverse down.
@@ -365,7 +388,11 @@ function propagateContextChanges<T>(
388 if (alternate !== null) {
389 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
390 }
368 - scheduleWorkOnParentPath(consumer.return, renderLanes);
391 + scheduleContextWorkOnParentPath(
392 + consumer.return,
393 + renderLanes,
394 + workInProgress,
395 + );
396
397 if (!forcePropagateEntireTree) {
398 // During lazy propagation, when we find a match, we can defer
@@ -406,7 +433,11 @@ function propagateContextChanges<T>(
433 // because we want to schedule this fiber as having work
434 // on its children. We'll use the childLanes on
435 // this fiber to indicate that a context has changed.
409 - scheduleWorkOnParentPath(parentSuspense, renderLanes);
436 + scheduleContextWorkOnParentPath(
437 + parentSuspense,
438 + renderLanes,
439 + workInProgress,
440 + );
441 nextFiber = null;
442 } else {
443 // Traverse down.
packages/react-reconciler/src/ReactFiberNewContext.old.js
+37 -6
@@ -138,9 +138,10 @@ export function popProvider(
138 }
139 }
140
141 -export function scheduleWorkOnParentPath(
141 +export function scheduleContextWorkOnParentPath(
142 parent: Fiber | null,
143 renderLanes: Lanes,
144 + propagationRoot: Fiber,
145 ) {
146 // Update the child lanes of all the ancestors, including the alternates.
147 let node = parent;
@@ -157,12 +158,26 @@ export function scheduleWorkOnParentPath(
158 ) {
159 alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
160 } else {
160 - // Neither alternate was updated, which means the rest of the
161 + // Neither alternate was updated.
162 + // Normally, this would mean that the rest of the
163 // ancestor path already has sufficient priority.
164 + // However, this is not necessarily true inside offscreen
165 + // or fallback trees because childLanes may be inconsistent
166 + // with the surroundings. This is why we continue the loop.
167 + }
168 + if (node === propagationRoot) {
169 break;
170 }
171 node = node.return;
172 }
173 + if (__DEV__) {
174 + if (node !== propagationRoot) {
175 + console.error(
176 + 'Expected to find the propagation root when scheduling context work. ' +
177 + 'This error is likely caused by a bug in React. Please file an issue.',
178 + );
179 + }
180 + }
181 }
182
183 export function propagateContextChange<T>(
@@ -246,7 +261,11 @@ function propagateContextChange_eager<T>(
261 if (alternate !== null) {
262 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
263 }
249 - scheduleWorkOnParentPath(fiber.return, renderLanes);
264 + scheduleContextWorkOnParentPath(
265 + fiber.return,
266 + renderLanes,
267 + workInProgress,
268 + );
269
270 // Mark the updated lanes on the list, too.
271 list.lanes = mergeLanes(list.lanes, renderLanes);
@@ -284,7 +303,11 @@ function propagateContextChange_eager<T>(
303 // because we want to schedule this fiber as having work
304 // on its children. We'll use the childLanes on
305 // this fiber to indicate that a context has changed.
287 - scheduleWorkOnParentPath(parentSuspense, renderLanes);
306 + scheduleContextWorkOnParentPath(
307 + parentSuspense,
308 + renderLanes,
309 + workInProgress,
310 + );
311 nextFiber = fiber.sibling;
312 } else {
313 // Traverse down.
@@ -365,7 +388,11 @@ function propagateContextChanges<T>(
388 if (alternate !== null) {
389 alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
390 }
368 - scheduleWorkOnParentPath(consumer.return, renderLanes);
391 + scheduleContextWorkOnParentPath(
392 + consumer.return,
393 + renderLanes,
394 + workInProgress,
395 + );
396
397 if (!forcePropagateEntireTree) {
398 // During lazy propagation, when we find a match, we can defer
@@ -406,7 +433,11 @@ function propagateContextChanges<T>(
433 // because we want to schedule this fiber as having work
434 // on its children. We'll use the childLanes on
435 // this fiber to indicate that a context has changed.
409 - scheduleWorkOnParentPath(parentSuspense, renderLanes);
436 + scheduleContextWorkOnParentPath(
437 + parentSuspense,
438 + renderLanes,
439 + workInProgress,
440 + );
441 nextFiber = null;
442 } else {
443 // Traverse down.
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
+63
@@ -1532,5 +1532,68 @@ describe('ReactSuspense', () => {
1532 expect(Scheduler).toFlushUntilNextPaint(['new value']);
1533 expect(root).toMatchRenderedOutput('new value');
1534 });
1535 +
1536 + it('updates context consumer within child of suspended suspense component when context updates', () => {
1537 + const {createContext, useState} = React;
1538 +
1539 + const ValueContext = createContext(null);
1540 +
1541 + const promiseThatNeverResolves = new Promise(() => {});
1542 + function Child() {
1543 + return (
1544 + <ValueContext.Consumer>
1545 + {value => {
1546 + Scheduler.unstable_yieldValue(
1547 + `Received context value [${value}]`,
1548 + );
1549 + if (value === 'default') return <Text text="default" />;
1550 + throw promiseThatNeverResolves;
1551 + }}
1552 + </ValueContext.Consumer>
1553 + );
1554 + }
1555 +
1556 + let setValue;
1557 + function Wrapper({children}) {
1558 + const [value, _setValue] = useState('default');
1559 + setValue = _setValue;
1560 + return (
1561 + <ValueContext.Provider value={value}>
1562 + {children}
1563 + </ValueContext.Provider>
1564 + );
1565 + }
1566 +
1567 + function App() {
1568 + return (
1569 + <Wrapper>
1570 + <Suspense fallback={<Text text="Loading..." />}>
1571 + <Child />
1572 + </Suspense>
1573 + </Wrapper>
1574 + );
1575 + }
1576 +
1577 + const root = ReactTestRenderer.create(<App />);
1578 + expect(Scheduler).toHaveYielded([
1579 + 'Received context value [default]',
1580 + 'default',
1581 + ]);
1582 + expect(root).toMatchRenderedOutput('default');
1583 +
1584 + act(() => setValue('new value'));
1585 + expect(Scheduler).toHaveYielded([
1586 + 'Received context value [new value]',
1587 + 'Loading...',
1588 + ]);
1589 + expect(root).toMatchRenderedOutput('Loading...');
1590 +
1591 + act(() => setValue('default'));
1592 + expect(Scheduler).toHaveYielded([
1593 + 'Received context value [default]',
1594 + 'default',
1595 + ]);
1596 + expect(root).toMatchRenderedOutput('default');
1597 + });
1598 });
1599 });
packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js
+98
@@ -2968,4 +2968,102 @@ describe('ReactSuspenseList', () => {
2968 // treeBaseDuration
2969 expect(onRender.mock.calls[3][3]).toBe(1 + 4 + 5 + 3);
2970 });
2971 +
2972 + // @gate enableSuspenseList
2973 + it('propagates despite a memo bailout', async () => {
2974 + const A = createAsyncText('A');
2975 + const B = createAsyncText('B');
2976 + const C = createAsyncText('C');
2977 +
2978 + const Bailout = React.memo(({children}) => {
2979 + return children;
2980 + });
2981 +
2982 + function Foo() {
2983 + // To test the part that relies on context propagation,
2984 + // we need to bailout *above* the Suspense's parent.
2985 + // Several layers of Bailout wrappers help verify we're
2986 + // marking updates all the way to the propagation root.
2987 + return (
2988 + <SuspenseList revealOrder="forwards">
2989 + <Bailout>
2990 + <Bailout>
2991 + <Bailout>
2992 + <Bailout>
2993 + <Suspense fallback={<Text text="Loading A" />}>
2994 + <A />
2995 + </Suspense>
2996 + </Bailout>
2997 + </Bailout>
2998 + </Bailout>
2999 + </Bailout>
3000 + <Bailout>
3001 + <Bailout>
3002 + <Bailout>
3003 + <Bailout>
3004 + <Suspense fallback={<Text text="Loading B" />}>
3005 + <B />
3006 + </Suspense>
3007 + </Bailout>
3008 + </Bailout>
3009 + </Bailout>
3010 + </Bailout>
3011 + <Bailout>
3012 + <Bailout>
3013 + <Bailout>
3014 + <Bailout>
3015 + <Suspense fallback={<Text text="Loading C" />}>
3016 + <C />
3017 + </Suspense>
3018 + </Bailout>
3019 + </Bailout>
3020 + </Bailout>
3021 + </Bailout>
3022 + </SuspenseList>
3023 + );
3024 + }
3025 +
3026 + await C.resolve();
3027 +
3028 + ReactNoop.render(<Foo />);
3029 +
3030 + expect(Scheduler).toFlushAndYield([
3031 + 'Suspend! [A]',
3032 + 'Loading A',
3033 + 'Loading B',
3034 + 'Loading C',
3035 + ]);
3036 +
3037 + expect(ReactNoop).toMatchRenderedOutput(
3038 + <>
3039 + <span>Loading A</span>
3040 + <span>Loading B</span>
3041 + <span>Loading C</span>
3042 + </>,
3043 + );
3044 +
3045 + await A.resolve();
3046 +
3047 + expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]']);
3048 +
3049 + expect(ReactNoop).toMatchRenderedOutput(
3050 + <>
3051 + <span>A</span>
3052 + <span>Loading B</span>
3053 + <span>Loading C</span>
3054 + </>,
3055 + );
3056 +
3057 + await B.resolve();
3058 +
3059 + expect(Scheduler).toFlushAndYield(['B', 'C']);
3060 +
3061 + expect(ReactNoop).toMatchRenderedOutput(
3062 + <>
3063 + <span>A</span>
3064 + <span>B</span>
3065 + <span>C</span>
3066 + </>,
3067 + );
3068 + });
3069 });