Skip to content
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

Reverting recent flushSync changes #21816

Merged
merged 2 commits into from
Jul 7, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Object {
6 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 16,
"updaters": Array [
Object {
Expand Down Expand Up @@ -87,7 +87,7 @@ Object {
4 => 2,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 15,
"updaters": Array [
Object {
Expand Down Expand Up @@ -186,7 +186,7 @@ Object {
6 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 12,
"updaters": Array [
Object {
Expand Down Expand Up @@ -445,7 +445,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 12,
"updaters": Array [
Object {
Expand Down Expand Up @@ -938,7 +938,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 11,
"updaters": Array [
Object {
Expand Down Expand Up @@ -1597,7 +1597,7 @@ Object {
17 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 24,
"updaters": Array [
Object {
Expand Down Expand Up @@ -1687,7 +1687,7 @@ Object {
"fiberActualDurations": Map {},
"fiberSelfDurations": Map {},
"passiveEffectDuration": 0,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 34,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2223,7 +2223,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 24,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2310,7 +2310,7 @@ Object {
"fiberActualDurations": Array [],
"fiberSelfDurations": Array [],
"passiveEffectDuration": 0,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 34,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2431,7 +2431,7 @@ Object {
2 => 0,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2506,7 +2506,7 @@ Object {
3 => 0,
},
"passiveEffectDuration": 0,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2715,7 +2715,7 @@ Object {
],
],
"passiveEffectDuration": 0,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -3071,7 +3071,7 @@ Object {
7 => 0,
},
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -3515,7 +3515,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Immediate",
"priorityLevel": "Normal",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down
8 changes: 7 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1154,13 +1154,19 @@ describe('ReactDOMFiber', () => {
expect(ops).toEqual(['A']);

if (__DEV__) {
expect(console.error.calls.count()).toBe(2);
const errorCalls = console.error.calls.count();
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
12 changes: 6 additions & 6 deletions packages/react-dom/src/__tests__/ReactMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ describe('ReactMount', () => {
expect(calls).toBe(5);
});

it('initial mount of legacy root is sync inside batchedUpdates, as if it were wrapped in flushSync', () => {
it('initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch', () => {
const container1 = document.createElement('div');
const container2 = document.createElement('div');

Expand All @@ -302,12 +302,12 @@ describe('ReactMount', () => {

// Initial mount on another root. Should flush immediately.
ReactDOM.render(<Foo>a</Foo>, container2);
// The earlier update also flushed, since flushSync flushes all pending
// sync work across all roots.
expect(container1.textContent).toEqual('2');
// Layout updates are also flushed synchronously
expect(container2.textContent).toEqual('a!');
// The update did not flush yet.
expect(container1.textContent).toEqual('1');
// The initial mount flushed, but not the update scheduled in cDM.
expect(container2.textContent).toEqual('a');
});
// All updates have flushed.
expect(container1.textContent).toEqual('2');
expect(container2.textContent).toEqual('a!');
});
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,
flushSyncWithoutWarningIfAlreadyRendering,
flushDiscreteUpdates,
);

function createPortal(
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/client/ReactDOMLegacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
createContainer,
findHostInstanceWithNoPortals,
updateContainer,
flushSyncWithoutWarningIfAlreadyRendering,
unbatchedUpdates,
getPublicRootInstance,
findHostInstance,
findHostInstanceWithWarning,
Expand Down Expand Up @@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
};
}
// Initial mount should not be batched.
flushSyncWithoutWarningIfAlreadyRendering(() => {
unbatchedUpdates(() => {
updateContainer(children, fiberRoot, parentComponent, callback);
});
} else {
Expand Down Expand Up @@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) {
}

// Unmount should not be batched.
flushSyncWithoutWarningIfAlreadyRendering(() => {
unbatchedUpdates(() => {
legacyRenderSubtreeIntoContainer(null, null, container, false, () => {
// $FlowFixMe This should probably use `delete container._reactRootContainer`
container._reactRootContainer = null;
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 flushSyncImpl = function() {};
let flushDiscreteUpdatesImpl = 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.
flushSyncImpl();
flushDiscreteUpdatesImpl();
restoreStateIfNeeded();
}
}
Expand Down Expand Up @@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) {
export function setBatchingImplementation(
_batchedUpdatesImpl,
_discreteUpdatesImpl,
_flushSyncImpl,
_flushDiscreteUpdatesImpl,
) {
batchedUpdatesImpl = _batchedUpdatesImpl;
discreteUpdatesImpl = _discreteUpdatesImpl;
flushSyncImpl = _flushSyncImpl;
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
}
2 changes: 2 additions & 0 deletions packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@ export const {
flushExpired,
batchedUpdates,
deferredUpdates,
unbatchedUpdates,
discreteUpdates,
idleUpdates,
flushDiscreteUpdates,
flushSync,
flushPassiveEffects,
act,
Expand Down
1 change: 1 addition & 0 deletions packages/react-noop-renderer/src/ReactNoopPersistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const {
flushExpired,
batchedUpdates,
deferredUpdates,
unbatchedUpdates,
discreteUpdates,
idleUpdates,
flushDiscreteUpdates,
Expand Down
4 changes: 4 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

deferredUpdates: NoopRenderer.deferredUpdates,

unbatchedUpdates: NoopRenderer.unbatchedUpdates,

discreteUpdates: NoopRenderer.discreteUpdates,

idleUpdates<T>(fn: () => T): T {
Expand All @@ -913,6 +915,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}
},

flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,

flushSync(fn: () => mixed) {
NoopRenderer.flushSync(fn);
},
Expand Down
15 changes: 10 additions & 5 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import {
createContainer as createContainer_old,
updateContainer as updateContainer_old,
batchedUpdates as batchedUpdates_old,
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 @@ -55,11 +56,12 @@ import {
createContainer as createContainer_new,
updateContainer as updateContainer_new,
batchedUpdates as batchedUpdates_new,
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 @@ -97,19 +99,22 @@ export const updateContainer = enableNewReconciler
export const batchedUpdates = enableNewReconciler
? batchedUpdates_new
: batchedUpdates_old;
export const unbatchedUpdates = enableNewReconciler
? unbatchedUpdates_new
: unbatchedUpdates_old;
export const deferredUpdates = enableNewReconciler
? deferredUpdates_new
: deferredUpdates_old;
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
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ import {
scheduleUpdateOnFiber,
flushRoot,
batchedUpdates,
unbatchedUpdates,
flushSync,
flushControlled,
deferredUpdates,
discreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
flushDiscreteUpdates,
flushPassiveEffects,
} from './ReactFiberWorkLoop.new';
import {
Expand Down Expand Up @@ -326,11 +327,12 @@ export function updateContainer(

export {
batchedUpdates,
unbatchedUpdates,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushControlled,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
};

Expand Down
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ import {
scheduleUpdateOnFiber,
flushRoot,
batchedUpdates,
unbatchedUpdates,
flushSync,
flushControlled,
deferredUpdates,
discreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
flushDiscreteUpdates,
flushPassiveEffects,
} from './ReactFiberWorkLoop.old';
import {
Expand Down Expand Up @@ -326,11 +327,12 @@ export function updateContainer(

export {
batchedUpdates,
unbatchedUpdates,
deferredUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushControlled,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushPassiveEffects,
};

Expand Down
Loading