Skip to content

Commit

Permalink
Update Suspense Priority Warning to Include Component that Triggered …
Browse files Browse the repository at this point in the history 
…Update (#16030)

Improved warning whenever lower priority events (ex. data fetching, page load) happen during a high priority update (ex. hover/click events) to include:
1.) Name of component that triggered the high priority update or
2.) Information that the update was triggered on the root
  • Loading branch information
@lunaruan
lunaruan committed Jul 22, 2019
1 parent 3f2cafe commit 03944bf
Show file tree
Hide file tree
Showing 6 changed files with 319 additions and 49 deletions.
11 changes: 11 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {HookEffectTag} from './ReactHookEffectTags';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

import ReactSharedInternals from 'shared/ReactSharedInternals';

Expand Down Expand Up @@ -48,6 +49,7 @@ import is from 'shared/objectIs';
import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork';
import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags';
import {requestCurrentSuspenseConfig} from './ReactFiberSuspenseConfig';
import {getCurrentPriorityLevel} from './SchedulerWithReactIntegration';

const {ReactCurrentDispatcher} = ReactSharedInternals;

Expand Down Expand Up @@ -96,6 +98,8 @@ type Update<S, A> = {
eagerReducer: ((S, A) => S) | null,
eagerState: S | null,
next: Update<S, A> | null,

priority?: ReactPriorityLevel,
};

type UpdateQueue<S, A> = {
Expand Down Expand Up @@ -1140,6 +1144,9 @@ function dispatchAction<S, A>(
eagerState: null,
next: null,
};
if (__DEV__) {
update.priority = getCurrentPriorityLevel();
}
if (renderPhaseUpdates === null) {
renderPhaseUpdates = new Map();
}
Expand Down Expand Up @@ -1176,6 +1183,10 @@ function dispatchAction<S, A>(
next: null,
};

if (__DEV__) {
update.priority = getCurrentPriorityLevel();
}

// Append the update to the end of the list.
const last = queue.last;
if (last === null) {
Expand Down
142 changes: 110 additions & 32 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,8 @@ function prepareFreshStack(root, expirationTime) {

if (__DEV__) {
ReactStrictModeWarnings.discardPendingWarnings();
componentsWithSuspendedDiscreteUpdates = null;
componentsThatSuspendedAtHighPri = null;
componentsThatTriggeredHighPriSuspend = null;
}
}

Expand Down Expand Up @@ -982,6 +983,8 @@ function renderRoot(
// Set this to null to indicate there's no in-progress render.
workInProgressRoot = null;

flushSuspensePriorityWarningInDEV();

switch (workInProgressRootExitStatus) {
case RootIncomplete: {
invariant(false, 'Should have a work-in-progress.');
Expand Down Expand Up @@ -1491,7 +1494,6 @@ function commitRoot(root) {
function commitRootImpl(root) {
flushPassiveEffects();
flushRenderPhaseStrictModeWarningsInDEV();
flushSuspensePriorityWarningInDEV();

invariant(
(executionContext & (RenderContext | CommitContext)) === NoContext,
Expand Down Expand Up @@ -2520,58 +2522,133 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {

export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV;

let componentsWithSuspendedDiscreteUpdates = null;
let componentsThatSuspendedAtHighPri = null;
let componentsThatTriggeredHighPriSuspend = null;
export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
if (__DEV__) {
const currentPriorityLevel = getCurrentPriorityLevel();
if (
(sourceFiber.mode & ConcurrentMode) !== NoEffect &&
// Check if we're currently rendering a discrete update. 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.
rootsWithPendingDiscreteUpdates !== null &&
workInProgressRoot !== null &&
renderExpirationTime ===
rootsWithPendingDiscreteUpdates.get(workInProgressRoot)
(currentPriorityLevel === UserBlockingPriority ||
currentPriorityLevel === ImmediatePriority)
) {
let workInProgressNode = sourceFiber;
while (workInProgressNode !== null) {
// Add the component that triggered the suspense
const current = workInProgressNode.alternate;
if (current !== null) {
// TODO: warn component that triggers the high priority
// suspend is the HostRoot
switch (workInProgressNode.tag) {
case ClassComponent:
// Loop through the component's update queue and see whether the component
// has triggered any high priority updates
const updateQueue = current.updateQueue;
if (updateQueue !== null) {
let update = updateQueue.firstUpdate;
while (update !== null) {
const priorityLevel = update.priority;
if (
priorityLevel === UserBlockingPriority ||
priorityLevel === ImmediatePriority
) {
if (componentsThatTriggeredHighPriSuspend === null) {
componentsThatTriggeredHighPriSuspend = new Set([
getComponentName(workInProgressNode.type),
]);
} else {
componentsThatTriggeredHighPriSuspend.add(
getComponentName(workInProgressNode.type),
);
}
break;
}
update = update.next;
}
}
break;
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
if (
workInProgressNode.memoizedState !== null &&
workInProgressNode.memoizedState.baseUpdate !== null
) {
let update = workInProgressNode.memoizedState.baseUpdate;
// Loop through the functional component's memoized state to see whether
// the component has triggered any high pri updates
while (update !== null) {
const priority = update.priority;
if (
priority === UserBlockingPriority ||
priority === ImmediatePriority
) {
if (componentsThatTriggeredHighPriSuspend === null) {
componentsThatTriggeredHighPriSuspend = new Set([
getComponentName(workInProgressNode.type),
]);
} else {
componentsThatTriggeredHighPriSuspend.add(
getComponentName(workInProgressNode.type),
);
}
break;
}
if (
update.next === workInProgressNode.memoizedState.baseUpdate
) {
break;
}
update = update.next;
}
}
break;
default:
break;
}
}
workInProgressNode = workInProgressNode.return;
}

// Add the component name to a set.
const componentName = getComponentName(sourceFiber.type);
if (componentsWithSuspendedDiscreteUpdates === null) {
componentsWithSuspendedDiscreteUpdates = new Set([componentName]);
if (componentsThatSuspendedAtHighPri === null) {
componentsThatSuspendedAtHighPri = new Set([componentName]);
} else {
componentsWithSuspendedDiscreteUpdates.add(componentName);
componentsThatSuspendedAtHighPri.add(componentName);
}
}
}
}

function flushSuspensePriorityWarningInDEV() {
if (__DEV__) {
if (componentsWithSuspendedDiscreteUpdates !== null) {
if (componentsThatSuspendedAtHighPri !== null) {
const componentNames = [];
componentsWithSuspendedDiscreteUpdates.forEach(name => {
componentsThatSuspendedAtHighPri.forEach(name => {
componentNames.push(name);
});
componentsWithSuspendedDiscreteUpdates = null;
componentsThatSuspendedAtHighPri = null;

const componentsThatTriggeredSuspendNames = [];
if (componentsThatTriggeredHighPriSuspend !== null) {
componentsThatTriggeredHighPriSuspend.forEach(name =>
componentsThatTriggeredSuspendNames.push(name),
);
}

componentsThatTriggeredHighPriSuspend = null;

// TODO: A more helpful version of this message could include the names of
// the component that were updated, not the ones that suspended. To do
// that we'd need to track all the components that updated during this
// render, perhaps using the same mechanism as `markRenderEventTime`.
const componentThatTriggeredSuspenseError =
componentsThatTriggeredSuspendNames.length > 0
? '\n' +
'The components that triggered the update: ' +
componentsThatTriggeredSuspendNames.sort().join(', ')
: '';
warningWithoutStack(
false,
'The following components suspended during a user-blocking update: %s' +
'%s' +
'\n\n' +
'Updates triggered by user interactions (e.g. click events) are ' +
'considered user-blocking by default. They should not suspend. ' +
Expand All @@ -2585,6 +2662,7 @@ function flushSuspensePriorityWarningInDEV() {
'feedback, and another update to perform the actual change.',
// TODO: Add link to React docs with more information, once it exists
componentNames.sort().join(', '),
componentThatTriggeredSuspenseError,
);
}
}
Expand Down
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactUpdateQueue.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

import {NoWork} from './ReactFiberExpirationTime';
import {
Expand All @@ -106,6 +107,7 @@ import {markRenderEventTimeAndConfig} from './ReactFiberWorkLoop';

import invariant from 'shared/invariant';
import warningWithoutStack from 'shared/warningWithoutStack';
import {getCurrentPriorityLevel} from './SchedulerWithReactIntegration';

export type Update<State> = {
expirationTime: ExpirationTime,
Expand All @@ -117,6 +119,9 @@ export type Update<State> = {

next: Update<State> | null,
nextEffect: Update<State> | null,

//DEV only
priority?: ReactPriorityLevel,
};

export type UpdateQueue<State> = {
Expand Down Expand Up @@ -197,7 +202,7 @@ export function createUpdate(
expirationTime: ExpirationTime,
suspenseConfig: null | SuspenseConfig,
): Update<*> {
return {
let update: Update<*> = {
expirationTime,
suspenseConfig,

Expand All @@ -208,6 +213,10 @@ export function createUpdate(
next: null,
nextEffect: null,
};
if (__DEV__) {
update.priority = getCurrentPriorityLevel();
}
return update;
}

function appendUpdateToQueue<State>(
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ describe('ReactSuspense', () => {
});

it('throws if tree suspends and none of the Suspense ancestors have a fallback', () => {
spyOnDev(console, 'error');
ReactTestRenderer.create(
<Suspense>
<AsyncText text="Hi" ms={1000} />
Expand All @@ -340,6 +341,13 @@ describe('ReactSuspense', () => {
'AsyncText suspended while rendering, but no fallback UI was specified.',
);
expect(Scheduler).toHaveYielded(['Suspend! [Hi]', 'Suspend! [Hi]']);
if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(2);
expect(console.error.calls.argsFor(0)[0]).toContain(
'Warning: The following components suspended during a user-blocking update: ',
);
expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
}
});

describe('outside concurrent mode', () => {
Expand Down
Loading

0 comments on commit 03944bf

Please sign in to comment.