Don't "schedule" discrete work if we're scheduling sync work (#18797)
Sebastian Markbåge committed
May 1, 2020 at 08:43 UTC
17dcc29cd1b53fe15b370631e1b781c070c01c7a
5 files changed
+98
-77
packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
+46
@@ -13,6 +13,7 @@ let React;
13
14
let ReactDOM;
15
let Scheduler;
16
+let act;
17
18
const setUntrackedInputValue = Object.getOwnPropertyDescriptor(
19
HTMLInputElement.prototype,
@@ -27,6 +28,7 @@ describe('ReactDOMFiberAsync', () => {
28
container = document.createElement('div');
29
React = require('react');
30
ReactDOM = require('react-dom');
31
+ act = require('react-dom/test-utils').act;
32
Scheduler = require('scheduler');
33
34
document.body.appendChild(container);
@@ -635,4 +637,48 @@ describe('ReactDOMFiberAsync', () => {
637
expect(container.textContent).toEqual('ABC');
638
});
639
});
640
+
641
+ // @gate experimental
642
+ it('unmounted roots should never clear newer root content from a container', () => {
643
+ const ref = React.createRef();
644
+
645
+ function OldApp() {
646
+ const [value, setValue] = React.useState('old');
647
+ function hideOnClick() {
648
+ // Schedule a discrete update.
649
+ setValue('update');
650
+ // Synchronously unmount this root.
651
+ ReactDOM.flushSync(() => oldRoot.unmount());
652
+ }
653
+ return (
654
+ <button onClick={hideOnClick} ref={ref}>
655
+ {value}
656
+ </button>
657
+ );
658
+ }
659
+
660
+ function NewApp() {
661
+ return <button ref={ref}>new</button>;
662
+ }
663
+
664
+ const oldRoot = ReactDOM.createRoot(container);
665
+ act(() => {
666
+ oldRoot.render(<OldApp />);
667
+ });
668
+
669
+ // Invoke discrete event.
670
+ ref.current.click();
671
+
672
+ // The root should now be unmounted.
673
+ expect(container.textContent).toBe('');
674
+
675
+ // We can now render a new one.
676
+ const newRoot = ReactDOM.createRoot(container);
677
+ ReactDOM.flushSync(() => {
678
+ newRoot.render(<NewApp />);
679
+ });
680
+ ref.current.click();
681
+
682
+ expect(container.textContent).toBe('new');
683
+ });
684
});
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+1
-16
@@ -688,22 +688,7 @@ function completeWork(
688
// This handles the case of React rendering into a container with previous children.
689
// It's also safe to do for updates too, because current.child would only be null
690
// if the previous render was null (so the the container would already be empty).
691
- //
692
- // The additional root.hydrate check above is required for hydration in legacy mode with no fallback.
693
- //
694
- // The root container check below also avoids a potential legacy mode problem
695
- // where unmounting from a container then rendering into it again
696
- // can sometimes cause the container to be cleared after the new render.
697
- const containerInfo = fiberRoot.containerInfo;
698
- const legacyRootContainer =
699
- containerInfo == null ? null : containerInfo._reactRootContainer;
700
- if (
701
- legacyRootContainer == null ||
702
- legacyRootContainer._internalRoot == null ||
703
- legacyRootContainer._internalRoot === fiberRoot
704
- ) {
705
- workInProgress.effectTag |= Snapshot;
706
- }
691
+ workInProgress.effectTag |= Snapshot;
692
}
693
}
694
updateHostContainer(workInProgress);
packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+1
-16
@@ -684,22 +684,7 @@ function completeWork(
684
// This handles the case of React rendering into a container with previous children.
685
// It's also safe to do for updates too, because current.child would only be null
686
// if the previous render was null (so the the container would already be empty).
687
- //
688
- // The additional root.hydrate check above is required for hydration in legacy mode with no fallback.
689
- //
690
- // The root container check below also avoids a potential legacy mode problem
691
- // where unmounting from a container then rendering into it again
692
- // can sometimes cause the container to be cleared after the new render.
693
- const containerInfo = fiberRoot.containerInfo;
694
- const legacyRootContainer =
695
- containerInfo == null ? null : containerInfo._reactRootContainer;
696
- if (
697
- legacyRootContainer == null ||
698
- legacyRootContainer._internalRoot == null ||
699
- legacyRootContainer._internalRoot === fiberRoot
700
- ) {
701
- workInProgress.effectTag |= Snapshot;
702
- }
687
+ workInProgress.effectTag |= Snapshot;
688
}
689
}
690
updateHostContainer(workInProgress);
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+25
-24
@@ -486,30 +486,31 @@ export function scheduleUpdateOnFiber(
486
}
487
}
488
} else {
489
- ensureRootIsScheduled(root);
490
- schedulePendingInteractions(root, expirationTime);
491
- }
492
-
493
- if (
494
- (executionContext & DiscreteEventContext) !== NoContext &&
495
- // Only updates at user-blocking priority or greater are considered
496
- // discrete, even inside a discrete event.
497
- (priorityLevel === UserBlockingSchedulerPriority ||
498
- priorityLevel === ImmediateSchedulerPriority)
499
- ) {
500
- // This is the result of a discrete event. Track the lowest priority
501
- // discrete update per root so we can flush them early, if needed.
502
- if (rootsWithPendingDiscreteUpdates === null) {
503
- rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
504
- } else {
505
- const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
506
- if (
507
- lastDiscreteTime === undefined ||
508
- !isSameOrHigherPriority(expirationTime, lastDiscreteTime)
509
- ) {
510
- rootsWithPendingDiscreteUpdates.set(root, expirationTime);
489
+ // Schedule a discrete update but only if it's not Sync.
490
+ if (
491
+ (executionContext & DiscreteEventContext) !== NoContext &&
492
+ // Only updates at user-blocking priority or greater are considered
493
+ // discrete, even inside a discrete event.
494
+ (priorityLevel === UserBlockingSchedulerPriority ||
495
+ priorityLevel === ImmediateSchedulerPriority)
496
+ ) {
497
+ // This is the result of a discrete event. Track the lowest priority
498
+ // discrete update per root so we can flush them early, if needed.
499
+ if (rootsWithPendingDiscreteUpdates === null) {
500
+ rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
501
+ } else {
502
+ const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
503
+ if (
504
+ lastDiscreteTime === undefined ||
505
+ !isSameOrHigherPriority(expirationTime, lastDiscreteTime)
506
+ ) {
507
+ rootsWithPendingDiscreteUpdates.set(root, expirationTime);
508
+ }
509
}
510
}
511
+ // Schedule other updates after in case the callback is sync.
512
+ ensureRootIsScheduled(root);
513
+ schedulePendingInteractions(root, expirationTime);
514
}
515
}
516
@@ -1167,9 +1168,9 @@ function flushPendingDiscreteUpdates() {
1168
markRootExpiredAtTime(root, expirationTime);
1169
ensureRootIsScheduled(root);
1170
});
1170
- // Now flush the immediate queue.
1171
- flushSyncCallbackQueue();
1171
}
1172
+ // Now flush the immediate queue.
1173
+ flushSyncCallbackQueue();
1174
}
1175
1176
export function batchedUpdates<A, R>(fn: A => R, a: A): R {
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+25
-21
@@ -456,27 +456,31 @@ export function scheduleUpdateOnFiber(
456
}
457
}
458
} else {
459
- ensureRootIsScheduled(root);
460
- schedulePendingInteractions(root, expirationTime);
461
- }
462
-
463
- if (
464
- (executionContext & DiscreteEventContext) !== NoContext &&
465
- // Only updates at user-blocking priority or greater are considered
466
- // discrete, even inside a discrete event.
467
- (priorityLevel === UserBlockingPriority ||
468
- priorityLevel === ImmediatePriority)
469
- ) {
470
- // This is the result of a discrete event. Track the lowest priority
471
- // discrete update per root so we can flush them early, if needed.
472
- if (rootsWithPendingDiscreteUpdates === null) {
473
- rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
474
- } else {
475
- const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
476
- if (lastDiscreteTime === undefined || lastDiscreteTime > expirationTime) {
477
- rootsWithPendingDiscreteUpdates.set(root, expirationTime);
459
+ // Schedule a discrete update but only if it's not Sync.
460
+ if (
461
+ (executionContext & DiscreteEventContext) !== NoContext &&
462
+ // Only updates at user-blocking priority or greater are considered
463
+ // discrete, even inside a discrete event.
464
+ (priorityLevel === UserBlockingPriority ||
465
+ priorityLevel === ImmediatePriority)
466
+ ) {
467
+ // This is the result of a discrete event. Track the lowest priority
468
+ // discrete update per root so we can flush them early, if needed.
469
+ if (rootsWithPendingDiscreteUpdates === null) {
470
+ rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]);
471
+ } else {
472
+ const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
473
+ if (
474
+ lastDiscreteTime === undefined ||
475
+ lastDiscreteTime > expirationTime
476
+ ) {
477
+ rootsWithPendingDiscreteUpdates.set(root, expirationTime);
478
+ }
479
}
480
}
481
+ // Schedule other updates after in case the callback is sync.
482
+ ensureRootIsScheduled(root);
483
+ schedulePendingInteractions(root, expirationTime);
484
}
485
}
486
@@ -1080,9 +1084,9 @@ function flushPendingDiscreteUpdates() {
1084
markRootExpiredAtTime(root, expirationTime);
1085
ensureRootIsScheduled(root);
1086
});
1083
- // Now flush the immediate queue.
1084
- flushSyncCallbackQueue();
1087
}
1088
+ // Now flush the immediate queue.
1089
+ flushSyncCallbackQueue();
1090
}
1091
1092
export function batchedUpdates<A, R>(fn: A => R, a: A): R {