Warn when suspending at wrong priority (#15492)
* Warn when suspending at wrong priority Adds a warning when a user-blocking update is suspended. Ideally, all we would need to do is check the current priority level. But we currently have no rigorous way to distinguish work that was scheduled at user- blocking priority from work that expired a bit and was "upgraded" to a higher priority. That's because we don't schedule separate callbacks for every level, only the highest priority level per root. The priority of subsequent levels is inferred from the expiration time, but this is an imprecise heuristic. However, we do store the last discrete pending update per root. So we can reliably compare to that one. (If we broaden this warning to include high pri updates that aren't discrete, then this won't be sufficient.) My rationale is that it's better for this warning to have false negatives than false positives. Potential follow-ups: - Bikeshed more on the message. I don't like what I landed on that much but I think it's good enough to start. - Include the names of the components that updated. (The ideal place to fire the warning is during the setState call but we don't know if something will suspend until the next update. Maybe we could be clever and warn during a subsequent update to the same component?) * Move Suspense priority check to throwException
Andrew Clark committed
May 7, 2019 at 16:50 UTC
f9e60c8a197b8fbdaebfe4b2a9bcbcd319ea3dd5
3 files changed
+125
-1
packages/react-reconciler/src/ReactFiberScheduler.js
+72
@@ -698,6 +698,7 @@ function prepareFreshStack(root, expirationTime) {
698
699
if (__DEV__) {
700
ReactStrictModeWarnings.discardPendingWarnings();
701
+ componentsWithSuspendedDiscreteUpdates = null;
702
}
703
}
704
@@ -1225,6 +1226,7 @@ function commitRoot(root, expirationTime) {
1226
function commitRootImpl(root, expirationTime) {
1227
flushPassiveEffects();
1228
flushRenderPhaseStrictModeWarningsInDEV();
1229
+ flushSuspensePriorityWarningInDEV();
1230
1231
invariant(
1232
workPhase !== RenderPhase && workPhase !== CommitPhase,
@@ -2114,6 +2116,76 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
2116
2117
export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV;
2118
2119
+let componentsWithSuspendedDiscreteUpdates = null;
2120
+export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
2121
+ if (__DEV__) {
2122
+ if (
2123
+ (sourceFiber.mode & ConcurrentMode) !== NoEffect &&
2124
+ // Check if we're currently rendering a discrete update. Ideally, all we
2125
+ // would need to do is check the current priority level. But we currently
2126
+ // have no rigorous way to distinguish work that was scheduled at user-
2127
+ // blocking priority from work that expired a bit and was "upgraded" to
2128
+ // a higher priority. That's because we don't schedule separate callbacks
2129
+ // for every level, only the highest priority level per root. The priority
2130
+ // of subsequent levels is inferred from the expiration time, but this is
2131
+ // an imprecise heuristic.
2132
+ //
2133
+ // However, we do store the last discrete pending update per root. So we
2134
+ // can reliably compare to that one. (If we broaden this warning to include
2135
+ // high pri updates that aren't discrete, then this won't be sufficient.)
2136
+ //
2137
+ // My rationale is that it's better for this warning to have false
2138
+ // negatives than false positives.
2139
+ rootsWithPendingDiscreteUpdates !== null &&
2140
+ workInProgressRoot !== null &&
2141
+ renderExpirationTime ===
2142
+ rootsWithPendingDiscreteUpdates.get(workInProgressRoot)
2143
+ ) {
2144
+ // Add the component name to a set.
2145
+ const componentName = getComponentName(sourceFiber.type);
2146
+ if (componentsWithSuspendedDiscreteUpdates === null) {
2147
+ componentsWithSuspendedDiscreteUpdates = new Set([componentName]);
2148
+ } else {
2149
+ componentsWithSuspendedDiscreteUpdates.add(componentName);
2150
+ }
2151
+ }
2152
+ }
2153
+}
2154
+
2155
+function flushSuspensePriorityWarningInDEV() {
2156
+ if (__DEV__) {
2157
+ if (componentsWithSuspendedDiscreteUpdates !== null) {
2158
+ const componentNames = [];
2159
+ componentsWithSuspendedDiscreteUpdates.forEach(name => {
2160
+ componentNames.push(name);
2161
+ });
2162
+ componentsWithSuspendedDiscreteUpdates = null;
2163
+
2164
+ // TODO: A more helpful version of this message could include the names of
2165
+ // the component that were updated, not the ones that suspended. To do
2166
+ // that we'd need to track all the components that updated during this
2167
+ // render, perhaps using the same mechanism as `markRenderEventTime`.
2168
+ warningWithoutStack(
2169
+ false,
2170
+ 'The following components suspended during a user-blocking update: %s' +
2171
+ '\n\n' +
2172
+ 'Updates triggered by user interactions (e.g. click events) are ' +
2173
+ 'considered user-blocking by default. They should not suspend. ' +
2174
+ 'Updates that can afford to take a bit longer should be wrapped ' +
2175
+ 'with `Scheduler.next` (or an equivalent abstraction). This ' +
2176
+ 'typically includes any update that shows new content, like ' +
2177
+ 'a navigation.' +
2178
+ '\n\n' +
2179
+ 'Generally, you should split user interactions into at least two ' +
2180
+ 'seprate updates: a user-blocking update to provide immediate ' +
2181
+ 'feedback, and another update to perform the actual change.',
2182
+ // TODO: Add link to React docs with more information, once it exists
2183
+ componentNames.sort().join(', '),
2184
+ );
2185
+ }
2186
+ }
2187
+}
2188
+
2189
function computeThreadID(root, expirationTime) {
2190
// Interaction threads are unique per root and expiration time.
2191
return expirationTime * 1000 + root.interactionThreadID;
packages/react-reconciler/src/ReactFiberUnwindWork.js
+3
@@ -68,6 +68,7 @@ import {
68
isAlreadyFailedLegacyErrorBoundary,
69
pingSuspendedRoot,
70
resolveRetryThenable,
71
+ checkForWrongSuspensePriorityInDEV,
72
} from './ReactFiberScheduler';
73
74
import invariant from 'shared/invariant';
@@ -203,6 +204,8 @@ function throwException(
204
// This is a thenable.
205
const thenable: Thenable = (value: any);
206
207
+ checkForWrongSuspensePriorityInDEV(sourceFiber);
208
+
209
// Schedule the nearest Suspense to re-render the timed out view.
210
let workInProgress = returnFiber;
211
do {
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+50
-1
@@ -1715,7 +1715,14 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1715
1716
// Flush some of the time
1717
Scheduler.advanceTime(500);
1718
- await advanceTimers(500);
1718
+ expect(() => {
1719
+ jest.advanceTimersByTime(500);
1720
+ }).toWarnDev(
1721
+ 'The following components suspended during a user-blocking ' +
1722
+ 'update: AsyncText',
1723
+ {withoutStack: true},
1724
+ );
1725
+
1726
// We should have already shown the fallback.
1727
// When we wrote this test, we inferred the start time of high priority
1728
// updates as way earlier in the past. This test ensures that we don't
@@ -1723,4 +1730,46 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1730
expect(Scheduler).toFlushWithoutYielding();
1731
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
1732
});
1733
+
1734
+ it('warns when suspending inside discrete update', async () => {
1735
+ function A() {
1736
+ TextResource.read(['A', 1000]);
1737
+ return 'A';
1738
+ }
1739
+
1740
+ function B() {
1741
+ return 'B';
1742
+ }
1743
+
1744
+ function C() {
1745
+ TextResource.read(['C', 1000]);
1746
+ return 'C';
1747
+ }
1748
+
1749
+ function App() {
1750
+ return (
1751
+ <Suspense fallback="Loading...">
1752
+ <A />
1753
+ <B />
1754
+ <C />
1755
+ <C />
1756
+ <C />
1757
+ <C />
1758
+ </Suspense>
1759
+ );
1760
+ }
1761
+
1762
+ ReactNoop.interactiveUpdates(() => ReactNoop.render(<App />));
1763
+ Scheduler.flushAll();
1764
+
1765
+ // Warning is not flushed until the commit phase
1766
+
1767
+ // Timeout and commit the fallback
1768
+ expect(() => {
1769
+ jest.advanceTimersByTime(1000);
1770
+ }).toWarnDev(
1771
+ 'The following components suspended during a user-blocking update: A, C',
1772
+ {withoutStack: true},
1773
+ );
1774
+ });
1775
});