Skip to content
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

Jump to bottom

Fix context propagation for offscreen/fallback trees #23095

Merged
merged 7 commits into from
Jan 19, 2022
Merged

Fix context propagation for offscreen/fallback trees #23095

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d6dc202
Failing test for Context.Consumer in suspended Suspense
overlookmotel Aug 26, 2020
0ce097b
Fix context propagation for offscreen trees
gaearon Jan 11, 2022
92c5c04
Address nits
gaearon Jan 18, 2022
7d93066
Specify propagation root for Suspense too
gaearon Jan 18, 2022
09a1975
Pass correct propagation root
gaearon Jan 19, 2022
1fed15d
Harden test coverage
gaearon Jan 19, 2022
d465a97
Remove superfluous warning
gaearon Jan 19, 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
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ import {
checkIfContextChanged,
readContext,
prepareToReadContext,
scheduleWorkOnParentPath,
scheduleContextWorkOnParentPath,
} from './ReactFiberNewContext.new';
import {
renderWithHooks,
Expand Down Expand Up @@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
}
}

function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
function scheduleSuspenseWorkOnFiber(
fiber: Fiber,
renderLanes: Lanes,
propagationRoot: Fiber,
) {
fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(fiber.return, renderLanes);
scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
}

function propagateSuspenseContextChange(
Expand All @@ -2776,15 +2780,15 @@ function propagateSuspenseContextChange(
if (node.tag === SuspenseComponent) {
const state: SuspenseState | null = node.memoizedState;
if (state !== null) {
scheduleWorkOnFiber(node, renderLanes);
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
}
} else if (node.tag === SuspenseListComponent) {
// If the tail is hidden there might not be an Suspense boundaries
// to schedule work on. In this case we have to schedule it on the
// list itself.
// We don't have to traverse to the children of the list since
// the list will propagate the change when it rerenders.
scheduleWorkOnFiber(node, renderLanes);
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
} else if (node.child !== null) {
node.child.return = node;
node = node.child;
Expand Down
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ import {
checkIfContextChanged,
readContext,
prepareToReadContext,
scheduleWorkOnParentPath,
scheduleContextWorkOnParentPath,
} from './ReactFiberNewContext.old';
import {
renderWithHooks,
Expand Down Expand Up @@ -2754,13 +2754,17 @@ function updateDehydratedSuspenseComponent(
}
}

function scheduleWorkOnFiber(fiber: Fiber, renderLanes: Lanes) {
function scheduleSuspenseWorkOnFiber(
fiber: Fiber,
renderLanes: Lanes,
propagationRoot: Fiber,
) {
fiber.lanes = mergeLanes(fiber.lanes, renderLanes);
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(fiber.return, renderLanes);
scheduleContextWorkOnParentPath(fiber.return, renderLanes, propagationRoot);
}

function propagateSuspenseContextChange(
Expand All @@ -2776,15 +2780,15 @@ function propagateSuspenseContextChange(
if (node.tag === SuspenseComponent) {
const state: SuspenseState | null = node.memoizedState;
if (state !== null) {
scheduleWorkOnFiber(node, renderLanes);
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
}
} else if (node.tag === SuspenseListComponent) {
// If the tail is hidden there might not be an Suspense boundaries
// to schedule work on. In this case we have to schedule it on the
// list itself.
// We don't have to traverse to the children of the list since
// the list will propagate the change when it rerenders.
scheduleWorkOnFiber(node, renderLanes);
scheduleSuspenseWorkOnFiber(node, renderLanes, workInProgress);
} else if (node.child !== null) {
node.child.return = node;
node = node.child;
Expand Down
51 changes: 45 additions & 6 deletions packages/react-reconciler/src/ReactFiberNewContext.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,10 @@ export function popProvider(
}
}

export function scheduleWorkOnParentPath(
export function scheduleContextWorkOnParentPath(
parent: Fiber | null,
renderLanes: Lanes,
propagationRoot: Fiber,
) {
// Update the child lanes of all the ancestors, including the alternates.
let node = parent;
Expand All @@ -157,12 +158,34 @@ export function scheduleWorkOnParentPath(
) {
alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
} else {
// Neither alternate was updated, which means the rest of the
// Neither alternate was updated.
// Normally, this would mean that the rest of the
// ancestor path already has sufficient priority.
// However, this is not necessarily true inside offscreen
// or fallback trees because childLanes may be inconsistent
// with the surroundings. This is why we continue the loop.
}
if (node === propagationRoot) {
break;
}
if (__DEV__) {
if (node === propagationRoot.alternate) {
gaearon marked this conversation as resolved.
Show resolved Hide resolved
console.error(
'Did not expect to encounter a propagation root alternate when scheduling context work. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
}
}
node = node.return;
}
if (__DEV__) {
if (node !== propagationRoot) {
console.error(
'Expected to find the propagation root when scheduling context work. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
}
}
}

export function propagateContextChange<T>(
Expand Down Expand Up @@ -246,7 +269,11 @@ function propagateContextChange_eager<T>(
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(fiber.return, renderLanes);
scheduleContextWorkOnParentPath(
fiber.return,
renderLanes,
workInProgress,
);

// Mark the updated lanes on the list, too.
list.lanes = mergeLanes(list.lanes, renderLanes);
Expand Down Expand Up @@ -284,7 +311,11 @@ function propagateContextChange_eager<T>(
// because we want to schedule this fiber as having work
// on its children. We'll use the childLanes on
// this fiber to indicate that a context has changed.
scheduleWorkOnParentPath(parentSuspense, renderLanes);
scheduleContextWorkOnParentPath(
parentSuspense,
renderLanes,
workInProgress,
);
nextFiber = fiber.sibling;
} else {
// Traverse down.
Expand Down Expand Up @@ -365,7 +396,11 @@ function propagateContextChanges<T>(
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(consumer.return, renderLanes);
scheduleContextWorkOnParentPath(
consumer.return,
renderLanes,
workInProgress,
);

if (!forcePropagateEntireTree) {
// During lazy propagation, when we find a match, we can defer
Expand Down Expand Up @@ -406,7 +441,11 @@ function propagateContextChanges<T>(
// because we want to schedule this fiber as having work
// on its children. We'll use the childLanes on
// this fiber to indicate that a context has changed.
scheduleWorkOnParentPath(parentSuspense, renderLanes);
scheduleContextWorkOnParentPath(
parentSuspense,
renderLanes,
workInProgress,
);
nextFiber = null;
} else {
// Traverse down.
Expand Down
51 changes: 45 additions & 6 deletions packages/react-reconciler/src/ReactFiberNewContext.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,10 @@ export function popProvider(
}
}

export function scheduleWorkOnParentPath(
export function scheduleContextWorkOnParentPath(
parent: Fiber | null,
renderLanes: Lanes,
propagationRoot: Fiber,
) {
// Update the child lanes of all the ancestors, including the alternates.
let node = parent;
Expand All @@ -157,12 +158,34 @@ export function scheduleWorkOnParentPath(
) {
alternate.childLanes = mergeLanes(alternate.childLanes, renderLanes);
} else {
// Neither alternate was updated, which means the rest of the
// Neither alternate was updated.
// Normally, this would mean that the rest of the
// ancestor path already has sufficient priority.
// However, this is not necessarily true inside offscreen
// or fallback trees because childLanes may be inconsistent
// with the surroundings. This is why we continue the loop.
}
if (node === propagationRoot) {
break;
}
if (__DEV__) {
if (node === propagationRoot.alternate) {
console.error(
'Did not expect to encounter a propagation root alternate when scheduling context work. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
}
}
node = node.return;
}
if (__DEV__) {
if (node !== propagationRoot) {
console.error(
'Expected to find the propagation root when scheduling context work. ' +
'This error is likely caused by a bug in React. Please file an issue.',
);
}
}
}

export function propagateContextChange<T>(
Expand Down Expand Up @@ -246,7 +269,11 @@ function propagateContextChange_eager<T>(
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(fiber.return, renderLanes);
scheduleContextWorkOnParentPath(
fiber.return,
renderLanes,
workInProgress,
);

// Mark the updated lanes on the list, too.
list.lanes = mergeLanes(list.lanes, renderLanes);
Expand Down Expand Up @@ -284,7 +311,11 @@ function propagateContextChange_eager<T>(
// because we want to schedule this fiber as having work
// on its children. We'll use the childLanes on
// this fiber to indicate that a context has changed.
scheduleWorkOnParentPath(parentSuspense, renderLanes);
scheduleContextWorkOnParentPath(
parentSuspense,
renderLanes,
workInProgress,
);
nextFiber = fiber.sibling;
} else {
// Traverse down.
Expand Down Expand Up @@ -365,7 +396,11 @@ function propagateContextChanges<T>(
if (alternate !== null) {
alternate.lanes = mergeLanes(alternate.lanes, renderLanes);
}
scheduleWorkOnParentPath(consumer.return, renderLanes);
scheduleContextWorkOnParentPath(
consumer.return,
renderLanes,
workInProgress,
);

if (!forcePropagateEntireTree) {
// During lazy propagation, when we find a match, we can defer
Expand Down Expand Up @@ -406,7 +441,11 @@ function propagateContextChanges<T>(
// because we want to schedule this fiber as having work
// on its children. We'll use the childLanes on
// this fiber to indicate that a context has changed.
scheduleWorkOnParentPath(parentSuspense, renderLanes);
scheduleContextWorkOnParentPath(
parentSuspense,
renderLanes,
workInProgress,
);
nextFiber = null;
} else {
// Traverse down.
Expand Down
63 changes: 63 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,5 +1532,68 @@ describe('ReactSuspense', () => {
expect(Scheduler).toFlushUntilNextPaint(['new value']);
expect(root).toMatchRenderedOutput('new value');
});

it('updates context consumer within child of suspended suspense component when context updates', () => {
const {createContext, useState} = React;

const ValueContext = createContext(null);

const promiseThatNeverResolves = new Promise(() => {});
function Child() {
return (
<ValueContext.Consumer>
{value => {
Scheduler.unstable_yieldValue(
`Received context value [${value}]`,
);
if (value === 'default') return <Text text="default" />;
throw promiseThatNeverResolves;
}}
</ValueContext.Consumer>
);
}

let setValue;
function Wrapper({children}) {
const [value, _setValue] = useState('default');
setValue = _setValue;
return (
<ValueContext.Provider value={value}>
{children}
</ValueContext.Provider>
);
}

function App() {
return (
<Wrapper>
<Suspense fallback={<Text text="Loading..." />}>
<Child />
</Suspense>
</Wrapper>
);
}

const root = ReactTestRenderer.create(<App />);
expect(Scheduler).toHaveYielded([
'Received context value [default]',
'default',
]);
expect(root).toMatchRenderedOutput('default');

act(() => setValue('new value'));
expect(Scheduler).toHaveYielded([
'Received context value [new value]',
'Loading...',
]);
expect(root).toMatchRenderedOutput('Loading...');

act(() => setValue('default'));
expect(Scheduler).toHaveYielded([
'Received context value [default]',
'default',
]);
expect(root).toMatchRenderedOutput('default');
});
});
});
Loading