Skip to content

Commit d8cfeaf

Browse files
gaearonoverlookmotel
gaearon
and
overlookmotel
authored
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 <[email protected]>
1 parent d504824 commit d8cfeaf

6 files changed

+253
-22
lines changed

Diff for: ‎packages/react-reconciler/src/ReactFiberBeginWork.new.js

+9-5
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ import {
174174
checkIfContextChanged,
175175
readContext,
176176
prepareToReadContext,
177-
scheduleWorkOnParentPath,
177+
scheduleContextWorkOnParentPath,
178178
} from './ReactFiberNewContext.new';
179179
import {
180180
renderWithHooks,
@@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
27542754
}
27552755
}
27562756

2757-
function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
2757+
function scheduleSuspenseWorkOnFiber(
2758+
fiber: Fiber,
2759+
renderLanes: Lanes,
2760+
propagationRoot: Fiber,
2761+
) {
27582762
fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
27592763
const alternate = fiber.alternate;
27602764
if (alternate !== null) {
27612765
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
27622766
}
2763-
scheduleWorkOnParentPath(fiber.return, renderLanes);
2767+
scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
27642768
}
27652769

27662770
function propagateSuspenseContextChange(
@@ -2776,15 +2780,15 @@ function propagateSuspenseContextChange(
27762780
if (node.tag === SuspenseComponent) {
27772781
const state: SuspenseState | null = node.memoizedState;
27782782
if (state !== null) {
2779-
scheduleWorkOnFiber(node, renderLanes);
2783+
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
27802784
}
27812785
} else if (node.tag === SuspenseListComponent) {
27822786
// If the tail is hidden there might not be an Suspense boundaries
27832787
// to schedule work on. In this case we have to schedule it on the
27842788
// list itself.
27852789
// We don't have to traverse to the children of the list since
27862790
// the list will propagate the change when it rerenders.
2787-
scheduleWorkOnFiber(node, renderLanes);
2791+
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
27882792
} else if (node.child !== null) {
27892793
node.child.return = node;
27902794
node = node.child;

Diff for: ‎packages/react-reconciler/src/ReactFiberBeginWork.old.js

+9-5
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ import {
174174
checkIfContextChanged,
175175
readContext,
176176
prepareToReadContext,
177-
scheduleWorkOnParentPath,
177+
scheduleContextWorkOnParentPath,
178178
} from './ReactFiberNewContext.old';
179179
import {
180180
renderWithHooks,
@@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
27542754
}
27552755
}
27562756

2757-
function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
2757+
function scheduleSuspenseWorkOnFiber(
2758+
fiber: Fiber,
2759+
renderLanes: Lanes,
2760+
propagationRoot: Fiber,
2761+
) {
27582762
fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
27592763
const alternate = fiber.alternate;
27602764
if (alternate !== null) {
27612765
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
27622766
}
2763-
scheduleWorkOnParentPath(fiber.return, renderLanes);
2767+
scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
27642768
}
27652769

27662770
function propagateSuspenseContextChange(
@@ -2776,15 +2780,15 @@ function propagateSuspenseContextChange(
27762780
if (node.tag === SuspenseComponent) {
27772781
const state: SuspenseState | null = node.memoizedState;
27782782
if (state !== null) {
2779-
scheduleWorkOnFiber(node, renderLanes);
2783+
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
27802784
}
27812785
} else if (node.tag === SuspenseListComponent) {
27822786
// If the tail is hidden there might not be an Suspense boundaries
27832787
// to schedule work on. In this case we have to schedule it on the
27842788
// list itself.
27852789
// We don't have to traverse to the children of the list since
27862790
// the list will propagate the change when it rerenders.
2787-
scheduleWorkOnFiber(node, renderLanes);
2791+
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
27882792
} else if (node.child !== null) {
27892793
node.child.return = node;
27902794
node = node.child;

Diff for: ‎packages/react-reconciler/src/ReactFiberNewContext.new.js

+37-6
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,10 @@ export function popProvider(
138138
}
139139
}
140140

141-
export function scheduleWorkOnParentPath(
141+
export function scheduleContextWorkOnParentPath(
142142
parent: Fiber | null,
143143
renderLanes: Lanes,
144+
propagationRoot: Fiber,
144145
) {
145146
// Update the child lanes of all the ancestors, including the alternates.
146147
let node = parent;
@@ -157,12 +158,26 @@ export function scheduleWorkOnParentPath(
157158
) {
158159
alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
159160
} 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
161163
// 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) {
162169
break;
163170
}
164171
node = node.return;
165172
}
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+
}
166181
}
167182

168183
export function propagateContextChange<T>(
@@ -246,7 +261,11 @@ function propagateContextChange_eager<T>(
246261
if (alternate !== null) {
247262
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
248263
}
249-
scheduleWorkOnParentPath(fiber.return, renderLanes);
264+
scheduleContextWorkOnParentPath(
265+
fiber.return,
266+
renderLanes,
267+
workInProgress,
268+
);
250269

251270
// Mark the updated lanes on the list, too.
252271
list.lanes = mergeLanes(list.lanes, renderLanes);
@@ -284,7 +303,11 @@ function propagateContextChange_eager<T>(
284303
// because we want to schedule this fiber as having work
285304
// on its children. We'll use the childLanes on
286305
// this fiber to indicate that a context has changed.
287-
scheduleWorkOnParentPath(parentSuspense, renderLanes);
306+
scheduleContextWorkOnParentPath(
307+
parentSuspense,
308+
renderLanes,
309+
workInProgress,
310+
);
288311
nextFiber = fiber.sibling;
289312
} else {
290313
// Traverse down.
@@ -365,7 +388,11 @@ function propagateContextChanges<T>(
365388
if (alternate !== null) {
366389
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
367390
}
368-
scheduleWorkOnParentPath(consumer.return, renderLanes);
391+
scheduleContextWorkOnParentPath(
392+
consumer.return,
393+
renderLanes,
394+
workInProgress,
395+
);
369396

370397
if (!forcePropagateEntireTree) {
371398
// During lazy propagation, when we find a match, we can defer
@@ -406,7 +433,11 @@ function propagateContextChanges<T>(
406433
// because we want to schedule this fiber as having work
407434
// on its children. We'll use the childLanes on
408435
// this fiber to indicate that a context has changed.
409-
scheduleWorkOnParentPath(parentSuspense, renderLanes);
436+
scheduleContextWorkOnParentPath(
437+
parentSuspense,
438+
renderLanes,
439+
workInProgress,
440+
);
410441
nextFiber = null;
411442
} else {
412443
// Traverse down.

Diff for: ‎packages/react-reconciler/src/ReactFiberNewContext.old.js

+37-6
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,10 @@ export function popProvider(
138138
}
139139
}
140140

141-
export function scheduleWorkOnParentPath(
141+
export function scheduleContextWorkOnParentPath(
142142
parent: Fiber | null,
143143
renderLanes: Lanes,
144+
propagationRoot: Fiber,
144145
) {
145146
// Update the child lanes of all the ancestors, including the alternates.
146147
let node = parent;
@@ -157,12 +158,26 @@ export function scheduleWorkOnParentPath(
157158
) {
158159
alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
159160
} 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
161163
// 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) {
162169
break;
163170
}
164171
node = node.return;
165172
}
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+
}
166181
}
167182

168183
export function propagateContextChange<T>(
@@ -246,7 +261,11 @@ function propagateContextChange_eager<T>(
246261
if (alternate !== null) {
247262
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
248263
}
249-
scheduleWorkOnParentPath(fiber.return, renderLanes);
264+
scheduleContextWorkOnParentPath(
265+
fiber.return,
266+
renderLanes,
267+
workInProgress,
268+
);
250269

251270
// Mark the updated lanes on the list, too.
252271
list.lanes = mergeLanes(list.lanes, renderLanes);
@@ -284,7 +303,11 @@ function propagateContextChange_eager<T>(
284303
// because we want to schedule this fiber as having work
285304
// on its children. We'll use the childLanes on
286305
// this fiber to indicate that a context has changed.
287-
scheduleWorkOnParentPath(parentSuspense, renderLanes);
306+
scheduleContextWorkOnParentPath(
307+
parentSuspense,
308+
renderLanes,
309+
workInProgress,
310+
);
288311
nextFiber = fiber.sibling;
289312
} else {
290313
// Traverse down.
@@ -365,7 +388,11 @@ function propagateContextChanges<T>(
365388
if (alternate !== null) {
366389
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
367390
}
368-
scheduleWorkOnParentPath(consumer.return, renderLanes);
391+
scheduleContextWorkOnParentPath(
392+
consumer.return,
393+
renderLanes,
394+
workInProgress,
395+
);
369396

370397
if (!forcePropagateEntireTree) {
371398
// During lazy propagation, when we find a match, we can defer
@@ -406,7 +433,11 @@ function propagateContextChanges<T>(
406433
// because we want to schedule this fiber as having work
407434
// on its children. We'll use the childLanes on
408435
// this fiber to indicate that a context has changed.
409-
scheduleWorkOnParentPath(parentSuspense, renderLanes);
436+
scheduleContextWorkOnParentPath(
437+
parentSuspense,
438+
renderLanes,
439+
workInProgress,
440+
);
410441
nextFiber = null;
411442
} else {
412443
// Traverse down.

Diff for: ‎packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js

+63
Original file line numberDiff line numberDiff line change
@@ -1532,5 +1532,68 @@ describe('ReactSuspense', () => {
15321532
expect(Scheduler).toFlushUntilNextPaint(['new value']);
15331533
expect(root).toMatchRenderedOutput('new value');
15341534
});
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+
});
15351598
});
15361599
});

0 commit comments

Comments
 (0)