Revert "Flush discrete passive effects before paint (#21150)"
This reverts commit 2e7aceeb5c8b6e5c61174c0e9731e263e956e445. If a discrete render results in passive effects, we should flush them synchronously at the end of the current task so that the result is immediately observable. For example, if a passive effect adds an event listener, the listener will be added before the next input. We don't need to do this for effects that don't have discrete/sync priority, because we assume they are not order-dependent and do not need to be observed by external systems. For legacy mode, we will maintain the existing behavior, since it hasn't been reported as an issue, and we'd have to do additional work to distinguish "legacy default sync" from "discrete sync" to prevent all passive effects from being treated this way.
Andrew Clark committed
Apr 23, 2021 at 11:46 UTC
7bac7607a764074e66dd57c68c09ec5cc898f4a5
4 files changed
+2
-104
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
-15
@@ -1976,21 +1976,6 @@ 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
-
1979
// If layout work was scheduled, flush it now.
1980
flushSyncCallbacks();
1981
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
-15
@@ -1976,21 +1976,6 @@ 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
-
1979
// If layout work was scheduled, flush it now.
1980
flushSyncCallbacks();
1981
packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
-69
@@ -61,73 +61,4 @@ 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
- });
64
});
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+2
-5
@@ -32,7 +32,6 @@ let useDeferredValue;
32
let forwardRef;
33
let memo;
34
let act;
35
-let ContinuousEventPriority;
35
36
describe('ReactHooksWithNoopRenderer', () => {
37
beforeEach(() => {
@@ -56,8 +55,6 @@ describe('ReactHooksWithNoopRenderer', () => {
55
useDeferredValue = React.unstable_useDeferredValue;
56
Suspense = React.Suspense;
57
act = ReactNoop.act;
59
- ContinuousEventPriority = require('react-reconciler/constants')
60
- .ContinuousEventPriority;
58
59
textCache = new Map();
60
@@ -1400,10 +1397,10 @@ describe('ReactHooksWithNoopRenderer', () => {
1397
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);
1398
1399
// Schedule unmount for the parent that unmounts children with pending update.
1403
- ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
1400
+ ReactNoop.flushSync(() => {
1401
setParentState(false);
1402
});
1406
- expect(Scheduler).toFlushUntilNextPaint([
1403
+ expect(Scheduler).toHaveYielded([
1404
'Parent false render',
1405
'Parent false commit',
1406
]);