Skip to content

Commit

Permalink
BugFix: Suspense priority warning firing when not supposed to (#16256)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lunaruan authored Aug 2, 2019
1 parent 05dce75 commit c4c9f08
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 187 deletions.
80 changes: 22 additions & 58 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,6 @@ function prepareFreshStack(root, expirationTime) {

if (__DEV__) {
ReactStrictModeWarnings.discardPendingWarnings();
componentsThatSuspendedAtHighPri = null;
componentsThatTriggeredHighPriSuspend = null;
}
}
Expand Down Expand Up @@ -990,8 +989,6 @@ 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 @@ -1022,6 +1019,8 @@ function renderRoot(
return commitRoot.bind(null, root);
}
case RootSuspended: {
flushSuspensePriorityWarningInDEV();

// We have an acceptable loading state. We need to figure out if we should
// immediately commit it or wait a bit.

Expand Down Expand Up @@ -1076,6 +1075,8 @@ function renderRoot(
return commitRoot.bind(null, root);
}
case RootSuspendedWithDelay: {
flushSuspensePriorityWarningInDEV();

if (
!isSync &&
// do not delay if we're inside an act() scope
Expand Down Expand Up @@ -2610,7 +2611,6 @@ export function warnIfUnmockedScheduler(fiber: Fiber) {
}
}

let componentsThatSuspendedAtHighPri = null;
let componentsThatTriggeredHighPriSuspend = null;
export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
if (__DEV__) {
Expand Down Expand Up @@ -2697,70 +2697,34 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
}
workInProgressNode = workInProgressNode.return;
}

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

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

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

componentsThatTriggeredHighPriSuspend.forEach(name =>
componentNames.push(name),
);
componentsThatTriggeredHighPriSuspend = null;

const componentNamesString = componentNames.sort().join(', ');
let componentThatTriggeredSuspenseError = '';
if (componentsThatTriggeredSuspendNames.length > 0) {
componentThatTriggeredSuspenseError =
'The following components triggered a user-blocking update:' +
'\n\n' +
' ' +
componentsThatTriggeredSuspendNames.sort().join(', ') +
'\n\n' +
'that was then suspended by:' +
'\n\n' +
' ' +
componentNamesString;
} else {
componentThatTriggeredSuspenseError =
'A user-blocking update was suspended by:' +
'\n\n' +
' ' +
componentNamesString;
if (componentNames.length > 0) {
warningWithoutStack(
false,
'%s triggered a user-blocking update that suspended.' +
'\n\n' +
'The fix is to split the update into multiple parts: a user-blocking ' +
'update to provide immediate feedback, and another update that ' +
'triggers the bulk of the changes.' +
'\n\n' +
'Refer to the documentation for useSuspenseTransition to learn how ' +
'to implement this pattern.',
// TODO: Add link to React docs with more information, once it exists
componentNames.sort().join(', '),
);
}

warningWithoutStack(
false,
'%s' +
'\n\n' +
'The fix is to split the update into multiple parts: a user-blocking ' +
'update to provide immediate feedback, and another update that ' +
'triggers the bulk of the changes.' +
'\n\n' +
'Refer to the documentation for useSuspenseTransition to learn how ' +
'to implement this pattern.',
// TODO: Add link to React docs with more information, once it exists
componentThatTriggeredSuspenseError,
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ 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 @@ -341,16 +340,6 @@ 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: %s\n\nThe fix is to split the update',
);
expect(console.error.calls.argsFor(0)[1]).toContain(
'A user-blocking update was suspended by:',
);
expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText');
}
});

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

0 comments on commit c4c9f08

Please sign in to comment.