Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Suspense priority warning #17971

Merged
merged 2 commits into from Feb 4, 2020
Merged

Remove Suspense priority warning #17971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd9fadd
Remove Suspense priority warning
gaearon Feb 4, 2020
57dca4e
Fix tests
gaearon Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberThrow.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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