Skip to content

Commit

Permalink
Remove Suspense priority warning (#17971)
Browse files Browse the repository at this point in the history 
* Remove Suspense priority warning

* Fix tests
  • Loading branch information
@gaearon
gaearon authored Feb 4, 2020
1 parent 812277d commit d6e08fe
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 166 deletions.
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberThrow.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import {
markLegacyErrorBoundaryAsFailed,
isAlreadyFailedLegacyErrorBoundary,
pingSuspendedRoot,
checkForWrongSuspensePriorityInDEV,
} from './ReactFiberWorkLoop';

import {Sync} from './ReactFiberExpirationTime';
Expand Down Expand Up @@ -207,8 +206,6 @@ function throwException(
}
}

checkForWrongSuspensePriorityInDEV(sourceFiber);

let hasInvisibleParentBoundary = hasSuspenseContext(
suspenseStackCursor.current,
(InvisibleParentSuspenseContext: SuspenseContext),
Expand Down
129 changes: 2 additions & 127 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
import type {Interaction} from 'scheduler/src/Tracing';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {SuspenseState} from './ReactFiberSuspenseComponent';
import type {Hook} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -779,7 +778,6 @@ function finishConcurrentRender(
if (expirationTime === lastSuspendedTime) {
root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
}
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 @@ -855,7 +853,6 @@ function finishConcurrentRender(
if (expirationTime === lastSuspendedTime) {
root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
}
flushSuspensePriorityWarningInDEV();

if (
// do not delay if we're inside an act() scope
Expand Down Expand Up @@ -1051,7 +1048,7 @@ function performSyncWorkOnRoot(root) {
stopFinishedWorkLoopTimer();
root.finishedWork = (root.current.alternate: any);
root.finishedExpirationTime = expirationTime;
finishSyncRender(root, workInProgressRootExitStatus, expirationTime);
finishSyncRender(root);
}

// Before exiting, make sure there's a callback scheduled for the next
Expand All @@ -1062,15 +1059,9 @@ function performSyncWorkOnRoot(root) {
return null;
}

function finishSyncRender(root, exitStatus, expirationTime) {
function finishSyncRender(root) {
// Set this to null to indicate there's no in-progress render.
workInProgressRoot = null;

if (__DEV__) {
if (exitStatus === RootSuspended || exitStatus === RootSuspendedWithDelay) {
flushSuspensePriorityWarningInDEV();
}
}
commitRoot(root);
}

Expand Down Expand Up @@ -1274,7 +1265,6 @@ function prepareFreshStack(root, expirationTime) {

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

Expand Down Expand Up @@ -2859,121 +2849,6 @@ export function warnIfUnmockedScheduler(fiber: Fiber) {
}
}

let componentsThatTriggeredHighPriSuspend = null;
export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
if (__DEV__) {
const currentPriorityLevel = getCurrentPriorityLevel();
if (
(sourceFiber.mode & ConcurrentMode) !== NoEffect &&
(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.baseQueue;
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:
case Chunk: {
let firstHook: null | Hook = current.memoizedState;
// TODO: This just checks the first Hook. Isn't it suppose to check all Hooks?
if (firstHook !== null && firstHook.baseQueue !== null) {
let update = firstHook.baseQueue;
// 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 === firstHook.baseQueue) {
break;
}
update = update.next;
}
}
break;
}
default:
break;
}
}
workInProgressNode = workInProgressNode.return;
}
}
}
}

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

if (componentNames.length > 0) {
console.error(
'%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 useTransition to learn how ' +
'to implement this pattern.', // TODO: Add link to React docs with more information, once it exists
componentNames.sort().join(', '),
);
}
}
}
}

function computeThreadID(root, expirationTime) {
// Interaction threads are unique per root and expiration time.
return expirationTime * 1000 + root.interactionThreadID;
Expand Down
63 changes: 27 additions & 36 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1867,7 +1867,8 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
});

it('warns when a low priority update suspends inside a high priority update for functional components', async () => {
// TODO: flip to "warns" when this is implemented again.
it('does not warn when a low priority update suspends inside a high priority update for functional components', async () => {
let _setShow;
function App() {
let [show, setShow] = React.useState(false);
Expand All @@ -1883,20 +1884,17 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactNoop.render(<App />);
});

expect(() => {
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => _setShow(true),
);
});
}).toErrorDev(
'Warning: App triggered a user-blocking update that suspended.' + '\n\n',
{withoutStack: true},
);
// TODO: assert toErrorDev() when the warning is implemented again.
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => _setShow(true),
);
});
});

it('warns when a low priority update suspends inside a high priority update for class components', async () => {
// TODO: flip to "warns" when this is implemented again.
it('does not warn when a low priority update suspends inside a high priority update for class components', async () => {
let show;
class App extends React.Component {
state = {show: false};
Expand All @@ -1915,17 +1913,13 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactNoop.render(<App />);
});

expect(() => {
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => show(),
);
});
}).toErrorDev(
'Warning: App triggered a user-blocking update that suspended.' + '\n\n',
{withoutStack: true},
);
// TODO: assert toErrorDev() when the warning is implemented again.
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => show(),
);
});
});

it('does not warn about wrong Suspense priority if no new fallbacks are shown', async () => {
Expand Down Expand Up @@ -1961,8 +1955,9 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded(['Suspend! [A]', 'Suspend! [B]']);
});

// TODO: flip to "warns" when this is implemented again.
it(
'warns when component that triggered user-blocking update is between Suspense boundary ' +
'does not warn when component that triggered user-blocking update is between Suspense boundary ' +
'and component that suspended',
async () => {
let _setShow;
Expand All @@ -1982,17 +1977,13 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactNoop.render(<App />);
});

expect(() => {
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => _setShow(true),
);
});
}).toErrorDev(
'Warning: A triggered a user-blocking update that suspended.' + '\n\n',
{withoutStack: true},
);
// TODO: assert toErrorDev() when the warning is implemented again.
ReactNoop.act(() => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => _setShow(true),
);
});
},
);

Expand Down

0 comments on commit d6e08fe

Please sign in to comment.