Skip to content

Commit

Permalink
Re-land "Flush discrete passive effects before paint (#21150)"
Browse files Browse the repository at this point in the history
This re-lands commit 2e7acee.
  • Loading branch information
acdlite committed May 3, 2021
1 parent 098600c commit bacc870
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 2 deletions.
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 @@ -1976,6 +1976,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.
flushSyncCallbacks();

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 @@ -1976,6 +1976,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.
flushSyncCallbacks();

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 @@ -61,4 +61,73 @@ describe('ReactFlushSync', () => {
});
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 @@ -32,6 +32,7 @@ let useDeferredValue;
let forwardRef;
let memo;
let act;
let ContinuousEventPriority;

describe('ReactHooksWithNoopRenderer', () => {
beforeEach(() => {
Expand All @@ -55,6 +56,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 @@ -1397,10 +1400,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

0 comments on commit bacc870

Please sign in to comment.