Replace flushDiscreteUpdates with flushSync (#21775)
* Replace flushDiscreteUpdates with flushSync flushDiscreteUpdates is almost the same as flushSync. It forces passive effects to fire, because of an outdated heuristic, which isn't ideal but not that important. Besides that, the only remaining difference between flushDiscreteUpdates and flushSync is that flushDiscreteUpdates does not warn if you call it from inside an effect/lifecycle. This is because it might get triggered by a nested event dispatch, like `el.focus()`. So I added a new method, flushSyncWithWarningIfAlreadyRendering, which is used for the public flushSync API. It includes the warning. And I removed the warning from flushSync, so the event system can call that one. In production, flushSyncWithWarningIfAlreadyRendering gets inlined to flushSync, so the behavior is identical. Another way of thinking about this PR is that I renamed flushSync to flushSyncWithWarningIfAlreadyRendering and flushDiscreteUpdates to flushSync (and fixed the passive effects thing). The point is to prevent these from subtly diverging in the future. * Invert so the one with the warning is the default one To make Seb happy
Andrew Clark committed
Jul 1, 2021 at 18:13 UTC
32eefcb3c5131f4d77a2195ff11a00a2513cf62f
11 files changed
+73
-99
packages/react-dom/src/__tests__/ReactDOMFiber-test.js
+1
-7
@@ -1154,19 +1154,13 @@ describe('ReactDOMFiber', () => {
1154
expect(ops).toEqual(['A']);
1155
1156
if (__DEV__) {
1157
- const errorCalls = console.error.calls.count();
1157
+ expect(console.error.calls.count()).toBe(2);
1158
expect(console.error.calls.argsFor(0)[0]).toMatch(
1159
'ReactDOM.render is no longer supported in React 18',
1160
);
1161
expect(console.error.calls.argsFor(1)[0]).toMatch(
1162
'ReactDOM.render is no longer supported in React 18',
1163
);
1164
- // TODO: this warning shouldn't be firing in the first place if user didn't call it.
1165
- for (let i = 2; i < errorCalls; i++) {
1166
- expect(console.error.calls.argsFor(i)[0]).toMatch(
1167
- 'unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering.',
1168
- );
1169
- }
1164
}
1165
});
1166
packages/react-dom/src/client/ReactDOM.js
+2
-2
@@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
23
import {
24
batchedUpdates,
25
discreteUpdates,
26
- flushDiscreteUpdates,
26
flushSync,
27
+ flushSyncWithoutWarningIfAlreadyRendering,
28
flushControlled,
29
injectIntoDevTools,
30
attemptSynchronousHydration,
@@ -100,7 +100,7 @@ setRestoreImplementation(restoreControlledState);
100
setBatchingImplementation(
101
batchedUpdates,
102
discreteUpdates,
103
- flushDiscreteUpdates,
103
+ flushSyncWithoutWarningIfAlreadyRendering,
104
);
105
106
function createPortal(
packages/react-dom/src/events/ReactDOMUpdateBatching.js
+4
-4
@@ -23,7 +23,7 @@ let batchedUpdatesImpl = function(fn, bookkeeping) {
23
let discreteUpdatesImpl = function(fn, a, b, c, d) {
24
return fn(a, b, c, d);
25
};
26
-let flushDiscreteUpdatesImpl = function() {};
26
+let flushSyncImpl = function() {};
27
28
let isInsideEventHandler = false;
29
@@ -39,7 +39,7 @@ function finishEventHandler() {
39
// bails out of the update without touching the DOM.
40
// TODO: Restore state in the microtask, after the discrete updates flush,
41
// instead of early flushing them here.
42
- flushDiscreteUpdatesImpl();
42
+ flushSyncImpl();
43
restoreStateIfNeeded();
44
}
45
}
@@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) {
67
export function setBatchingImplementation(
68
_batchedUpdatesImpl,
69
_discreteUpdatesImpl,
70
- _flushDiscreteUpdatesImpl,
70
+ _flushSyncImpl,
71
) {
72
batchedUpdatesImpl = _batchedUpdatesImpl;
73
discreteUpdatesImpl = _discreteUpdatesImpl;
74
- flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
74
+ flushSyncImpl = _flushSyncImpl;
75
}
packages/react-noop-renderer/src/ReactNoop.js
-1
@@ -41,7 +41,6 @@ export const {
41
unbatchedUpdates,
42
discreteUpdates,
43
idleUpdates,
44
- flushDiscreteUpdates,
44
flushSync,
45
flushPassiveEffects,
46
act,
packages/react-noop-renderer/src/createReactNoop.js
-2
@@ -915,8 +915,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
915
}
916
},
917
918
- flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,
919
-
918
flushSync(fn: () => mixed) {
919
NoopRenderer.flushSync(fn);
920
},
packages/react-reconciler/src/ReactFiberReconciler.js
+5
-5
@@ -21,9 +21,9 @@ import {
21
unbatchedUpdates as unbatchedUpdates_old,
22
deferredUpdates as deferredUpdates_old,
23
discreteUpdates as discreteUpdates_old,
24
- flushDiscreteUpdates as flushDiscreteUpdates_old,
24
flushControlled as flushControlled_old,
25
flushSync as flushSync_old,
26
+ flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_old,
27
flushPassiveEffects as flushPassiveEffects_old,
28
getPublicRootInstance as getPublicRootInstance_old,
29
attemptSynchronousHydration as attemptSynchronousHydration_old,
@@ -59,9 +59,9 @@ import {
59
unbatchedUpdates as unbatchedUpdates_new,
60
deferredUpdates as deferredUpdates_new,
61
discreteUpdates as discreteUpdates_new,
62
- flushDiscreteUpdates as flushDiscreteUpdates_new,
62
flushControlled as flushControlled_new,
63
flushSync as flushSync_new,
64
+ flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_new,
65
flushPassiveEffects as flushPassiveEffects_new,
66
getPublicRootInstance as getPublicRootInstance_new,
67
attemptSynchronousHydration as attemptSynchronousHydration_new,
@@ -108,13 +108,13 @@ export const deferredUpdates = enableNewReconciler
108
export const discreteUpdates = enableNewReconciler
109
? discreteUpdates_new
110
: discreteUpdates_old;
111
-export const flushDiscreteUpdates = enableNewReconciler
112
- ? flushDiscreteUpdates_new
113
- : flushDiscreteUpdates_old;
111
export const flushControlled = enableNewReconciler
112
? flushControlled_new
113
: flushControlled_old;
114
export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old;
115
+export const flushSyncWithoutWarningIfAlreadyRendering = enableNewReconciler
116
+ ? flushSyncWithoutWarningIfAlreadyRendering_new
117
+ : flushSyncWithoutWarningIfAlreadyRendering_old;
118
export const flushPassiveEffects = enableNewReconciler
119
? flushPassiveEffects_new
120
: flushPassiveEffects_old;
packages/react-reconciler/src/ReactFiberReconciler.new.js
+2
-2
@@ -57,7 +57,7 @@ import {
57
flushControlled,
58
deferredUpdates,
59
discreteUpdates,
60
- flushDiscreteUpdates,
60
+ flushSyncWithoutWarningIfAlreadyRendering,
61
flushPassiveEffects,
62
} from './ReactFiberWorkLoop.new';
63
import {
@@ -330,9 +330,9 @@ export {
330
unbatchedUpdates,
331
deferredUpdates,
332
discreteUpdates,
333
- flushDiscreteUpdates,
333
flushControlled,
334
flushSync,
335
+ flushSyncWithoutWarningIfAlreadyRendering,
336
flushPassiveEffects,
337
};
338
packages/react-reconciler/src/ReactFiberReconciler.old.js
+2
-2
@@ -57,7 +57,7 @@ import {
57
flushControlled,
58
deferredUpdates,
59
discreteUpdates,
60
- flushDiscreteUpdates,
60
+ flushSyncWithoutWarningIfAlreadyRendering,
61
flushPassiveEffects,
62
} from './ReactFiberWorkLoop.old';
63
import {
@@ -330,9 +330,9 @@ export {
330
unbatchedUpdates,
331
deferredUpdates,
332
discreteUpdates,
333
- flushDiscreteUpdates,
333
flushControlled,
334
flushSync,
335
+ flushSyncWithoutWarningIfAlreadyRendering,
336
flushPassiveEffects,
337
};
338
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+17
-37
@@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
1044
return executionContext;
1045
}
1046
1047
-export function flushDiscreteUpdates() {
1048
- // TODO: Should be able to flush inside batchedUpdates, but not inside `act`.
1049
- // However, `act` uses `batchedUpdates`, so there's no way to distinguish
1050
- // those two cases. Need to fix this before exposing flushDiscreteUpdates
1051
- // as a public API.
1052
- if (
1053
- (executionContext & (BatchedContext | RenderContext | CommitContext)) !==
1054
- NoContext
1055
- ) {
1056
- if (__DEV__) {
1057
- if ((executionContext & RenderContext) !== NoContext) {
1058
- console.error(
1059
- 'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' +
1060
- 'already rendering.',
1061
- );
1062
- }
1063
- }
1064
- // We're already rendering, so we can't synchronously flush pending work.
1065
- // This is probably a nested event dispatch triggered by a lifecycle/effect,
1066
- // like `el.focus()`. Exit.
1067
- return;
1068
- }
1069
- flushSyncCallbacks();
1070
- // If the discrete updates scheduled passive effects, flush them now so that
1071
- // they fire before the next serial event.
1072
- flushPassiveEffects();
1073
-}
1074
-
1047
export function deferredUpdates<A>(fn: () => A): A {
1048
const previousPriority = getCurrentUpdatePriority();
1049
const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1142,7 +1114,10 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
1114
}
1115
}
1116
1145
-export function flushSync<A, R>(fn: A => R, a: A): R {
1117
+export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
1118
+ fn: A => R,
1119
+ a: A,
1120
+): R {
1121
const prevExecutionContext = executionContext;
1122
executionContext |= BatchedContext;
1123
@@ -1165,18 +1140,23 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
1140
// the stack.
1141
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
1142
flushSyncCallbacks();
1168
- } else {
1169
- if (__DEV__) {
1170
- console.error(
1171
- 'flushSync was called from inside a lifecycle method. React cannot ' +
1172
- 'flush when React is already rendering. Consider moving this call to ' +
1173
- 'a scheduler task or micro task.',
1174
- );
1175
- }
1143
}
1144
}
1145
}
1146
1147
+export function flushSync<A, R>(fn: A => R, a: A): R {
1148
+ if (__DEV__) {
1149
+ if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
1150
+ console.error(
1151
+ 'flushSync was called from inside a lifecycle method. React cannot ' +
1152
+ 'flush when React is already rendering. Consider moving this call to ' +
1153
+ 'a scheduler task or micro task.',
1154
+ );
1155
+ }
1156
+ }
1157
+ return flushSyncWithoutWarningIfAlreadyRendering(fn, a);
1158
+}
1159
+
1160
export function flushControlled(fn: () => mixed): void {
1161
const prevExecutionContext = executionContext;
1162
executionContext |= BatchedContext;
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+17
-37
@@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
1044
return executionContext;
1045
}
1046
1047
-export function flushDiscreteUpdates() {
1048
- // TODO: Should be able to flush inside batchedUpdates, but not inside `act`.
1049
- // However, `act` uses `batchedUpdates`, so there's no way to distinguish
1050
- // those two cases. Need to fix this before exposing flushDiscreteUpdates
1051
- // as a public API.
1052
- if (
1053
- (executionContext & (BatchedContext | RenderContext | CommitContext)) !==
1054
- NoContext
1055
- ) {
1056
- if (__DEV__) {
1057
- if ((executionContext & RenderContext) !== NoContext) {
1058
- console.error(
1059
- 'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' +
1060
- 'already rendering.',
1061
- );
1062
- }
1063
- }
1064
- // We're already rendering, so we can't synchronously flush pending work.
1065
- // This is probably a nested event dispatch triggered by a lifecycle/effect,
1066
- // like `el.focus()`. Exit.
1067
- return;
1068
- }
1069
- flushSyncCallbacks();
1070
- // If the discrete updates scheduled passive effects, flush them now so that
1071
- // they fire before the next serial event.
1072
- flushPassiveEffects();
1073
-}
1074
-
1047
export function deferredUpdates<A>(fn: () => A): A {
1048
const previousPriority = getCurrentUpdatePriority();
1049
const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1142,7 +1114,10 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
1114
}
1115
}
1116
1145
-export function flushSync<A, R>(fn: A => R, a: A): R {
1117
+export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
1118
+ fn: A => R,
1119
+ a: A,
1120
+): R {
1121
const prevExecutionContext = executionContext;
1122
executionContext |= BatchedContext;
1123
@@ -1165,18 +1140,23 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
1140
// the stack.
1141
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
1142
flushSyncCallbacks();
1168
- } else {
1169
- if (__DEV__) {
1170
- console.error(
1171
- 'flushSync was called from inside a lifecycle method. React cannot ' +
1172
- 'flush when React is already rendering. Consider moving this call to ' +
1173
- 'a scheduler task or micro task.',
1174
- );
1175
- }
1143
}
1144
}
1145
}
1146
1147
+export function flushSync<A, R>(fn: A => R, a: A): R {
1148
+ if (__DEV__) {
1149
+ if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
1150
+ console.error(
1151
+ 'flushSync was called from inside a lifecycle method. React cannot ' +
1152
+ 'flush when React is already rendering. Consider moving this call to ' +
1153
+ 'a scheduler task or micro task.',
1154
+ );
1155
+ }
1156
+ }
1157
+ return flushSyncWithoutWarningIfAlreadyRendering(fn, a);
1158
+}
1159
+
1160
export function flushControlled(fn: () => mixed): void {
1161
const prevExecutionContext = executionContext;
1162
executionContext |= BatchedContext;
packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
+23
@@ -173,4 +173,27 @@ describe('ReactFlushSync', () => {
173
// Effect flushes after paint.
174
expect(Scheduler).toHaveYielded(['Effect']);
175
});
176
+
177
+ test('does not flush pending passive effects', async () => {
178
+ function App() {
179
+ useEffect(() => {
180
+ Scheduler.unstable_yieldValue('Effect');
181
+ }, []);
182
+ return <Text text="Child" />;
183
+ }
184
+
185
+ const root = ReactNoop.createRoot();
186
+ await act(async () => {
187
+ root.render(<App />);
188
+ expect(Scheduler).toFlushUntilNextPaint(['Child']);
189
+ expect(root).toMatchRenderedOutput('Child');
190
+
191
+ // Passive effects are pending. Calling flushSync should not affect them.
192
+ ReactNoop.flushSync();
193
+ // Effects still haven't fired.
194
+ expect(Scheduler).toHaveYielded([]);
195
+ });
196
+ // Now the effects have fired.
197
+ expect(Scheduler).toHaveYielded(['Effect']);
198
+ });
199
});