From 62105dede34ae79d5aad30a5d9d66e3ace28a5c4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 14 Sep 2020 11:46:28 -0500 Subject: [PATCH] Fix `act` bundle size regression 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. --- .../src/__tests__/ReactTestUtilsAct-test.js | 13 ++ packages/react-dom/src/client/ReactDOM.js | 4 +- .../src/test-utils/ReactTestUtils.js | 6 +- ...ilsAct.js => ReactTestUtilsInternalAct.js} | 0 .../src/test-utils/ReactTestUtilsPublicAct.js | 217 ++++++++++++++++++ 5 files changed, 234 insertions(+), 6 deletions(-) rename packages/react-dom/src/test-utils/{ReactTestUtilsAct.js => ReactTestUtilsInternalAct.js} (100%) create mode 100644 packages/react-dom/src/test-utils/ReactTestUtilsPublicAct.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 7b48d0118bebd..4067df79f372a 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -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 => { diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 78c920ec44afb..fcd54a58ede5d 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -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'; @@ -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, ], }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 4a6feaaf5cdf8..dc59d2a64420f 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -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, @@ -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) {} diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsInternalAct.js similarity index 100% rename from packages/react-dom/src/test-utils/ReactTestUtilsAct.js rename to packages/react-dom/src/test-utils/ReactTestUtilsInternalAct.js diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsPublicAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsPublicAct.js new file mode 100644 index 0000000000000..91ce4c9c5f68f --- /dev/null +++ b/packages/react-dom/src/test-utils/ReactTestUtilsPublicAct.js @@ -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): Thenable { + 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(); + }, + }; + } +}