Skip to content

Commit

Permalink
Fix act bundle size regression (#19832)
Browse files Browse the repository at this point in the history
Adds back the `TestUtils.act` implementation that I had removed
in #19745. This version of `act` is implemented in "userspace" (i.e. not
the reconciler), so it doesn't add to the production bundle size.

I had removed this in #19745 in favor of the `act` exported by the
reconciler because I thought we would remove support for `act` in
production in the impending major release. (It currently warns.)

However, we've since decided to continue supporting `act` in prod for
now, so that it doesn't block people from upgrading to v17. We'll drop
support in a future major release.

So, to avoid bloating the production bundle size, we need to move the
public version of `act` back to "userspace", like it was before.

This doesn't negate the main goal of #19745, though, which was to
decouple the public version(s) of `act` from the internal one that we
use to test React itself.
  • Loading branch information
acdlite authored Sep 14, 2020
1 parent ec39a5e commit b93f3e7
Show file tree
Hide file tree
Showing 5 changed files with 234 additions and 6 deletions.
13 changes: 13 additions & 0 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,20 @@ function runActTests(label, render, unmount, rerender) {
describe('suspense', () => {
if (__DEV__ && __EXPERIMENTAL__) {
// todo - remove __DEV__ check once we start using testing builds

it('triggers fallbacks if available', async () => {
if (label !== 'legacy mode') {
// FIXME: Support for Blocking* and Concurrent Mode were
// intentionally removed from the public version of `act`. It will
// be added back in a future major version, before Blocking and and
// Concurrent Mode are officially released. Consider disabling all
// non-Legacy tests in this suite until then.
//
// *Blocking Mode actually does happen to work, though
// not "officially" since it's an unreleased feature.
return;
}

let resolved = false;
let resolve;
const promise = new Promise(_resolve => {
Expand Down
4 changes: 1 addition & 3 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import {
attemptHydrationAtCurrentPriority,
runWithPriority,
getCurrentUpdateLanePriority,
act,
} from 'react-reconciler/src/ReactFiberReconciler';
import {createPortal as createPortalImpl} from 'react-reconciler/src/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
Expand Down Expand Up @@ -184,9 +183,8 @@ const Internals = {
enqueueStateRestore,
restoreStateIfNeeded,
flushPassiveEffects,
// TODO: These are related to `act`, not events. Move to separate key?
// TODO: This is related to `act`, not events. Move to separate key?
IsThisRendererActing,
act,
],
};

Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/test-utils/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import {
import {SyntheticEvent} from '../events/SyntheticEvent';
import invariant from 'shared/invariant';
import {ELEMENT_NODE} from '../shared/HTMLNodeType';
import {unstable_concurrentAct} from './ReactTestUtilsAct';
import {act} from './ReactTestUtilsPublicAct';
import {unstable_concurrentAct} from './ReactTestUtilsInternalAct';
import {
rethrowCaughtError,
invokeGuardedCallbackAndCatchFirstError,
Expand All @@ -33,9 +34,8 @@ const getFiberCurrentPropsFromNode = EventInternals[2];
const enqueueStateRestore = EventInternals[3];
const restoreStateIfNeeded = EventInternals[4];
// const flushPassiveEffects = EventInternals[5];
// TODO: These are related to `act`, not events. Move to separate key?
// TODO: This is related to `act`, not events. Move to separate key?
// const IsThisRendererActing = EventInternals[6];
const act = EventInternals[7];

function Event(suffix) {}

Expand Down
217 changes: 217 additions & 0 deletions packages/react-dom/src/test-utils/ReactTestUtilsPublicAct.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import type {Thenable} from 'shared/ReactTypes';

import * as ReactDOM from 'react-dom';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import enqueueTask from 'shared/enqueueTask';
import * as Scheduler from 'scheduler';

// Keep in sync with ReactDOM.js, and ReactTestUtils.js:
const EventInternals =
ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;
// const getInstanceFromNode = EventInternals[0];
// const getNodeFromInstance = EventInternals[1];
// const getFiberCurrentPropsFromNode = EventInternals[2];
// const enqueueStateRestore = EventInternals[3];
// const restoreStateIfNeeded = EventInternals[4];
const flushPassiveEffects = EventInternals[5];
const IsThisRendererActing = EventInternals[6];

const batchedUpdates = ReactDOM.unstable_batchedUpdates;

const {IsSomeRendererActing} = ReactSharedInternals;

// This is the public version of `ReactTestUtils.act`. It is implemented in
// "userspace" (i.e. not the reconciler), so that it doesn't add to the
// production bundle size.
// TODO: Remove this implementation of `act` in favor of the one exported by
// the reconciler. To do this, we must first drop support for `act` in
// production mode.

// TODO: Remove support for the mock scheduler build, which was only added for
// the purposes of internal testing. Internal tests should use
// `unstable_concurrentAct` instead.
const isSchedulerMocked =
typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
const flushWork =
Scheduler.unstable_flushAllWithoutAsserting ||
function() {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}

return didFlushWork;
};

function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {
try {
flushWork();
enqueueTask(() => {
if (flushWork()) {
flushWorkAndMicroTasks(onDone);
} else {
onDone();
}
});
} catch (err) {
onDone(err);
}
}

// we track the 'depth' of the act() calls with this counter,
// so we can tell if any async act() calls try to run in parallel.

let actingUpdatesScopeDepth = 0;
let didWarnAboutUsingActInProd = false;

export function act(callback: () => Thenable<mixed>): Thenable<void> {
if (!__DEV__) {
if (didWarnAboutUsingActInProd === false) {
didWarnAboutUsingActInProd = true;
// eslint-disable-next-line react-internal/no-production-logging
console.error(
'act(...) is not supported in production builds of React, and might not behave as expected.',
);
}
}
const previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
actingUpdatesScopeDepth++;

const previousIsSomeRendererActing = IsSomeRendererActing.current;
const previousIsThisRendererActing = IsThisRendererActing.current;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;

function onDone() {
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
if (__DEV__) {
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
console.error(
'You seem to have overlapping act() calls, this is not supported. ' +
'Be sure to await previous act() calls before making a new one. ',
);
}
}
}

let result;
try {
result = batchedUpdates(callback);
} catch (error) {
// on sync errors, we still want to 'cleanup' and decrement actingUpdatesScopeDepth
onDone();
throw error;
}

if (
result !== null &&
typeof result === 'object' &&
typeof result.then === 'function'
) {
// setup a boolean that gets set to true only
// once this act() call is await-ed
let called = false;
if (__DEV__) {
if (typeof Promise !== 'undefined') {
//eslint-disable-next-line no-undef
Promise.resolve()
.then(() => {})
.then(() => {
if (called === false) {
console.error(
'You called act(async () => ...) without await. ' +
'This could lead to unexpected testing behaviour, interleaving multiple act ' +
'calls and mixing their scopes. You should - await act(async () => ...);',
);
}
});
}
}

// in the async case, the returned thenable runs the callback, flushes
// effects and microtasks in a loop until flushPassiveEffects() === false,
// and cleans up
return {
then(resolve, reject) {
called = true;
result.then(
() => {
if (
actingUpdatesScopeDepth > 1 ||
(isSchedulerMocked === true &&
previousIsSomeRendererActing === true)
) {
onDone();
resolve();
return;
}
// we're about to exit the act() scope,
// now's the time to flush tasks/effects
flushWorkAndMicroTasks((err: ?Error) => {
onDone();
if (err) {
reject(err);
} else {
resolve();
}
});
},
err => {
onDone();
reject(err);
},
);
},
};
} else {
if (__DEV__) {
if (result !== undefined) {
console.error(
'The callback passed to act(...) function ' +
'must return undefined, or a Promise. You returned %s',
result,
);
}
}

// flush effects until none remain, and cleanup
try {
if (
actingUpdatesScopeDepth === 1 &&
(isSchedulerMocked === false || previousIsSomeRendererActing === false)
) {
// we're about to exit the act() scope,
// now's the time to flush effects
flushWork();
}
onDone();
} catch (err) {
onDone();
throw err;
}

// in the sync case, the returned thenable only warns *if* await-ed
return {
then(resolve) {
if (__DEV__) {
console.error(
'Do not await the result of calling act(...) with sync logic, it is not a Promise.',
);
}
resolve();
},
};
}
}

0 comments on commit b93f3e7

Please sign in to comment.