Skip to content

Commit

Permalink
Bugfix: Flush legacy sync passive effects at beginning of event (face…
Browse files Browse the repository at this point in the history
…book#21846)

* Re-land recent flushSync changes

Adds back facebook#21776 and facebook#21775, which were removed due to an internal
e2e test failure.

Will attempt to fix in subsequent commits.

* Failing test: Legacy mode sync passive effects

In concurrent roots, if a render is synchronous, we flush its passive
effects synchronously. In legacy roots, we don't do this because all
updates are synchronous — so we need to flush at the beginning of the
next event. This is how `discreteUpdates` worked.

* Flush legacy passive effects at beginning of event

Fixes test added in previous commit.
  • Loading branch information
acdlite authored and zhengjitf committed Apr 15, 2022
1 parent 364efa8 commit 04faf12
Show file tree
Hide file tree
Showing 17 changed files with 184 additions and 337 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Object {
6 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 16,
"updaters": Array [
Object {
Expand Down Expand Up @@ -87,7 +87,7 @@ Object {
4 => 2,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 15,
"updaters": Array [
Object {
Expand Down Expand Up @@ -186,7 +186,7 @@ Object {
6 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 12,
"updaters": Array [
Object {
Expand Down Expand Up @@ -445,7 +445,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 12,
"updaters": Array [
Object {
Expand Down Expand Up @@ -938,7 +938,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 11,
"updaters": Array [
Object {
Expand Down Expand Up @@ -1597,7 +1597,7 @@ Object {
17 => 1,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 24,
"updaters": Array [
Object {
Expand Down Expand Up @@ -1687,7 +1687,7 @@ Object {
"fiberActualDurations": Map {},
"fiberSelfDurations": Map {},
"passiveEffectDuration": 0,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 34,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2223,7 +2223,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 24,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2310,7 +2310,7 @@ Object {
"fiberActualDurations": Array [],
"fiberSelfDurations": Array [],
"passiveEffectDuration": 0,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 34,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2431,7 +2431,7 @@ Object {
2 => 0,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2506,7 +2506,7 @@ Object {
3 => 0,
},
"passiveEffectDuration": 0,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -2715,7 +2715,7 @@ Object {
],
],
"passiveEffectDuration": 0,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -3071,7 +3071,7 @@ Object {
7 => 0,
},
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down Expand Up @@ -3515,7 +3515,7 @@ Object {
],
],
"passiveEffectDuration": null,
"priorityLevel": "Normal",
"priorityLevel": "Immediate",
"timestamp": 0,
"updaters": Array [
Object {
Expand Down
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
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 is sync inside batchedUpdates, but task work is deferred until the end of the batch', () => {
it('initial mount of legacy root is sync inside batchedUpdates, as if it were wrapped in flushSync', () => {
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 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');
// 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!');
});
// 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,
flushDiscreteUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
);

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,
unbatchedUpdates,
flushSyncWithoutWarningIfAlreadyRendering,
getPublicRootInstance,
findHostInstance,
findHostInstanceWithWarning,
Expand Down Expand Up @@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
};
}
// Initial mount should not be batched.
unbatchedUpdates(() => {
flushSyncWithoutWarningIfAlreadyRendering(() => {
updateContainer(children, fiberRoot, parentComponent, callback);
});
} else {
Expand Down Expand Up @@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) {
}

// Unmount should not be batched.
unbatchedUpdates(() => {
flushSyncWithoutWarningIfAlreadyRendering(() => {
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 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;
}
2 changes: 0 additions & 2 deletions packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,8 @@ export const {
flushExpired,
batchedUpdates,
deferredUpdates,
unbatchedUpdates,
discreteUpdates,
idleUpdates,
flushDiscreteUpdates,
flushSync,
flushPassiveEffects,
act,
Expand Down
1 change: 0 additions & 1 deletion packages/react-noop-renderer/src/ReactNoopPersistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export const {
flushExpired,
batchedUpdates,
deferredUpdates,
unbatchedUpdates,
discreteUpdates,
idleUpdates,
flushDiscreteUpdates,
Expand Down
4 changes: 0 additions & 4 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,8 +901,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

deferredUpdates: NoopRenderer.deferredUpdates,

unbatchedUpdates: NoopRenderer.unbatchedUpdates,

discreteUpdates: NoopRenderer.discreteUpdates,

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

flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,

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

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

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

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

Expand Down
Loading

0 comments on commit 04faf12

Please sign in to comment.