BugFix: Suspense priority warning firing when not supposed to (#16256)
Previously, the suspense priority warning was fired even if the Root wasn't suspended. Changed the warning to fire only when the root is suspended. Also refactored the suspense priority warning so it's easier to read.
lunaruan committed
Aug 2, 2019 at 13:54 UTC
c4c9f086eb9b61a36d9d96a847374ea65147b6cb
4 files changed
+88
-187
packages/react-reconciler/src/ReactFiberWorkLoop.js
+22
-58
@@ -802,7 +802,6 @@ function prepareFreshStack(root, expirationTime) {
802
803
if (__DEV__) {
804
ReactStrictModeWarnings.discardPendingWarnings();
805
- componentsThatSuspendedAtHighPri = null;
805
componentsThatTriggeredHighPriSuspend = null;
806
}
807
}
@@ -990,8 +989,6 @@ function renderRoot(
989
// Set this to null to indicate there's no in-progress render.
990
workInProgressRoot = null;
991
993
- flushSuspensePriorityWarningInDEV();
994
-
992
switch (workInProgressRootExitStatus) {
993
case RootIncomplete: {
994
invariant(false, 'Should have a work-in-progress.');
@@ -1022,6 +1019,8 @@ function renderRoot(
1019
return commitRoot.bind(null, root);
1020
}
1021
case RootSuspended: {
1022
+ flushSuspensePriorityWarningInDEV();
1023
+
1024
// We have an acceptable loading state. We need to figure out if we should
1025
// immediately commit it or wait a bit.
1026
@@ -1076,6 +1075,8 @@ function renderRoot(
1075
return commitRoot.bind(null, root);
1076
}
1077
case RootSuspendedWithDelay: {
1078
+ flushSuspensePriorityWarningInDEV();
1079
+
1080
if (
1081
!isSync &&
1082
// do not delay if we're inside an act() scope
@@ -2610,7 +2611,6 @@ export function warnIfUnmockedScheduler(fiber: Fiber) {
2611
}
2612
}
2613
2613
-let componentsThatSuspendedAtHighPri = null;
2614
let componentsThatTriggeredHighPriSuspend = null;
2615
export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
2616
if (__DEV__) {
@@ -2697,70 +2697,34 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
2697
}
2698
workInProgressNode = workInProgressNode.return;
2699
}
2700
-
2701
- // Add the component name to a set.
2702
- const componentName = getComponentName(sourceFiber.type);
2703
- if (componentsThatSuspendedAtHighPri === null) {
2704
- componentsThatSuspendedAtHighPri = new Set([componentName]);
2705
- } else {
2706
- componentsThatSuspendedAtHighPri.add(componentName);
2707
- }
2700
}
2701
}
2702
}
2703
2704
function flushSuspensePriorityWarningInDEV() {
2705
if (__DEV__) {
2714
- if (componentsThatSuspendedAtHighPri !== null) {
2706
+ if (componentsThatTriggeredHighPriSuspend !== null) {
2707
const componentNames = [];
2716
- componentsThatSuspendedAtHighPri.forEach(name => {
2717
- componentNames.push(name);
2718
- });
2719
- componentsThatSuspendedAtHighPri = null;
2720
-
2721
- const componentsThatTriggeredSuspendNames = [];
2722
- if (componentsThatTriggeredHighPriSuspend !== null) {
2723
- componentsThatTriggeredHighPriSuspend.forEach(name =>
2724
- componentsThatTriggeredSuspendNames.push(name),
2725
- );
2726
- }
2727
-
2708
+ componentsThatTriggeredHighPriSuspend.forEach(name =>
2709
+ componentNames.push(name),
2710
+ );
2711
componentsThatTriggeredHighPriSuspend = null;
2712
2730
- const componentNamesString = componentNames.sort().join(', ');
2731
- let componentThatTriggeredSuspenseError = '';
2732
- if (componentsThatTriggeredSuspendNames.length > 0) {
2733
- componentThatTriggeredSuspenseError =
2734
- 'The following components triggered a user-blocking update:' +
2735
- '\n\n' +
2736
- ' ' +
2737
- componentsThatTriggeredSuspendNames.sort().join(', ') +
2738
- '\n\n' +
2739
- 'that was then suspended by:' +
2740
- '\n\n' +
2741
- ' ' +
2742
- componentNamesString;
2743
- } else {
2744
- componentThatTriggeredSuspenseError =
2745
- 'A user-blocking update was suspended by:' +
2746
- '\n\n' +
2747
- ' ' +
2748
- componentNamesString;
2713
+ if (componentNames.length > 0) {
2714
+ warningWithoutStack(
2715
+ false,
2716
+ '%s triggered a user-blocking update that suspended.' +
2717
+ '\n\n' +
2718
+ 'The fix is to split the update into multiple parts: a user-blocking ' +
2719
+ 'update to provide immediate feedback, and another update that ' +
2720
+ 'triggers the bulk of the changes.' +
2721
+ '\n\n' +
2722
+ 'Refer to the documentation for useSuspenseTransition to learn how ' +
2723
+ 'to implement this pattern.',
2724
+ // TODO: Add link to React docs with more information, once it exists
2725
+ componentNames.sort().join(', '),
2726
+ );
2727
}
2750
-
2751
- warningWithoutStack(
2752
- false,
2753
- '%s' +
2754
- '\n\n' +
2755
- 'The fix is to split the update into multiple parts: a user-blocking ' +
2756
- 'update to provide immediate feedback, and another update that ' +
2757
- 'triggers the bulk of the changes.' +
2758
- '\n\n' +
2759
- 'Refer to the documentation for useSuspenseTransition to learn how ' +
2760
- 'to implement this pattern.',
2761
- // TODO: Add link to React docs with more information, once it exists
2762
- componentThatTriggeredSuspenseError,
2763
- );
2728
}
2729
}
2730
}
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
-11
@@ -327,7 +327,6 @@ describe('ReactSuspense', () => {
327
});
328
329
it('throws if tree suspends and none of the Suspense ancestors have a fallback', () => {
330
- spyOnDev(console, 'error');
330
ReactTestRenderer.create(
331
<Suspense>
332
<AsyncText text="Hi" ms={1000} />
@@ -341,16 +340,6 @@ describe('ReactSuspense', () => {
340
'AsyncText suspended while rendering, but no fallback UI was specified.',
341
);
342
expect(Scheduler).toHaveYielded(['Suspend! [Hi]', 'Suspend! [Hi]']);
344
- if (__DEV__) {
345
- expect(console.error).toHaveBeenCalledTimes(2);
346
- expect(console.error.calls.argsFor(0)[0]).toContain(
347
- 'Warning: %s\n\nThe fix is to split the update',
348
- );
349
- expect(console.error.calls.argsFor(0)[1]).toContain(
350
- 'A user-blocking update was suspended by:',
351
- );
352
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
353
- }
343
});
344
345
describe('outside concurrent mode', () => {
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+66
-106
@@ -489,7 +489,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
489
});
490
491
it('tries rendering a lower priority pending update even if a higher priority one suspends', async () => {
492
- spyOnDev(console, 'error');
492
function App(props) {
493
if (props.hide) {
494
return <Text text="(empty)" />;
@@ -517,16 +516,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
516
'(empty)',
517
]);
518
expect(ReactNoop.getChildren()).toEqual([span('(empty)')]);
520
- if (__DEV__) {
521
- expect(console.error).toHaveBeenCalledTimes(1);
522
- expect(console.error.calls.argsFor(0)[0]).toContain(
523
- 'Warning: %s\n\nThe fix is to split the update',
524
- );
525
- expect(console.error.calls.argsFor(0)[1]).toContain(
526
- 'A user-blocking update was suspended by:',
527
- );
528
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
529
- }
519
});
520
521
it('forces an expiration after an update times out', async () => {
@@ -671,22 +660,9 @@ describe('ReactSuspenseWithNoopRenderer', () => {
660
expect(Scheduler).toHaveYielded(['Promise resolved [Async]']);
661
expect(Scheduler).toFlushAndYield(['Async']);
662
expect(ReactNoop.getChildren()).toEqual([span('Async'), span('Sync')]);
674
-
675
- if (__DEV__) {
676
- expect(console.error).toHaveBeenCalledTimes(1);
677
- expect(console.error.calls.argsFor(0)[0]).toContain(
678
- 'Warning: %s\n\nThe fix is to split the update',
679
- );
680
- expect(console.error.calls.argsFor(0)[1]).toContain(
681
- 'A user-blocking update was suspended by:',
682
- );
683
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
684
- }
663
});
664
665
it('suspending inside an expired expiration boundary will bubble to the next one', async () => {
688
- spyOnDev(console, 'error');
689
-
666
ReactNoop.flushSync(() =>
667
ReactNoop.render(
668
<Fragment>
@@ -707,17 +683,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
683
]);
684
// The tree commits synchronously
685
expect(ReactNoop.getChildren()).toEqual([span('Loading (outer)...')]);
710
-
711
- if (__DEV__) {
712
- expect(console.error).toHaveBeenCalledTimes(1);
713
- expect(console.error.calls.argsFor(0)[0]).toContain(
714
- 'Warning: %s\n\nThe fix is to split the update',
715
- );
716
- expect(console.error.calls.argsFor(0)[1]).toContain(
717
- 'A user-blocking update was suspended by:',
718
- );
719
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
720
- }
686
});
687
688
it('expires early by default', async () => {
@@ -795,21 +760,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
760
});
761
762
it('throws a helpful error when an update is suspends without a placeholder', () => {
798
- spyOnDev(console, 'error');
763
ReactNoop.render(<AsyncText ms={1000} text="Async" />);
764
expect(Scheduler).toFlushAndThrow(
765
'AsyncText suspended while rendering, but no fallback UI was specified.',
766
);
803
- if (__DEV__) {
804
- expect(console.error).toHaveBeenCalledTimes(2);
805
- expect(console.error.calls.argsFor(0)[0]).toContain(
806
- 'Warning: %s\n\nThe fix is to split the update',
807
- );
808
- expect(console.error.calls.argsFor(0)[1]).toContain(
809
- 'A user-blocking update was suspended by:',
810
- );
811
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
812
- }
767
});
768
769
it('a Suspense component correctly handles more than one suspended child', async () => {
@@ -1637,18 +1591,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1591
Scheduler.unstable_advanceTime(100);
1592
await advanceTimers(100);
1593
1640
- expect(() => {
1641
- expect(Scheduler).toFlushAndYield([
1642
- // A suspends
1643
- 'Suspend! [A]',
1644
- 'Loading...',
1645
- ]);
1646
- }).toWarnDev(
1647
- 'Warning: A user-blocking update was suspended by:' +
1648
- '\n\n' +
1649
- ' AsyncText',
1650
- {withoutStack: true},
1651
- );
1594
+ expect(Scheduler).toFlushAndYield([
1595
+ // A suspends
1596
+ 'Suspend! [A]',
1597
+ 'Loading...',
1598
+ ]);
1599
1600
// We're now suspended and we haven't shown anything yet.
1601
expect(ReactNoop.getChildren()).toEqual([]);
@@ -1689,13 +1636,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1636
);
1637
});
1638
}).toWarnDev(
1692
- 'Warning: The following components triggered a user-blocking update:' +
1693
- '\n\n' +
1694
- ' App' +
1695
- '\n\n' +
1696
- 'that was then suspended by:' +
1697
- '\n\n' +
1698
- ' AsyncText',
1639
+ 'Warning: App triggered a user-blocking update that suspended.' + '\n\n',
1640
{withoutStack: true},
1641
);
1642
});
@@ -1727,59 +1668,78 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1668
);
1669
});
1670
}).toWarnDev(
1730
- 'Warning: The following components triggered a user-blocking update:' +
1731
- '\n\n' +
1732
- ' App' +
1733
- '\n\n' +
1734
- 'that was then suspended by:' +
1735
- '\n\n' +
1736
- ' AsyncText',
1671
+ 'Warning: App triggered a user-blocking update that suspended.' + '\n\n',
1672
{withoutStack: true},
1673
);
1674
});
1675
1741
- it('warns when suspending inside discrete update', async () => {
1742
- function A() {
1743
- Scheduler.unstable_yieldValue('A');
1744
- TextResource.read(['A', 1000]);
1745
- return 'A';
1746
- }
1676
+ it('does not warn about wrong Suspense priority if no new fallbacks are shown', async () => {
1677
+ let showB;
1678
+ class App extends React.Component {
1679
+ state = {showB: false};
1680
1748
- function B() {
1749
- return 'B';
1681
+ render() {
1682
+ showB = () => this.setState({showB: true});
1683
+ return (
1684
+ <Suspense fallback="Loading...">
1685
+ {<AsyncText text="A" />}
1686
+ {this.state.showB && <AsyncText text="B" />}
1687
+ </Suspense>
1688
+ );
1689
+ }
1690
}
1691
1752
- function C() {
1753
- TextResource.read(['C', 1000]);
1754
- return 'C';
1755
- }
1692
+ await ReactNoop.act(async () => {
1693
+ ReactNoop.render(<App />);
1694
+ });
1695
1757
- function App() {
1758
- return (
1759
- <Suspense fallback="Loading...">
1760
- <A />
1761
- <B />
1762
- <C />
1763
- <C />
1764
- <C />
1765
- <C />
1766
- </Suspense>
1696
+ expect(Scheduler).toHaveYielded(['Suspend! [A]']);
1697
+ expect(ReactNoop).toMatchRenderedOutput('Loading...');
1698
+
1699
+ ReactNoop.act(() => {
1700
+ Scheduler.unstable_runWithPriority(
1701
+ Scheduler.unstable_UserBlockingPriority,
1702
+ () => showB(),
1703
);
1768
- }
1704
+ });
1705
1770
- ReactNoop.discreteUpdates(() => ReactNoop.render(<App />));
1771
- expect(Scheduler).toFlushAndYieldThrough(['A']);
1706
+ expect(Scheduler).toHaveYielded(['Suspend! [A]', 'Suspend! [B]']);
1707
+ });
1708
1773
- // Warning is not flushed until the commit phase
1709
+ it(
1710
+ 'warns when component that triggered user-blocking update is between Suspense boundary ' +
1711
+ 'and component that suspended',
1712
+ async () => {
1713
+ let _setShow;
1714
+ function A() {
1715
+ const [show, setShow] = React.useState(false);
1716
+ _setShow = setShow;
1717
+ return show && <AsyncText text="A" />;
1718
+ }
1719
+ function App() {
1720
+ return (
1721
+ <Suspense fallback="Loading...">
1722
+ <A />
1723
+ </Suspense>
1724
+ );
1725
+ }
1726
+ await ReactNoop.act(async () => {
1727
+ ReactNoop.render(<App />);
1728
+ });
1729
1775
- // Timeout and commit the fallback
1776
- expect(() => {
1777
- Scheduler.unstable_flushAll();
1778
- }).toWarnDev(
1779
- 'Warning: A user-blocking update was suspended by:' + '\n\n' + ' A, C',
1780
- {withoutStack: true},
1781
- );
1782
- });
1730
+ expect(() => {
1731
+ ReactNoop.act(() => {
1732
+ Scheduler.unstable_runWithPriority(
1733
+ Scheduler.unstable_UserBlockingPriority,
1734
+ () => _setShow(true),
1735
+ );
1736
+ });
1737
+ }).toWarnDev(
1738
+ 'Warning: A triggered a user-blocking update that suspended.' + '\n\n',
1739
+ {withoutStack: true},
1740
+ );
1741
+ },
1742
+ );
1743
1744
it('normal priority updates suspending do not warn for class components', async () => {
1745
let show;
packages/react/src/__tests__/ReactProfiler-test.internal.js
-12
@@ -2626,7 +2626,6 @@ describe('Profiler', () => {
2626
});
2627
2628
it('handles high-pri renderers between suspended and resolved (async) trees', async () => {
2629
- spyOnDev(console, 'error');
2629
// Set up an initial shell. We need to set this up before the test sceanrio
2630
// because we want initial render to suspend on navigation to the initial state.
2631
let renderer = ReactTestRenderer.create(
@@ -2729,17 +2728,6 @@ describe('Profiler', () => {
2728
expect(
2729
onInteractionScheduledWorkCompleted.mock.calls[1][0],
2730
).toMatchInteraction(highPriUpdateInteraction);
2732
-
2733
- if (__DEV__) {
2734
- expect(console.error).toHaveBeenCalledTimes(1);
2735
- expect(console.error.calls.argsFor(0)[0]).toContain(
2736
- 'Warning: %s\n\nThe fix is to split the update',
2737
- );
2738
- expect(console.error.calls.argsFor(0)[1]).toContain(
2739
- 'A user-blocking update was suspended by:',
2740
- );
2741
- expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
2742
- }
2731
});
2732
});
2733
});