allow nested act()s from different renderers (facebook#16039)

Sunil Pai · web-flow · commit a457e02ae3a2 · 2019-07-02T22:20:17.000+01:00
* 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
diff --git a/fixtures/dom/src/index.test.js b/fixtures/dom/src/index.test.js
@@ -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()', () => {
@@ -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;
@@ -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 />);
+    });
+  });
+});
diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js
@@ -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;
 
diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js
@@ -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';
@@ -817,7 +817,7 @@ const ReactDOM: Object = {
       dispatchEvent,
       runEventsInBatch,
       flushPassiveEffects,
-      ReactActingRendererSigil,
+      IsThisRendererActing,
     ],
   },
 };
diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -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.
diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js
@@ -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';
@@ -823,7 +823,7 @@ const ReactDOM: Object = {
       dispatchEvent,
       runEventsInBatch,
       flushPassiveEffects,
-      ReactActingRendererSigil,
+      IsThisRendererActing,
     ],
   },
 };
diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js
@@ -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;
 
diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js
@@ -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
@@ -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(
diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js
@@ -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;
diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js
@@ -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;
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -65,7 +65,7 @@ type TextInstance = {|
 |};
 type HostContext = Object;
 
-const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
+const {IsSomeRendererActing} = ReactSharedInternals;
 
 const NO_CONTEXT = {};
 const UPPERCASE_CONTEXT = {};
@@ -393,7 +393,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
     now: Scheduler.unstable_now,
 
     isPrimaryRenderer: true,
-    shouldWarnUnactedUpdates: true,
+    warnsIfNotActing: true,
     supportsHydration: false,
 
     mountEventComponent(): void {
@@ -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
@@ -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(
diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js
@@ -57,7 +57,7 @@ import {
   flushDiscreteUpdates,
   flushPassiveEffects,
   warnIfNotScopedWithMatchingAct,
-  ReactActingRendererSigil,
+  IsThisRendererActing,
 } from './ReactFiberWorkLoop';
 import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
 import ReactFiberInstrumentation from './ReactFiberInstrumentation';
@@ -345,7 +345,7 @@ export {
   flushControlled,
   flushSync,
   flushPassiveEffects,
-  ReactActingRendererSigil,
+  IsThisRendererActing,
 };
 
 export function getPublicRootInstance(
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -55,7 +55,7 @@ import {
   scheduleTimeout,
   cancelTimeout,
   noTimeout,
-  shouldWarnUnactedUpdates,
+  warnsIfNotActing,
 } from './ReactFiberHostConfig';
 
 import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber';
@@ -174,7 +174,7 @@ const ceil = Math.ceil;
 const {
   ReactCurrentDispatcher,
   ReactCurrentOwner,
-  ReactCurrentActingRendererSigil,
+  IsSomeRendererActing,
 } = ReactSharedInternals;
 
 type ExecutionContext = number;
@@ -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,
@@ -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,
@@ -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,
diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
@@ -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;
diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js
@@ -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;
diff --git a/packages/react-test-renderer/src/ReactTestRendererAct.js b/packages/react-test-renderer/src/ReactTestRendererAct.js
@@ -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
@@ -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(
diff --git a/packages/react/src/IsSomeRendererActing.js b/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;
diff --git a/packages/react/src/ReactCurrentActingRendererSigil.js b/packages/react/src/ReactCurrentActingRendererSigil.js
diff --git a/packages/react/src/ReactSharedInternals.js b/packages/react/src/ReactSharedInternals.js

Original file line number	Diff line number	Diff line change
`@@ -348,7 +348,7 @@ export function createTextInstance(`
`348`	`348`	`}`
`349`	`349`
`350`	`350`	`export const isPrimaryRenderer = true;`
`351`		`-export const shouldWarnUnactedUpdates = true;`
	`351`	`+export const warnsIfNotActing = true;`
`352`	`352`	`// This initialization code may run even on server environments`
`353`	`353`	`// if a component just imports ReactDOM (e.g. for findDOMNode).`
`354`	`354`	`// Some environments might not have setTimeout or clearTimeout.`
Original file line number	Diff line number	Diff line change
`@@ -247,7 +247,7 @@ export function resetAfterCommit(containerInfo: Container): void {`
`247`	`247`	`}`
`248`	`248`
`249`	`249`	`export const isPrimaryRenderer = true;`
`250`		`-export const shouldWarnUnactedUpdates = true;`
	`250`	`+export const warnsIfNotActing = true;`
`251`	`251`
`252`	`252`	`export const scheduleTimeout = setTimeout;`
`253`	`253`	`export const cancelTimeout = clearTimeout;`