Skip to content

Replace flushDiscreteUpdates with flushSync #21775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1154,19 +1154,13 @@ describe('ReactDOMFiber', () => {
expect(ops).toEqual(['A']);

if (__DEV__) {
const errorCalls = console.error.calls.count();
expect(console.error.calls.count()).toBe(2);
expect(console.error.calls.argsFor(0)[0]).toMatch(
'ReactDOM.render is no longer supported in React 18',
);
expect(console.error.calls.argsFor(1)[0]).toMatch(
'ReactDOM.render is no longer supported in React 18',
);
// TODO: this warning shouldn't be firing in the first place if user didn't call it.
for (let i = 2; i < errorCalls; i++) {
expect(console.error.calls.argsFor(i)[0]).toMatch(
'unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering.',
);
}
}
});

Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
import {
batchedUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushControlled,
injectIntoDevTools,
attemptSynchronousHydration,
Expand Down Expand Up @@ -100,7 +100,7 @@ setRestoreImplementation(restoreControlledState);
setBatchingImplementation(
batchedUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
);

function createPortal(
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom/src/events/ReactDOMUpdateBatching.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ let batchedUpdatesImpl = function(fn, bookkeeping) {
let discreteUpdatesImpl = function(fn, a, b, c, d) {
return fn(a, b, c, d);
};
let flushDiscreteUpdatesImpl = function() {};
let flushSyncImpl = function() {};

let isInsideEventHandler = false;

Expand All @@ -39,7 +39,7 @@ function finishEventHandler() {
// bails out of the update without touching the DOM.
// TODO: Restore state in the microtask, after the discrete updates flush,
// instead of early flushing them here.
flushDiscreteUpdatesImpl();
flushSyncImpl();
restoreStateIfNeeded();
}
}
Expand Down Expand Up @@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) {
export function setBatchingImplementation(
_batchedUpdatesImpl,
_discreteUpdatesImpl,
_flushDiscreteUpdatesImpl,
_flushSyncImpl,
) {
batchedUpdatesImpl = _batchedUpdatesImpl;
discreteUpdatesImpl = _discreteUpdatesImpl;
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
flushSyncImpl = _flushSyncImpl;
}
1 change: 0 additions & 1 deletion packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export const {
unbatchedUpdates,
discreteUpdates,
idleUpdates,
flushDiscreteUpdates,
flushSync,
flushPassiveEffects,
act,
Expand Down
2 changes: 0 additions & 2 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -915,8 +915,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}
},

flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,

flushSync(fn: () => mixed) {
NoopRenderer.flushSync(fn);
},
Expand Down
10 changes: 5 additions & 5 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import {
unbatchedUpdates as unbatchedUpdates_old,
deferredUpdates as deferredUpdates_old,
discreteUpdates as discreteUpdates_old,
flushDiscreteUpdates as flushDiscreteUpdates_old,
flushControlled as flushControlled_old,
flushSync as flushSync_old,
flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_old,
flushPassiveEffects as flushPassiveEffects_old,
getPublicRootInstance as getPublicRootInstance_old,
attemptSynchronousHydration as attemptSynchronousHydration_old,
Expand Down Expand Up @@ -59,9 +59,9 @@ import {
unbatchedUpdates as unbatchedUpdates_new,
deferredUpdates as deferredUpdates_new,
discreteUpdates as discreteUpdates_new,
flushDiscreteUpdates as flushDiscreteUpdates_new,
flushControlled as flushControlled_new,
flushSync as flushSync_new,
flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_new,
flushPassiveEffects as flushPassiveEffects_new,
getPublicRootInstance as getPublicRootInstance_new,
attemptSynchronousHydration as attemptSynchronousHydration_new,
Expand Down Expand Up @@ -108,13 +108,13 @@ export const deferredUpdates = enableNewReconciler
export const discreteUpdates = enableNewReconciler
? discreteUpdates_new
: discreteUpdates_old;
export const flushDiscreteUpdates = enableNewReconciler
? flushDiscreteUpdates_new
: flushDiscreteUpdates_old;
export const flushControlled = enableNewReconciler
? flushControlled_new
: flushControlled_old;
export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old;
export const flushSyncWithoutWarningIfAlreadyRendering = enableNewReconciler
? flushSyncWithoutWarningIfAlreadyRendering_new
: flushSyncWithoutWarningIfAlreadyRendering_old;
export const flushPassiveEffects = enableNewReconciler
? flushPassiveEffects_new
: flushPassiveEffects_old;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import {
flushControlled,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
} from './ReactFiberWorkLoop.new';
import {
Expand Down Expand Up @@ -330,9 +330,9 @@ export {
unbatchedUpdates,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushControlled,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
};

Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import {
flushControlled,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
} from './ReactFiberWorkLoop.old';
import {
Expand Down Expand Up @@ -330,9 +330,9 @@ export {
unbatchedUpdates,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushControlled,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
};

Expand Down
54 changes: 17 additions & 37 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
return executionContext;
}

export function flushDiscreteUpdates() {
// TODO: Should be able to flush inside batchedUpdates, but not inside `act`.
// However, `act` uses `batchedUpdates`, so there's no way to distinguish
// those two cases. Need to fix this before exposing flushDiscreteUpdates
// as a public API.
if (
(executionContext & (BatchedContext | RenderContext | CommitContext)) !==
NoContext
) {
if (__DEV__) {
if ((executionContext & RenderContext) !== NoContext) {
console.error(
'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' +
'already rendering.',
);
}
}
// We're already rendering, so we can't synchronously flush pending work.
// This is probably a nested event dispatch triggered by a lifecycle/effect,
// like `el.focus()`. Exit.
return;
}
flushSyncCallbacks();
// If the discrete updates scheduled passive effects, flush them now so that
// they fire before the next serial event.
flushPassiveEffects();
}

export function deferredUpdates<A>(fn: () => A): A {
const previousPriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
Expand Down Expand Up @@ -1142,7 +1114,10 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
}
}

export function flushSync<A, R>(fn: A => R, a: A): R {
export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
fn: A => R,
a: A,
): R {
const prevExecutionContext = executionContext;
executionContext |= BatchedContext;

Expand All @@ -1165,18 +1140,23 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
// the stack.
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
flushSyncCallbacks();
} else {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
}
}

export function flushSync<A, R>(fn: A => R, a: A): R {
if (__DEV__) {
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
return flushSyncWithoutWarningIfAlreadyRendering(fn, a);
}

export function flushControlled(fn: () => mixed): void {
const prevExecutionContext = executionContext;
executionContext |= BatchedContext;
Expand Down
54 changes: 17 additions & 37 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1044,34 +1044,6 @@ export function getExecutionContext(): ExecutionContext {
return executionContext;
}

export function flushDiscreteUpdates() {
// TODO: Should be able to flush inside batchedUpdates, but not inside `act`.
// However, `act` uses `batchedUpdates`, so there's no way to distinguish
// those two cases. Need to fix this before exposing flushDiscreteUpdates
// as a public API.
if (
(executionContext & (BatchedContext | RenderContext | CommitContext)) !==
NoContext
) {
if (__DEV__) {
if ((executionContext & RenderContext) !== NoContext) {
console.error(
'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' +
'already rendering.',
);
}
}
// We're already rendering, so we can't synchronously flush pending work.
// This is probably a nested event dispatch triggered by a lifecycle/effect,
// like `el.focus()`. Exit.
return;
}
flushSyncCallbacks();
// If the discrete updates scheduled passive effects, flush them now so that
// they fire before the next serial event.
flushPassiveEffects();
}

export function deferredUpdates<A>(fn: () => A): A {
const previousPriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
Expand Down Expand Up @@ -1142,7 +1114,10 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
}
}

export function flushSync<A, R>(fn: A => R, a: A): R {
export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
fn: A => R,
a: A,
): R {
const prevExecutionContext = executionContext;
executionContext |= BatchedContext;

Expand All @@ -1165,18 +1140,23 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
// the stack.
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
flushSyncCallbacks();
} else {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
}
}

export function flushSync<A, R>(fn: A => R, a: A): R {
if (__DEV__) {
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
return flushSyncWithoutWarningIfAlreadyRendering(fn, a);
}

export function flushControlled(fn: () => mixed): void {
const prevExecutionContext = executionContext;
executionContext |= BatchedContext;
Expand Down
23 changes: 23 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,27 @@ describe('ReactFlushSync', () => {
// Effect flushes after paint.
expect(Scheduler).toHaveYielded(['Effect']);
});

test('does not flush pending passive effects', async () => {
function App() {
useEffect(() => {
Scheduler.unstable_yieldValue('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(<App />);
expect(Scheduler).toFlushUntilNextPaint(['Child']);
expect(root).toMatchRenderedOutput('Child');

// Passive effects are pending. Calling flushSync should not affect them.
ReactNoop.flushSync();
// Effects still haven't fired.
expect(Scheduler).toHaveYielded([]);
});
// Now the effects have fired.
expect(Scheduler).toHaveYielded(['Effect']);
});
});