Skip to content

Commit

Permalink
Expand act warning to include Suspense resolutions
Browse files Browse the repository at this point in the history 
For the same reason we warn when an update is not wrapped with act,
we should warn if a Suspense promise resolution is not wrapped with act.
Both "pings" and "retries".

Legacy root behavior is unchanged.
  • Loading branch information
@acdlite
acdlite committed Oct 20, 2021
1 parent 986274e commit 21cc354
Show file tree
Hide file tree
Showing 5 changed files with 243 additions and 9 deletions.
6 changes: 3 additions & 3 deletions ...-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].promiseName).toBe('Testing displayName');
}
Expand Down Expand Up @@ -1682,7 +1682,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].warning).toMatchInlineSnapshot(
`"A component suspended during an update which caused a fallback to be shown. Consider using the Transition API to avoid hiding components after they've been mounted."`,
Expand Down Expand Up @@ -1740,7 +1740,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].warning).toBe(null);
}
Expand Down
26 changes: 26 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2368,6 +2368,8 @@ export function pingSuspendedRoot(
const eventTime = requestEventTime();
markRootPinged(root, pingedLanes, eventTime);

warnIfSuspenseResolutionNotWrappedWithActDEV(root);

if (
workInProgressRoot === root &&
isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes)
Expand Down Expand Up @@ -2902,3 +2904,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
}
}
}

function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
if (__DEV__) {
if (
root.tag !== LegacyRoot &&
isConcurrentActEnvironment() &&
ReactCurrentActQueue.current === null
) {
console.error(
'A suspended resource finished loading inside a test, but the event ' +
'was not wrapped in act(...).\n\n' +
'When testing, code that resolves suspended data should be wrapped ' +
'into act(...):\n\n' +
'act(() => {\n' +
' /* finish loading suspended data */\n' +
'});\n' +
'/* assert on the output */\n\n' +
"This ensures that you're testing the behavior the user would see " +
'in the browser.' +
' Learn more at https://reactjs.org/link/wrap-tests-with-act',
);
}
}
}
26 changes: 26 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2368,6 +2368,8 @@ export function pingSuspendedRoot(
const eventTime = requestEventTime();
markRootPinged(root, pingedLanes, eventTime);

warnIfSuspenseResolutionNotWrappedWithActDEV(root);

if (
workInProgressRoot === root &&
isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes)
Expand Down Expand Up @@ -2902,3 +2904,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
}
}
}

function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
if (__DEV__) {
if (
root.tag !== LegacyRoot &&
isConcurrentActEnvironment() &&
ReactCurrentActQueue.current === null
) {
console.error(
'A suspended resource finished loading inside a test, but the event ' +
'was not wrapped in act(...).\n\n' +
'When testing, code that resolves suspended data should be wrapped ' +
'into act(...):\n\n' +
'act(() => {\n' +
' /* finish loading suspended data */\n' +
'});\n' +
'/* assert on the output */\n\n' +
"This ensures that you're testing the behavior the user would see " +
'in the browser.' +
' Learn more at https://reactjs.org/link/wrap-tests-with-act',
);
}
}
}
17 changes: 14 additions & 3 deletions packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,20 @@ describe('DebugTracing', () => {

// @gate experimental && build === 'development' && enableDebugTracing
it('should log concurrent render with suspense', async () => {
const fakeSuspensePromise = Promise.resolve(true);
let isResolved = false;
let resolveFakeSuspensePromise;
const fakeSuspensePromise = new Promise(resolve => {
resolveFakeSuspensePromise = () => {
resolve();
isResolved = true;
};
});

function Example() {
throw fakeSuspensePromise;
if (!isResolved) {
throw fakeSuspensePromise;
}
return null;
}

ReactTestRenderer.act(() =>
Expand All @@ -164,7 +175,7 @@ describe('DebugTracing', () => {

logs.splice(0);

await fakeSuspensePromise;
await ReactTestRenderer.act(async () => await resolveFakeSuspensePromise());
expect(logs).toEqual(['log: ⚛️ Example resolved']);
});

Expand Down
177 changes: 174 additions & 3 deletions packages/react-reconciler/src/__tests__/ReactActWarnings-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ let Scheduler;
let ReactNoop;
let useState;
let act;
let Suspense;
let startTransition;
let getCacheForType;
let caches;

// These tests are mostly concerned with concurrent roots. The legacy root
// behavior is covered by other older test suites and is unchanged from
Expand All @@ -24,11 +28,110 @@ describe('act warnings', () => {
ReactNoop = require('react-noop-renderer');
act = React.unstable_act;
useState = React.useState;
Suspense = React.Suspense;
startTransition = React.startTransition;
getCacheForType = React.unstable_getCacheForType;
caches = [];
});

function Text(props) {
Scheduler.unstable_yieldValue(props.text);
return props.text;
function createTextCache() {
const data = new Map();
const version = caches.length + 1;
const cache = {
version,
data,
resolve(text) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'resolved',
value: text,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'resolved';
record.value = text;
thenable.pings.forEach(t => t());
}
},
reject(text, error) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'rejected',
value: error,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'rejected';
record.value = error;
thenable.pings.forEach(t => t());
}
},
};
caches.push(cache);
return cache;
}

function readText(text) {
const textCache = getCacheForType(createTextCache);
const record = textCache.data.get(text);
if (record !== undefined) {
switch (record.status) {
case 'pending':
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
throw record.value;
case 'rejected':
Scheduler.unstable_yieldValue(`Error! [${text}]`);
throw record.value;
case 'resolved':
return textCache.version;
}
} else {
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);

const thenable = {
pings: [],
then(resolve) {
if (newRecord.status === 'pending') {
thenable.pings.push(resolve);
} else {
Promise.resolve().then(() => resolve(newRecord.value));
}
},
};

const newRecord = {
status: 'pending',
value: thenable,
};
textCache.data.set(text, newRecord);

throw thenable;
}
}

function Text({text}) {
Scheduler.unstable_yieldValue(text);
return text;
}

function AsyncText({text}) {
readText(text);
Scheduler.unstable_yieldValue(text);
return text;
}

function resolveText(text) {
if (caches.length === 0) {
throw Error('Cache does not exist.');
} else {
// Resolve the most recently created cache. An older cache can by
// resolved with `caches[index].resolve(text)`.
caches[caches.length - 1].resolve(text);
}
}

function withActEnvironment(value, scope) {
Expand Down Expand Up @@ -187,4 +290,72 @@ describe('act warnings', () => {
expect(root).toMatchRenderedOutput('1');
});
});

// @gate __DEV__
// @gate enableCache
test('warns if Suspense retry is not wrapped', () => {
function App() {
return (
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText text="Async" />
</Suspense>
);
}

withActEnvironment(true, () => {
const root = ReactNoop.createRoot();
act(() => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
expect(root).toMatchRenderedOutput('Loading...');

// This is a retry, not a ping, because we already showed a fallback.
expect(() =>
resolveText('Async'),
).toErrorDev(
'A suspended resource finished loading inside a test, but the event ' +
'was not wrapped in act(...)',
{withoutStack: true},
);
});
});

// @gate __DEV__
// @gate enableCache
test('warns if Suspense ping is not wrapped', () => {
function App({showMore}) {
return (
<Suspense fallback={<Text text="Loading..." />}>
{showMore ? <AsyncText text="Async" /> : <Text text="(empty)" />}
</Suspense>
);
}

withActEnvironment(true, () => {
const root = ReactNoop.createRoot();
act(() => {
root.render(<App showMore={false} />);
});
expect(Scheduler).toHaveYielded(['(empty)']);
expect(root).toMatchRenderedOutput('(empty)');

act(() => {
startTransition(() => {
root.render(<App showMore={true} />);
});
});
expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
expect(root).toMatchRenderedOutput('(empty)');

// This is a ping, not a retry, because no fallback is showing.
expect(() =>
resolveText('Async'),
).toErrorDev(
'A suspended resource finished loading inside a test, but the event ' +
'was not wrapped in act(...)',
{withoutStack: true},
);
});
});
});

0 comments on commit 21cc354

Please sign in to comment.