Skip to content

Commit

Permalink
act should work without mock Scheduler
Browse files Browse the repository at this point in the history
Currently, in a React 18 root, `act` only works if you mock the
Scheduler package. This was because we didn't want to add additional
checks at runtime.

But now that the `act` testing API is dev-only, we can simplify its
implementation.

Now when an update is wrapped with `act`, React will bypass Scheduler
entirely and push its tasks onto a special internal queue. Then, when
the outermost `act` scope exists, we'll flush that queue.

I also removed the "wrong act" warning, because the plan is to move
`act` to an isomorphic entry point, simlar to `startTransition`. That's
not directly related to this PR, but I didn't want to bother
re-implementing that warning only to immediately remove it.

I'll add the isomorphic API in a follow up.

Note that the internal version of `act` that we use in our own tests
still depends on mocking the Scheduler package, because it needs to work
in production. I'm planning to move that implementation to a shared
(internal) module, too.
  • Loading branch information
acdlite committed Jun 22, 2021
1 parent b5debd5 commit 3400ee6
Show file tree
Hide file tree
Showing 25 changed files with 470 additions and 713 deletions.
67 changes: 37 additions & 30 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,32 +354,6 @@ function runActTests(label, render, unmount, rerender) {
expect(container.innerHTML).toBe('2');
});
});

// @gate __DEV__
it('warns if you return a value inside act', () => {
expect(() => act(() => null)).toErrorDev(
[
'The callback passed to act(...) function must return undefined, or a Promise.',
],
{withoutStack: true},
);
expect(() => act(() => 123)).toErrorDev(
[
'The callback passed to act(...) function must return undefined, or a Promise.',
],
{withoutStack: true},
);
});

// @gate __DEV__
it('warns if you try to await a sync .act call', () => {
expect(() => act(() => {}).then(() => {})).toErrorDev(
[
'Do not await the result of calling act(...) with sync logic, it is not a Promise.',
],
{withoutStack: true},
);
});
});

describe('asynchronous tests', () => {
Expand All @@ -401,15 +375,17 @@ function runActTests(label, render, unmount, rerender) {

await act(async () => {
render(<App />, container);
// flush a little to start the timer
expect(Scheduler).toFlushAndYield([]);
});
expect(container.innerHTML).toBe('0');
// Flush the pending timers
await act(async () => {
await sleep(100);
});
expect(container.innerHTML).toBe('1');
});

// @gate __DEV__
it('flushes microtasks before exiting', async () => {
it('flushes microtasks before exiting (async function)', async () => {
function App() {
const [ctr, setCtr] = React.useState(0);
async function someAsyncFunction() {
Expand All @@ -431,6 +407,31 @@ function runActTests(label, render, unmount, rerender) {
expect(container.innerHTML).toEqual('1');
});

// @gate __DEV__
it('flushes microtasks before exiting (sync function)', async () => {
// Same as previous test, but the callback passed to `act` is not itself
// an async function.
function App() {
const [ctr, setCtr] = React.useState(0);
async function someAsyncFunction() {
// queue a bunch of promises to be sure they all flush
await null;
await null;
await null;
setCtr(1);
}
React.useEffect(() => {
someAsyncFunction();
}, []);
return ctr;
}

await act(() => {
render(<App />, container);
});
expect(container.innerHTML).toEqual('1');
});

// @gate __DEV__
it('warns if you do not await an act call', async () => {
spyOnDevAndProd(console, 'error');
Expand Down Expand Up @@ -461,7 +462,13 @@ function runActTests(label, render, unmount, rerender) {

await sleep(150);
if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error).toHaveBeenCalledTimes(2);
expect(console.error.calls.argsFor(0)[0]).toMatch(
'You seem to have overlapping act() calls',
);
expect(console.error.calls.argsFor(1)[0]).toMatch(
'You seem to have overlapping act() calls',
);
}
});

Expand Down

This file was deleted.

This file was deleted.

10 changes: 9 additions & 1 deletion packages/react-dom/src/test-utils/ReactTestUtilsInternalAct.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@ const IsThisRendererActing = SecretInternals.IsThisRendererActing;

const batchedUpdates = ReactDOM.unstable_batchedUpdates;

const {IsSomeRendererActing} = ReactSharedInternals;
const {IsSomeRendererActing, ReactCurrentActQueue} = ReactSharedInternals;

// This version of `act` is only used by our tests. Unlike the public version
// of `act`, it's designed to work identically in both production and
// development. It may have slightly different behavior from the public
// version, too, since our constraints in our test suite are not the same as
// those of developers using React — we're testing React itself, as opposed to
// building an app with React.
// TODO: Replace the internal "concurrent" implementations of `act` with a
// single shared module.

let actingUpdatesScopeDepth = 0;

Expand All @@ -50,8 +52,14 @@ export function unstable_concurrentAct(scope: () => Thenable<mixed> | void) {
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
actingUpdatesScopeDepth++;
if (__DEV__ && actingUpdatesScopeDepth === 1) {
ReactCurrentActQueue.disableActWarning = true;
}

const unwind = () => {
if (__DEV__ && actingUpdatesScopeDepth === 1) {
ReactCurrentActQueue.disableActWarning = false;
}
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
Expand Down
10 changes: 9 additions & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {

import ReactSharedInternals from 'shared/ReactSharedInternals';
import enqueueTask from 'shared/enqueueTask';
const {IsSomeRendererActing} = ReactSharedInternals;
const {IsSomeRendererActing, ReactCurrentActQueue} = ReactSharedInternals;

type Container = {
rootID: string,
Expand Down Expand Up @@ -1048,6 +1048,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
// version, too, since our constraints in our test suite are not the same as
// those of developers using React — we're testing React itself, as opposed to
// building an app with React.
// TODO: Replace the internal "concurrent" implementations of `act` with a
// single shared module.

const {batchedUpdates, IsThisRendererActing} = NoopRenderer;
let actingUpdatesScopeDepth = 0;
Expand All @@ -1071,8 +1073,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
actingUpdatesScopeDepth++;
if (__DEV__ && actingUpdatesScopeDepth === 1) {
ReactCurrentActQueue.disableActWarning = true;
}

const unwind = () => {
if (__DEV__ && actingUpdatesScopeDepth === 1) {
ReactCurrentActQueue.disableActWarning = false;
}
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
Expand Down
2 changes: 0 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ import {
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.new';
Expand Down Expand Up @@ -2011,7 +2010,6 @@ function dispatchAction<S, A>(
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfNotScopedWithMatchingAct(fiber);
warnIfNotCurrentlyActingUpdatesInDev(fiber);
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ import {
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.old';
Expand Down Expand Up @@ -2011,7 +2010,6 @@ function dispatchAction<S, A>(
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfNotScopedWithMatchingAct(fiber);
warnIfNotCurrentlyActingUpdatesInDev(fiber);
}
}
Expand Down
9 changes: 0 additions & 9 deletions packages/react-reconciler/src/ReactFiberReconciler.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ import {
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
warnIfUnmockedScheduler,
IsThisRendererActing,
act,
} from './ReactFiberWorkLoop.new';
Expand Down Expand Up @@ -272,13 +270,6 @@ export function updateContainer(
}
const current = container.current;
const eventTime = requestEventTime();
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfUnmockedScheduler(current);
warnIfNotScopedWithMatchingAct(current);
}
}
const lane = requestUpdateLane(current);

if (enableSchedulingProfiler) {
Expand Down
9 changes: 0 additions & 9 deletions packages/react-reconciler/src/ReactFiberReconciler.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ import {
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
warnIfUnmockedScheduler,
IsThisRendererActing,
act,
} from './ReactFiberWorkLoop.old';
Expand Down Expand Up @@ -272,13 +270,6 @@ export function updateContainer(
}
const current = container.current;
const eventTime = requestEventTime();
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfUnmockedScheduler(current);
warnIfNotScopedWithMatchingAct(current);
}
}
const lane = requestUpdateLane(current);

if (enableSchedulingProfiler) {
Expand Down
Loading

0 comments on commit 3400ee6

Please sign in to comment.