Skip to content

Commit

Permalink
allow nested act()s from different renderers (facebook#16039)
Browse files Browse the repository at this point in the history
* allow nested `act()`s from different renderers

There are usecases where multiple renderers need to oprate inside an act() scope
- ReactDOM.render being used inside another component tree. The parent component will be rendered using ReactTestRenderer.create for a snapshot test or something.
- a ReactDOM instance interacting with a ReactTestRenderer instance (like for the new devtools)

This PR changes the way the acting sigils operate to allow for this. It keeps 2 booleans, one attached to React, one attached to the renderer. act() changes these values, and the workloop reads them to decide what warning to trigger.

I also renamed shouldWarnUnactedUpdates to warnsIfNotActing

* s/ReactIsActing/IsSomeRendererActing and s/ReactRendererIsActing/IsThisRendererActing
  • Loading branch information
Sunil Pai authored and trueadm committed Jul 3, 2019
1 parent 068b86b commit 4dc279d
Show file tree
Hide file tree
Showing 18 changed files with 85 additions and 75 deletions.
18 changes: 10 additions & 8 deletions fixtures/dom/src/index.test.js
Expand Up @@ -70,10 +70,7 @@ it('warns when using the wrong act version - test + dom: updates', () => {
TestRenderer.act(() => {
setCtr(1);
});
}).toWarnDev([
'An update to Counter inside a test was not wrapped in act',
"It looks like you're using the wrong act()",
]);
}).toWarnDev(["It looks like you're using the wrong act()"]);
});

it('warns when using the wrong act version - dom + test: .create()', () => {
Expand Down Expand Up @@ -109,10 +106,7 @@ it('warns when using the wrong act version - dom + test: updates', () => {
TestUtils.act(() => {
setCtr(1);
});
}).toWarnDev([
'An update to Counter inside a test was not wrapped in act',
"It looks like you're using the wrong act()",
]);
}).toWarnDev(["It looks like you're using the wrong act()"]);
});

const {Surface, Group, Shape} = ReactART;
Expand Down Expand Up @@ -158,3 +152,11 @@ it('does not warn when nesting react-act inside react-test-renderer', () => {
TestRenderer.create(<ARTTest />);
});
});

it("doesn't warn if you use nested acts from different renderers", () => {
TestRenderer.act(() => {
TestUtils.act(() => {
TestRenderer.create(<App />);
});
});
});
2 changes: 1 addition & 1 deletion packages/react-art/src/ReactARTHostConfig.js
Expand Up @@ -347,7 +347,7 @@ export function shouldSetTextContent(type, props) {
export const isPrimaryRenderer = false;

// The ART renderer shouldn't trigger missing act() warnings
export const shouldWarnUnactedUpdates = false;
export const warnsIfNotActing = false;

export const supportsMutation = true;

Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/ReactDOM.js
Expand Up @@ -38,7 +38,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
} from 'react-reconciler/inline.dom';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
Expand Down Expand Up @@ -817,7 +817,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
],
},
};
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMHostConfig.js
Expand Up @@ -348,7 +348,7 @@ export function createTextInstance(
}

export const isPrimaryRenderer = true;
export const shouldWarnUnactedUpdates = true;
export const warnsIfNotActing = true;
// This initialization code may run even on server environments
// if a component just imports ReactDOM (e.g. for findDOMNode).
// Some environments might not have setTimeout or clearTimeout.
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/fire/ReactFire.js
Expand Up @@ -43,7 +43,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
} from 'react-reconciler/inline.fire';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
Expand Down Expand Up @@ -823,7 +823,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
],
},
};
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/test-utils/ReactTestUtils.js
Expand Up @@ -44,7 +44,7 @@ const [
runEventsInBatch,
/* eslint-disable no-unused-vars */
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
/* eslint-enable no-unused-vars */
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

Expand Down
16 changes: 10 additions & 6 deletions packages/react-dom/src/test-utils/ReactTestUtilsAct.js
Expand Up @@ -33,12 +33,12 @@ const [
runEventsInBatch,
/* eslint-enable no-unused-vars */
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

const batchedUpdates = ReactDOM.unstable_batchedUpdates;

const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
const {IsSomeRendererActing} = ReactSharedInternals;

// this implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
Expand Down Expand Up @@ -86,17 +86,21 @@ let actingUpdatesScopeDepth = 0;

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
let previousActingUpdatesSigil;
let previousIsSomeRendererActing;
let previousIsThisRendererActing;
actingUpdatesScopeDepth++;
if (__DEV__) {
previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
previousIsSomeRendererActing = IsSomeRendererActing.current;
previousIsThisRendererActing = IsSomeRendererActing.current;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
}

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
warningWithoutStack(
Expand Down
Expand Up @@ -352,7 +352,7 @@ export function shouldSetTextContent(type: string, props: Props): boolean {
export const isPrimaryRenderer = false;

// The Fabric renderer shouldn't trigger missing act() warnings
export const shouldWarnUnactedUpdates = false;
export const warnsIfNotActing = false;

export const scheduleTimeout = setTimeout;
export const cancelTimeout = clearTimeout;
Expand Down
Expand Up @@ -247,7 +247,7 @@ export function resetAfterCommit(containerInfo: Container): void {
}

export const isPrimaryRenderer = true;
export const shouldWarnUnactedUpdates = true;
export const warnsIfNotActing = true;

export const scheduleTimeout = setTimeout;
export const cancelTimeout = clearTimeout;
Expand Down
18 changes: 11 additions & 7 deletions packages/react-noop-renderer/src/createReactNoop.js
Expand Up @@ -65,7 +65,7 @@ type TextInstance = {|
|};
type HostContext = Object;

const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
const {IsSomeRendererActing} = ReactSharedInternals;

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
Expand Down Expand Up @@ -393,7 +393,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
now: Scheduler.unstable_now,

isPrimaryRenderer: true,
shouldWarnUnactedUpdates: true,
warnsIfNotActing: true,
supportsHydration: false,

mountEventComponent(): void {
Expand Down Expand Up @@ -566,7 +566,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const {
flushPassiveEffects,
batchedUpdates,
ReactActingRendererSigil,
IsThisRendererActing,
} = NoopRenderer;

// this act() implementation should be exactly the same in
Expand Down Expand Up @@ -615,17 +615,21 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
let previousActingUpdatesSigil;
let previousIsSomeRendererActing;
let previousIsThisRendererActing;
actingUpdatesScopeDepth++;
if (__DEV__) {
previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
previousIsSomeRendererActing = IsSomeRendererActing.current;
previousIsThisRendererActing = IsSomeRendererActing.current;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
}

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
warningWithoutStack(
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Expand Up @@ -57,7 +57,7 @@ import {
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
ReactActingRendererSigil,
IsThisRendererActing,
} from './ReactFiberWorkLoop';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -345,7 +345,7 @@ export {
flushControlled,
flushSync,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
};

export function getPublicRootInstance(
Expand Down
26 changes: 12 additions & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Expand Up @@ -55,7 +55,7 @@ import {
scheduleTimeout,
cancelTimeout,
noTimeout,
shouldWarnUnactedUpdates,
warnsIfNotActing,
} from './ReactFiberHostConfig';

import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber';
Expand Down Expand Up @@ -174,7 +174,7 @@ const ceil = Math.ceil;
const {
ReactCurrentDispatcher,
ReactCurrentOwner,
ReactCurrentActingRendererSigil,
IsSomeRendererActing,
} = ReactSharedInternals;

type ExecutionContext = number;
Expand Down Expand Up @@ -2420,18 +2420,14 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
}
}

// We export a simple object here to be used by a renderer/test-utils
// as the value of ReactCurrentActingRendererSigil.current
// This identity lets us identify (ha!) when the wrong renderer's act()
// wraps anothers' updates/effects
export const ReactActingRendererSigil = {};
export const IsThisRendererActing = {current: (false: boolean)};

export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
if (__DEV__) {
if (
shouldWarnUnactedUpdates === true &&
ReactCurrentActingRendererSigil.current !== null &&
ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil
warnsIfNotActing === true &&
IsSomeRendererActing.current === true &&
IsThisRendererActing.current !== true
) {
warningWithoutStack(
false,
Expand All @@ -2456,8 +2452,9 @@ export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
if (__DEV__) {
if (
shouldWarnUnactedUpdates === true &&
ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil
warnsIfNotActing === true &&
IsSomeRendererActing.current === false &&
IsThisRendererActing.current === false
) {
warningWithoutStack(
false,
Expand All @@ -2482,9 +2479,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
if (__DEV__) {
if (
shouldWarnUnactedUpdates === true &&
warnsIfNotActing === true &&
executionContext === NoContext &&
ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil
IsSomeRendererActing.current === false &&
IsThisRendererActing.current === false
) {
warningWithoutStack(
false,
Expand Down
Expand Up @@ -59,7 +59,7 @@ export const cancelTimeout = $$$hostConfig.clearTimeout;
export const noTimeout = $$$hostConfig.noTimeout;
export const now = $$$hostConfig.now;
export const isPrimaryRenderer = $$$hostConfig.isPrimaryRenderer;
export const shouldWarnUnactedUpdates = $$$hostConfig.shouldWarnUnactedUpdates;
export const warnsIfNotActing = $$$hostConfig.warnsIfNotActing;
export const supportsMutation = $$$hostConfig.supportsMutation;
export const supportsPersistence = $$$hostConfig.supportsPersistence;
export const supportsHydration = $$$hostConfig.supportsHydration;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-test-renderer/src/ReactTestHostConfig.js
Expand Up @@ -217,7 +217,7 @@ export function createTextInstance(
}

export const isPrimaryRenderer = false;
export const shouldWarnUnactedUpdates = true;
export const warnsIfNotActing = true;

export const scheduleTimeout = setTimeout;
export const cancelTimeout = clearTimeout;
Expand Down
16 changes: 10 additions & 6 deletions packages/react-test-renderer/src/ReactTestRendererAct.js
Expand Up @@ -11,15 +11,15 @@ import type {Thenable} from 'react-reconciler/src/ReactFiberWorkLoop';
import {
batchedUpdates,
flushPassiveEffects,
ReactActingRendererSigil,
IsThisRendererActing,
} from 'react-reconciler/inline.test';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import warningWithoutStack from 'shared/warningWithoutStack';
import {warnAboutMissingMockScheduler} from 'shared/ReactFeatureFlags';
import enqueueTask from 'shared/enqueueTask';
import * as Scheduler from 'scheduler';

const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
const {IsSomeRendererActing} = ReactSharedInternals;

// this implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
Expand Down Expand Up @@ -67,17 +67,21 @@ let actingUpdatesScopeDepth = 0;

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
let previousActingUpdatesSigil;
let previousIsSomeRendererActing;
let previousIsThisRendererActing;
actingUpdatesScopeDepth++;
if (__DEV__) {
previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
previousIsSomeRendererActing = IsSomeRendererActing.current;
previousIsThisRendererActing = IsSomeRendererActing.current;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
}

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
warningWithoutStack(
Expand Down
17 changes: 17 additions & 0 deletions packages/react/src/IsSomeRendererActing.js
@@ -0,0 +1,17 @@
/**
* 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
*/

/**
* Used by act() to track whether you're inside an act() scope.
*/

const IsSomeRendererActing = {
current: (false: boolean),
};
export default IsSomeRendererActing;
19 changes: 0 additions & 19 deletions packages/react/src/ReactCurrentActingRendererSigil.js

This file was deleted.

0 comments on commit 4dc279d

Please sign in to comment.