Skip to content

Commit 8a27dbb

Browse files
acdliteacdlite
committed
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.
1 parent c3db1fc commit 8a27dbb

File tree

12 files changed

+75
-105
lines changed

12 files changed

+75
-105
lines changed

‎packages/react-dom/src/__tests__/ReactDOMFiber-test.js

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1154,19 +1154,13 @@ describe('ReactDOMFiber', () => {
11541154
expect(ops).toEqual(['A']);
11551155

11561156
if (__DEV__) {
1157-
const errorCalls = console.error.calls.count();
1157+
expect(console.error.calls.count()).toBe(2);
11581158
expect(console.error.calls.argsFor(0)[0]).toMatch(
11591159
'ReactDOM.render is no longer supported in React 18',
11601160
);
11611161
expect(console.error.calls.argsFor(1)[0]).toMatch(
11621162
'ReactDOM.render is no longer supported in React 18',
11631163
);
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-
}
11701164
}
11711165
});
11721166

‎packages/react-dom/src/client/ReactDOM.js

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
2323
import {
2424
batchedUpdates,
2525
discreteUpdates,
26-
flushDiscreteUpdates,
2726
flushSync,
27+
flushSyncWithWarningIfAlreadyRendering,
2828
flushControlled,
2929
injectIntoDevTools,
3030
attemptSynchronousHydration,
@@ -97,11 +97,7 @@ if (__DEV__) {
9797
}
9898

9999
setRestoreImplementation(restoreControlledState);
100-
setBatchingImplementation(
101-
batchedUpdates,
102-
discreteUpdates,
103-
flushDiscreteUpdates,
104-
);
100+
setBatchingImplementation(batchedUpdates, discreteUpdates, flushSync);
105101

106102
function createPortal(
107103
children: ReactNodeList,
@@ -166,7 +162,7 @@ const Internals = {
166162
export {
167163
createPortal,
168164
batchedUpdates as unstable_batchedUpdates,
169-
flushSync,
165+
flushSyncWithWarningIfAlreadyRendering as flushSync,
170166
Internals as __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED,
171167
ReactVersion as version,
172168
// Disabled behind disableLegacyReactDOMAPIs

‎packages/react-dom/src/events/ReactDOMUpdateBatching.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ let batchedUpdatesImpl = function(fn, bookkeeping) {
2323
let discreteUpdatesImpl = function(fn, a, b, c, d) {
2424
return fn(a, b, c, d);
2525
};
26-
let flushDiscreteUpdatesImpl = function() {};
26+
let flushSyncImpl = function() {};
2727

2828
let isInsideEventHandler = false;
2929

@@ -39,7 +39,7 @@ function finishEventHandler() {
3939
// bails out of the update without touching the DOM.
4040
// TODO: Restore state in the microtask, after the discrete updates flush,
4141
// instead of early flushing them here.
42-
flushDiscreteUpdatesImpl();
42+
flushSyncImpl();
4343
restoreStateIfNeeded();
4444
}
4545
}
@@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) {
6767
export function setBatchingImplementation(
6868
_batchedUpdatesImpl,
6969
_discreteUpdatesImpl,
70-
_flushDiscreteUpdatesImpl,
70+
_flushSyncImpl,
7171
) {
7272
batchedUpdatesImpl = _batchedUpdatesImpl;
7373
discreteUpdatesImpl = _discreteUpdatesImpl;
74-
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
74+
flushSyncImpl = _flushSyncImpl;
7575
}

‎packages/react-noop-renderer/src/ReactNoop.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ export const {
4141
unbatchedUpdates,
4242
discreteUpdates,
4343
idleUpdates,
44-
flushDiscreteUpdates,
4544
flushSync,
4645
flushPassiveEffects,
4746
act,

‎packages/react-noop-renderer/src/createReactNoop.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -915,10 +915,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
915915
}
916916
},
917917

918-
flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,
919-
920918
flushSync(fn: () => mixed) {
921-
NoopRenderer.flushSync(fn);
919+
NoopRenderer.flushSyncWithWarningIfAlreadyRendering(fn);
922920
},
923921

924922
flushPassiveEffects: NoopRenderer.flushPassiveEffects,

‎packages/react-reconciler/src/ReactFiberReconciler.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ import {
2121
unbatchedUpdates as unbatchedUpdates_old,
2222
deferredUpdates as deferredUpdates_old,
2323
discreteUpdates as discreteUpdates_old,
24-
flushDiscreteUpdates as flushDiscreteUpdates_old,
2524
flushControlled as flushControlled_old,
2625
flushSync as flushSync_old,
26+
flushSyncWithWarningIfAlreadyRendering as flushSyncWithWarningIfAlreadyRendering_old,
2727
flushPassiveEffects as flushPassiveEffects_old,
2828
getPublicRootInstance as getPublicRootInstance_old,
2929
attemptSynchronousHydration as attemptSynchronousHydration_old,
@@ -59,9 +59,9 @@ import {
5959
unbatchedUpdates as unbatchedUpdates_new,
6060
deferredUpdates as deferredUpdates_new,
6161
discreteUpdates as discreteUpdates_new,
62-
flushDiscreteUpdates as flushDiscreteUpdates_new,
6362
flushControlled as flushControlled_new,
6463
flushSync as flushSync_new,
64+
flushSyncWithWarningIfAlreadyRendering as flushSyncWithWarningIfAlreadyRendering_new,
6565
flushPassiveEffects as flushPassiveEffects_new,
6666
getPublicRootInstance as getPublicRootInstance_new,
6767
attemptSynchronousHydration as attemptSynchronousHydration_new,
@@ -108,13 +108,13 @@ export const deferredUpdates = enableNewReconciler
108108
export const discreteUpdates = enableNewReconciler
109109
? discreteUpdates_new
110110
: discreteUpdates_old;
111-
export const flushDiscreteUpdates = enableNewReconciler
112-
? flushDiscreteUpdates_new
113-
: flushDiscreteUpdates_old;
114111
export const flushControlled = enableNewReconciler
115112
? flushControlled_new
116113
: flushControlled_old;
117114
export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old;
115+
export const flushSyncWithWarningIfAlreadyRendering = enableNewReconciler
116+
? flushSyncWithWarningIfAlreadyRendering_new
117+
: flushSyncWithWarningIfAlreadyRendering_old;
118118
export const flushPassiveEffects = enableNewReconciler
119119
? flushPassiveEffects_new
120120
: flushPassiveEffects_old;

‎packages/react-reconciler/src/ReactFiberReconciler.new.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ import {
5757
flushControlled,
5858
deferredUpdates,
5959
discreteUpdates,
60-
flushDiscreteUpdates,
60+
flushSyncWithWarningIfAlreadyRendering,
6161
flushPassiveEffects,
6262
} from './ReactFiberWorkLoop.new';
6363
import {
@@ -330,9 +330,9 @@ export {
330330
unbatchedUpdates,
331331
deferredUpdates,
332332
discreteUpdates,
333-
flushDiscreteUpdates,
334333
flushControlled,
335334
flushSync,
335+
flushSyncWithWarningIfAlreadyRendering,
336336
flushPassiveEffects,
337337
};
338338

‎packages/react-reconciler/src/ReactFiberReconciler.old.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ import {
5757
flushControlled,
5858
deferredUpdates,
5959
discreteUpdates,
60-
flushDiscreteUpdates,
60+
flushSyncWithWarningIfAlreadyRendering,
6161
flushPassiveEffects,
6262
} from './ReactFiberWorkLoop.old';
6363
import {
@@ -330,9 +330,9 @@ export {
330330
unbatchedUpdates,
331331
deferredUpdates,
332332
discreteUpdates,
333-
flushDiscreteUpdates,
334333
flushControlled,
335334
flushSync,
335+
flushSyncWithWarningIfAlreadyRendering,
336336
flushPassiveEffects,
337337
};
338338

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines changed: 16 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
10441044
return executionContext;
10451045
}
10461046

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-
10751047
export function deferredUpdates<A>(fn: () => A): A {
10761048
const previousPriority = getCurrentUpdatePriority();
10771049
const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1165,18 +1137,26 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
11651137
// the stack.
11661138
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
11671139
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-
}
11761140
}
11771141
}
11781142
}
11791143

1144+
export function flushSyncWithWarningIfAlreadyRendering<A, R>(
1145+
fn: A => R,
1146+
a: A,
1147+
): 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 flushSync(fn, a);
1158+
}
1159+
11801160
export function flushControlled(fn: () => mixed): void {
11811161
const prevExecutionContext = executionContext;
11821162
executionContext |= BatchedContext;

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines changed: 16 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
10441044
return executionContext;
10451045
}
10461046

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-
10751047
export function deferredUpdates<A>(fn: () => A): A {
10761048
const previousPriority = getCurrentUpdatePriority();
10771049
const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1165,18 +1137,26 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
11651137
// the stack.
11661138
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
11671139
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-
}
11761140
}
11771141
}
11781142
}
11791143

1144+
export function flushSyncWithWarningIfAlreadyRendering<A, R>(
1145+
fn: A => R,
1146+
a: A,
1147+
): 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 flushSync(fn, a);
1158+
}
1159+
11801160
export function flushControlled(fn: () => mixed): void {
11811161
const prevExecutionContext = executionContext;
11821162
executionContext |= BatchedContext;

‎packages/react-reconciler/src/__tests__/ReactFlushSync-test.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,4 +173,27 @@ describe('ReactFlushSync', () => {
173173
// Effect flushes after paint.
174174
expect(Scheduler).toHaveYielded(['Effect']);
175175
});
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+
});
176199
});

‎packages/react-test-renderer/src/ReactTestRenderer.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
getPublicRootInstance,
1919
createContainer,
2020
updateContainer,
21-
flushSync,
21+
flushSyncWithWarningIfAlreadyRendering,
2222
injectIntoDevTools,
2323
batchedUpdates,
2424
} from 'react-reconciler/src/ReactFiberReconciler';
@@ -543,7 +543,7 @@ function create(element: React$Element<any>, options: TestRendererOptions) {
543543
},
544544

545545
unstable_flushSync<T>(fn: () => T): T {
546-
return flushSync(fn);
546+
return flushSyncWithWarningIfAlreadyRendering(fn);
547547
},
548548
};
549549

0 commit comments

Comments
 (0)