Re-land "Flush discrete passive effects before paint (#21150)"
This re-lands commit 2e7aceeb5c8b6e5c61174c0e9731e263e956e445.
Andrew Clark committed
May 3, 2021 at 13:34 UTC
bacc87068a5818b597f5cd9938ce0ce205cab53b
4 files changed
+104
-2
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+15
@@ -1976,6 +1976,21 @@ function commitRootImpl(root, renderPriorityLevel) {
1976
return null;
1977
}
1978
1979
+ // If the passive effects are the result of a discrete render, flush them
1980
+ // synchronously at the end of the current task so that the result is
1981
+ // immediately observable. Otherwise, we assume that they are not
1982
+ // order-dependent and do not need to be observed by external systems, so we
1983
+ // can wait until after paint.
1984
+ // TODO: We can optimize this by not scheduling the callback earlier. Since we
1985
+ // currently schedule the callback in multiple places, will wait until those
1986
+ // are consolidated.
1987
+ if (
1988
+ includesSomeLane(pendingPassiveEffectsLanes, SyncLane) &&
1989
+ root.tag !== LegacyRoot
1990
+ ) {
1991
+ flushPassiveEffects();
1992
+ }
1993
+
1994
// If layout work was scheduled, flush it now.
1995
flushSyncCallbacks();
1996
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+15
@@ -1976,6 +1976,21 @@ function commitRootImpl(root, renderPriorityLevel) {
1976
return null;
1977
}
1978
1979
+ // If the passive effects are the result of a discrete render, flush them
1980
+ // synchronously at the end of the current task so that the result is
1981
+ // immediately observable. Otherwise, we assume that they are not
1982
+ // order-dependent and do not need to be observed by external systems, so we
1983
+ // can wait until after paint.
1984
+ // TODO: We can optimize this by not scheduling the callback earlier. Since we
1985
+ // currently schedule the callback in multiple places, will wait until those
1986
+ // are consolidated.
1987
+ if (
1988
+ includesSomeLane(pendingPassiveEffectsLanes, SyncLane) &&
1989
+ root.tag !== LegacyRoot
1990
+ ) {
1991
+ flushPassiveEffects();
1992
+ }
1993
+
1994
// If layout work was scheduled, flush it now.
1995
flushSyncCallbacks();
1996
packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
+69
@@ -61,4 +61,73 @@ describe('ReactFlushSync', () => {
61
});
62
expect(root).toMatchRenderedOutput('1, 1');
63
});
64
+
65
+ test('flushes passive effects synchronously when they are the result of a sync render', async () => {
66
+ function App() {
67
+ useEffect(() => {
68
+ Scheduler.unstable_yieldValue('Effect');
69
+ }, []);
70
+ return <Text text="Child" />;
71
+ }
72
+
73
+ const root = ReactNoop.createRoot();
74
+ await ReactNoop.act(async () => {
75
+ ReactNoop.flushSync(() => {
76
+ root.render(<App />);
77
+ });
78
+ expect(Scheduler).toHaveYielded([
79
+ 'Child',
80
+ // Because the pending effect was the result of a sync update, calling
81
+ // flushSync should flush it.
82
+ 'Effect',
83
+ ]);
84
+ expect(root).toMatchRenderedOutput('Child');
85
+ });
86
+ });
87
+
88
+ test('do not flush passive effects synchronously in legacy mode', async () => {
89
+ function App() {
90
+ useEffect(() => {
91
+ Scheduler.unstable_yieldValue('Effect');
92
+ }, []);
93
+ return <Text text="Child" />;
94
+ }
95
+
96
+ const root = ReactNoop.createLegacyRoot();
97
+ await ReactNoop.act(async () => {
98
+ ReactNoop.flushSync(() => {
99
+ root.render(<App />);
100
+ });
101
+ expect(Scheduler).toHaveYielded([
102
+ 'Child',
103
+ // Because we're in legacy mode, we shouldn't have flushed the passive
104
+ // effects yet.
105
+ ]);
106
+ expect(root).toMatchRenderedOutput('Child');
107
+ });
108
+ // Effect flushes after paint.
109
+ expect(Scheduler).toHaveYielded(['Effect']);
110
+ });
111
+
112
+ test("do not flush passive effects synchronously when they aren't the result of a sync render", async () => {
113
+ function App() {
114
+ useEffect(() => {
115
+ Scheduler.unstable_yieldValue('Effect');
116
+ }, []);
117
+ return <Text text="Child" />;
118
+ }
119
+
120
+ const root = ReactNoop.createRoot();
121
+ await ReactNoop.act(async () => {
122
+ root.render(<App />);
123
+ expect(Scheduler).toFlushUntilNextPaint([
124
+ 'Child',
125
+ // Because the passive effect was not the result of a sync update, it
126
+ // should not flush before paint.
127
+ ]);
128
+ expect(root).toMatchRenderedOutput('Child');
129
+ });
130
+ // Effect flushes after paint.
131
+ expect(Scheduler).toHaveYielded(['Effect']);
132
+ });
133
});
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+5
-2
@@ -32,6 +32,7 @@ let useDeferredValue;
32
let forwardRef;
33
let memo;
34
let act;
35
+let ContinuousEventPriority;
36
37
describe('ReactHooksWithNoopRenderer', () => {
38
beforeEach(() => {
@@ -55,6 +56,8 @@ describe('ReactHooksWithNoopRenderer', () => {
56
useDeferredValue = React.unstable_useDeferredValue;
57
Suspense = React.Suspense;
58
act = ReactNoop.act;
59
+ ContinuousEventPriority = require('react-reconciler/constants')
60
+ .ContinuousEventPriority;
61
62
textCache = new Map();
63
@@ -1397,10 +1400,10 @@ describe('ReactHooksWithNoopRenderer', () => {
1400
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);
1401
1402
// Schedule unmount for the parent that unmounts children with pending update.
1400
- ReactNoop.flushSync(() => {
1403
+ ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
1404
setParentState(false);
1405
});
1403
- expect(Scheduler).toHaveYielded([
1406
+ expect(Scheduler).toFlushUntilNextPaint([
1407
'Parent false render',
1408
'Parent false commit',
1409
]);