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.

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

Flush discrete passive effects before paint #21150

Merged
merged 1 commit into from
Mar 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -2015,6 +2015,21 @@ function commitRootImpl(root, renderPriorityLevel) {
return null;
}

// If the passive effects are the result of a discrete render, flush them
// synchronously at the end of the current task so that the result is
// immediately observable. Otherwise, we assume that they are not
// order-dependent and do not need to be observed by external systems, so we
// can wait until after paint.
// TODO: We can optimize this by not scheduling the callback earlier. Since we
// currently schedule the callback in multiple places, will wait until those
// are consolidated.
if (
includesSomeLane(pendingPassiveEffectsLanes, SyncLane) &&
root.tag !== LegacyRoot
) {
flushPassiveEffects();
}

// If layout work was scheduled, flush it now.
flushSyncCallbackQueue();

Expand Down
15 changes: 15 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -2015,6 +2015,21 @@ function commitRootImpl(root, renderPriorityLevel) {
return null;
}

// If the passive effects are the result of a discrete render, flush them
// synchronously at the end of the current task so that the result is
// immediately observable. Otherwise, we assume that they are not
// order-dependent and do not need to be observed by external systems, so we
// can wait until after paint.
// TODO: We can optimize this by not scheduling the callback earlier. Since we
// currently schedule the callback in multiple places, will wait until those
// are consolidated.
if (
includesSomeLane(pendingPassiveEffectsLanes, SyncLane) &&
root.tag !== LegacyRoot
) {
flushPassiveEffects();
}

// If layout work was scheduled, flush it now.
flushSyncCallbackQueue();

Expand Down
69 changes: 69 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,73 @@ describe('ReactFlushSync', () => {
expect(Scheduler).toHaveYielded(['1, 1']);
expect(root).toMatchRenderedOutput('1, 1');
});

test('flushes passive effects synchronously when they are the result of a sync render', async () => {
function App() {
useEffect(() => {
Scheduler.unstable_yieldValue('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
ReactNoop.flushSync(() => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([
'Child',
// Because the pending effect was the result of a sync update, calling
// flushSync should flush it.
'Effect',
]);
expect(root).toMatchRenderedOutput('Child');
});
});

test('do not flush passive effects synchronously in legacy mode', async () => {
function App() {
useEffect(() => {
Scheduler.unstable_yieldValue('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createLegacyRoot();
await ReactNoop.act(async () => {
ReactNoop.flushSync(() => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([
'Child',
// Because we're in legacy mode, we shouldn't have flushed the passive
// effects yet.
]);
expect(root).toMatchRenderedOutput('Child');
});
// Effect flushes after paint.
expect(Scheduler).toHaveYielded(['Effect']);
});

test("do not flush passive effects synchronously when they aren't the result of a sync render", async () => {
function App() {
useEffect(() => {
Scheduler.unstable_yieldValue('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
expect(Scheduler).toFlushUntilNextPaint([
'Child',
// Because the passive effect was not the result of a sync update, it
// should not flush before paint.
]);
expect(root).toMatchRenderedOutput('Child');
});
// Effect flushes after paint.
expect(Scheduler).toHaveYielded(['Effect']);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ let useDeferredValue;
let forwardRef;
let memo;
let act;
let ContinuousEventPriority;

describe('ReactHooksWithNoopRenderer', () => {
beforeEach(() => {
Expand All @@ -57,6 +58,8 @@ describe('ReactHooksWithNoopRenderer', () => {
useDeferredValue = React.unstable_useDeferredValue;
Suspense = React.Suspense;
act = ReactNoop.act;
ContinuousEventPriority = require('react-reconciler/constants')
.ContinuousEventPriority;

textCache = new Map();

Expand Down Expand Up @@ -1351,10 +1354,10 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);

// Schedule unmount for the parent that unmounts children with pending update.
ReactNoop.flushSync(() => {
ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
setParentState(false);
});
expect(Scheduler).toHaveYielded([
expect(Scheduler).toFlushUntilNextPaint([
'Parent false render',
'Parent false commit',
]);
Expand Down
11 changes: 4 additions & 7 deletions packages/react/src/__tests__/ReactProfiler-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -3764,13 +3764,10 @@ describe('Profiler', () => {
);
});

// Profiler tag causes passive effects to be scheduled,
// so the interactions are still not completed.
expect(Scheduler).toHaveYielded(['SecondComponent']);
expect(onInteractionTraced).toHaveBeenCalledTimes(2);
expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled();
expect(Scheduler).toFlushAndYieldThrough(['onPostCommit']);

expect(Scheduler).toHaveYielded([
'SecondComponent',
'onPostCommit',
]);
expect(onInteractionTraced).toHaveBeenCalledTimes(2);
expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(
1,
Expand Down