flush work on exiting outermost act(), with nested act()s from different renderers (#16181)

Given this snippet: ```jsx TestRenderer.act(() => { TestUtils.act(() => { TestRenderer.create(<Effecty />); }); }); ``` We want to make sure that all work is only flushed on exiting the outermost act(). Now, naively doing this based on actingScopeDepth would work with a mocked scheduler, where flushAll() would flush all work across renderers. This doesn't work without mocking the scheduler though; and where flushing work only works per renderer. So we disable this behaviour for a non-mocked scenario. This seems like an ok tradeoff.
2025-11-01 09:12:30 +00:00 · 2019-07-24 00:20:28 +01:00
parent 5098891193
commit c73e1f236f
4 changed files with 242 additions and 161 deletions
@@ -7,15 +7,14 @@
 * @emails react-core
 */

-import React from 'react';
-import ReactDOM from 'react-dom';
-import ReactART from 'react-art';
-import ARTSVGMode from 'art/modes/svg';
-import ARTCurrentMode from 'art/modes/current';
-import TestUtils from 'react-dom/test-utils';
-import TestRenderer from 'react-test-renderer';
-
-ARTCurrentMode.setCurrent(ARTSVGMode);
+let React;
+let ReactDOM;
+let ReactART;
+let ARTSVGMode;
+let ARTCurrentMode;
+let TestUtils;
+let TestRenderer;
+let ARTTest;

 global.__DEV__ = process.env.NODE_ENV !== 'production';

@@ -25,153 +24,210 @@ function App(props) {
  return 'hello world';
 }

-it("doesn't warn when you use the right act + renderer: dom", () => {
-  TestUtils.act(() => {
-    TestUtils.renderIntoDocument(<App />);
-  });
+describe('legacy mode', () => {
+  runTests();
 });

-it("doesn't warn when you use the right act + renderer: test", () => {
-  TestRenderer.act(() => {
-    TestRenderer.create(<App />);
+describe('mocked scheduler', () => {
+  beforeEach(() => {
+    jest.mock('scheduler', () =>
+      require.requireActual('scheduler/unstable_mock')
+    );
  });
+  afterEach(() => {
+    jest.unmock('scheduler');
+  });
+  runTests();
 });

-it('resets correctly across renderers', () => {
-  function Effecty() {
-    React.useEffect(() => {}, []);
-    return null;
-  }
-  TestUtils.act(() => {
-    TestRenderer.act(() => {});
+function runTests() {
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    ReactDOM = require('react-dom');
+    ReactART = require('react-art');
+    ARTSVGMode = require('art/modes/svg');
+    ARTCurrentMode = require('art/modes/current');
+    TestUtils = require('react-dom/test-utils');
+    TestRenderer = require('react-test-renderer');
+
+    ARTCurrentMode.setCurrent(ARTSVGMode);
+
+    ARTTest = function ARTTest(props) {
+      return (
+        <ReactART.Surface width={150} height={200}>
+          <ReactART.Group>
+            <ReactART.Shape
+              d="M0,0l50,0l0,50l-50,0z"
+              fill={new ReactART.LinearGradient(['black', 'white'])}
+              key="a"
+              width={50}
+              height={50}
+              x={50}
+              y={50}
+              opacity={0.1}
+            />
+            <ReactART.Shape
+              fill="#3C5A99"
+              key="b"
+              scale={0.5}
+              x={50}
+              y={50}
+              title="This is an F"
+              cursor="pointer">
+              M64.564,38.583H54l0.008-5.834c0-3.035,0.293-4.666,4.657-4.666
+              h5.833V16.429h-9.33c-11.213,0-15.159,5.654-15.159,15.16v6.994
+              h-6.99v11.652h6.99v33.815H54V50.235h9.331L64.564,38.583z
+            </ReactART.Shape>
+          </ReactART.Group>
+        </ReactART.Surface>
+      );
+    };
+  });
+  it("doesn't warn when you use the right act + renderer: dom", () => {
+    TestUtils.act(() => {
+      TestUtils.renderIntoDocument(<App />);
+    });
+  });
+
+  it("doesn't warn when you use the right act + renderer: test", () => {
+    TestRenderer.act(() => {
+      TestRenderer.create(<App />);
+    });
+  });
+
+  it('resets correctly across renderers', () => {
+    function Effecty() {
+      React.useEffect(() => {}, []);
+      return null;
+    }
+    TestUtils.act(() => {
+      TestRenderer.act(() => {});
+      expect(() => {
+        TestRenderer.create(<Effecty />);
+      }).toWarnDev(["It looks like you're using the wrong act()"], {
+        withoutStack: true,
+      });
+    });
+  });
+
+  it('warns when using createRoot() + .render', () => {
+    const root = ReactDOM.unstable_createRoot(document.createElement('div'));
    expect(() => {
-      TestRenderer.create(<Effecty />);
+      TestRenderer.act(() => {
+        root.render(<App />);
+      });
    }).toWarnDev(["It looks like you're using the wrong act()"], {
      withoutStack: true,
    });
  });
-});

-it('warns when using createRoot() + .render', () => {
-  const root = ReactDOM.unstable_createRoot(document.createElement('div'));
-  expect(() => {
-    TestRenderer.act(() => {
-      root.render(<App />);
+  it('warns when using the wrong act version - test + dom: render', () => {
+    expect(() => {
+      TestRenderer.act(() => {
+        TestUtils.renderIntoDocument(<App />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
    });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
  });
-});

-it('warns when using the wrong act version - test + dom: render', () => {
-  expect(() => {
-    TestRenderer.act(() => {
-      TestUtils.renderIntoDocument(<App />);
-    });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
+  it('warns when using the wrong act version - test + dom: updates', () => {
+    let setCtr;
+    function Counter(props) {
+      const [ctr, _setCtr] = React.useState(0);
+      setCtr = _setCtr;
+      return ctr;
+    }
+    TestUtils.renderIntoDocument(<Counter />);
+    expect(() => {
+      TestRenderer.act(() => {
+        setCtr(1);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"]);
  });
-});

-it('warns when using the wrong act version - test + dom: updates', () => {
-  let setCtr;
-  function Counter(props) {
-    const [ctr, _setCtr] = React.useState(0);
-    setCtr = _setCtr;
-    return ctr;
-  }
-  TestUtils.renderIntoDocument(<Counter />);
-  expect(() => {
-    TestRenderer.act(() => {
-      setCtr(1);
+  it('warns when using the wrong act version - dom + test: .create()', () => {
+    expect(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<App />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
    });
-  }).toWarnDev(["It looks like you're using the wrong act()"]);
-});
+  });

-it('warns when using the wrong act version - dom + test: .create()', () => {
-  expect(() => {
+  it('warns when using the wrong act version - dom + test: .update()', () => {
+    const root = TestRenderer.create(<App key="one" />);
+    expect(() => {
+      TestUtils.act(() => {
+        root.update(<App key="two" />);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"], {
+      withoutStack: true,
+    });
+  });
+
+  it('warns when using the wrong act version - dom + test: updates', () => {
+    let setCtr;
+    function Counter(props) {
+      const [ctr, _setCtr] = React.useState(0);
+      setCtr = _setCtr;
+      return ctr;
+    }
+    const root = TestRenderer.create(<Counter />);
+    expect(() => {
+      TestUtils.act(() => {
+        setCtr(1);
+      });
+    }).toWarnDev(["It looks like you're using the wrong act()"]);
+  });
+
+  it('does not warn when nesting react-act inside react-dom', () => {
    TestUtils.act(() => {
-      TestRenderer.create(<App />);
+      TestUtils.renderIntoDocument(<ARTTest />);
    });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
  });
-});

-it('warns when using the wrong act version - dom + test: .update()', () => {
-  const root = TestRenderer.create(<App key="one" />);
-  expect(() => {
-    TestUtils.act(() => {
-      root.update(<App key="two" />);
+  it('does not warn when nesting react-act inside react-test-renderer', () => {
+    TestRenderer.act(() => {
+      TestRenderer.create(<ARTTest />);
    });
-  }).toWarnDev(["It looks like you're using the wrong act()"], {
-    withoutStack: true,
  });
-});

-it('warns when using the wrong act version - dom + test: updates', () => {
-  let setCtr;
-  function Counter(props) {
-    const [ctr, _setCtr] = React.useState(0);
-    setCtr = _setCtr;
-    return ctr;
-  }
-  const root = TestRenderer.create(<Counter />);
-  expect(() => {
-    TestUtils.act(() => {
-      setCtr(1);
+  it("doesn't warn if you use nested acts from different renderers", () => {
+    TestRenderer.act(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<App />);
+      });
    });
-  }).toWarnDev(["It looks like you're using the wrong act()"]);
-});
+  });

-const {Surface, Group, Shape} = ReactART;
-function ARTTest(props) {
-  return (
-    <Surface width={150} height={200}>
-      <Group>
-        <Shape
-          d="M0,0l50,0l0,50l-50,0z"
-          fill={new ReactART.LinearGradient(['black', 'white'])}
-          key="a"
-          width={50}
-          height={50}
-          x={50}
-          y={50}
-          opacity={0.1}
-        />
-        <Shape
-          fill="#3C5A99"
-          key="b"
-          scale={0.5}
-          x={50}
-          y={50}
-          title="This is an F"
-          cursor="pointer">
-          M64.564,38.583H54l0.008-5.834c0-3.035,0.293-4.666,4.657-4.666
-          h5.833V16.429h-9.33c-11.213,0-15.159,5.654-15.159,15.16v6.994
-          h-6.99v11.652h6.99v33.815H54V50.235h9.331L64.564,38.583z
-        </Shape>
-      </Group>
-    </Surface>
-  );
+  it('flushes work only outside the outermost act(), even when nested from different renderers', () => {
+    const log = [];
+    function Effecty() {
+      React.useEffect(() => {
+        log.push('called');
+      }, []);
+      return null;
+    }
+    // in legacy mode, this tests whether an act only flushes its own effects
+    // with a mocked scheduler, this tests whether it flushes all work only on the outermost act
+    TestRenderer.act(() => {
+      TestUtils.act(() => {
+        TestRenderer.create(<Effecty />);
+      });
+      expect(log).toEqual([]);
+    });
+    expect(log).toEqual(['called']);
+
+    log.splice(0);
+    // for doublechecking, we flip it inside out, and assert on the outermost
+    TestUtils.act(() => {
+      TestRenderer.act(() => {
+        TestRenderer.create(<Effecty />);
+      });
+    });
+    expect(log).toEqual(['called']);
+  });
 }
-
-it('does not warn when nesting react-act inside react-dom', () => {
-  TestUtils.act(() => {
-    TestUtils.renderIntoDocument(<ARTTest />);
-  });
-});
-
-it('does not warn when nesting react-act inside react-test-renderer', () => {
-  TestRenderer.act(() => {
-    TestRenderer.create(<ARTTest />);
-  });
-});
-
-it("doesn't warn if you use nested acts from different renderers", () => {
-  TestRenderer.act(() => {
-    TestUtils.act(() => {
-      TestRenderer.create(<App />);
-    });
-  });
-});
@@ -44,6 +44,8 @@ const {IsSomeRendererActing} = ReactSharedInternals;
 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js

 let hasWarnedAboutMissingMockScheduler = false;
+const isSchedulerMocked =
+  typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
 const flushWork =
  Scheduler.unstable_flushAllWithoutAsserting ||
  function() {
@@ -89,18 +91,17 @@ function act(callback: () => Thenable) {
  let previousIsSomeRendererActing;
  let previousIsThisRendererActing;
  actingUpdatesScopeDepth++;
-  if (__DEV__) {
-    previousIsSomeRendererActing = IsSomeRendererActing.current;
-    previousIsThisRendererActing = IsThisRendererActing.current;
-    IsSomeRendererActing.current = true;
-    IsThisRendererActing.current = true;
-  }
+
+  previousIsSomeRendererActing = IsSomeRendererActing.current;
+  previousIsThisRendererActing = IsThisRendererActing.current;
+  IsSomeRendererActing.current = true;
+  IsThisRendererActing.current = true;

  function onDone() {
    actingUpdatesScopeDepth--;
+    IsSomeRendererActing.current = previousIsSomeRendererActing;
+    IsThisRendererActing.current = previousIsThisRendererActing;
    if (__DEV__) {
-      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(
@@ -155,7 +156,11 @@ function act(callback: () => Thenable) {
        called = true;
        result.then(
          () => {
-            if (actingUpdatesScopeDepth > 1) {
+            if (
+              actingUpdatesScopeDepth > 1 ||
+              (isSchedulerMocked === true &&
+                previousIsSomeRendererActing === true)
+            ) {
              onDone();
              resolve();
              return;
@@ -190,7 +195,10 @@ function act(callback: () => Thenable) {

    // flush effects until none remain, and cleanup
    try {
-      if (actingUpdatesScopeDepth === 1) {
+      if (
+        actingUpdatesScopeDepth === 1 &&
+        (isSchedulerMocked === false || previousIsSomeRendererActing === false)
+      ) {
        // we're about to exit the act() scope,
        // now's the time to flush effects
        flushWork();
@@ -600,6 +600,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
  // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js

  let hasWarnedAboutMissingMockScheduler = false;
+  const isSchedulerMocked =
+    typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
  const flushWork =
    Scheduler.unstable_flushAllWithoutAsserting ||
    function() {
@@ -645,18 +647,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
    let previousIsSomeRendererActing;
    let previousIsThisRendererActing;
    actingUpdatesScopeDepth++;
-    if (__DEV__) {
-      previousIsSomeRendererActing = IsSomeRendererActing.current;
-      previousIsThisRendererActing = IsThisRendererActing.current;
-      IsSomeRendererActing.current = true;
-      IsThisRendererActing.current = true;
-    }
+
+    previousIsSomeRendererActing = IsSomeRendererActing.current;
+    previousIsThisRendererActing = IsThisRendererActing.current;
+    IsSomeRendererActing.current = true;
+    IsThisRendererActing.current = true;

    function onDone() {
      actingUpdatesScopeDepth--;
+      IsSomeRendererActing.current = previousIsSomeRendererActing;
+      IsThisRendererActing.current = previousIsThisRendererActing;
      if (__DEV__) {
-        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(
@@ -711,7 +712,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
          called = true;
          result.then(
            () => {
-              if (actingUpdatesScopeDepth > 1) {
+              if (
+                actingUpdatesScopeDepth > 1 ||
+                (isSchedulerMocked === true &&
+                  previousIsSomeRendererActing === true)
+              ) {
                onDone();
                resolve();
                return;
@@ -746,7 +751,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

      // flush effects until none remain, and cleanup
      try {
-        if (actingUpdatesScopeDepth === 1) {
+        if (
+          actingUpdatesScopeDepth === 1 &&
+          (isSchedulerMocked === false ||
+            previousIsSomeRendererActing === false)
+        ) {
          // we're about to exit the act() scope,
          // now's the time to flush effects
          flushWork();
@@ -25,6 +25,8 @@ const {IsSomeRendererActing} = ReactSharedInternals;
 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js

 let hasWarnedAboutMissingMockScheduler = false;
+const isSchedulerMocked =
+  typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
 const flushWork =
  Scheduler.unstable_flushAllWithoutAsserting ||
  function() {
@@ -70,18 +72,17 @@ function act(callback: () => Thenable) {
  let previousIsSomeRendererActing;
  let previousIsThisRendererActing;
  actingUpdatesScopeDepth++;
-  if (__DEV__) {
-    previousIsSomeRendererActing = IsSomeRendererActing.current;
-    previousIsThisRendererActing = IsThisRendererActing.current;
-    IsSomeRendererActing.current = true;
-    IsThisRendererActing.current = true;
-  }
+
+  previousIsSomeRendererActing = IsSomeRendererActing.current;
+  previousIsThisRendererActing = IsThisRendererActing.current;
+  IsSomeRendererActing.current = true;
+  IsThisRendererActing.current = true;

  function onDone() {
    actingUpdatesScopeDepth--;
+    IsSomeRendererActing.current = previousIsSomeRendererActing;
+    IsThisRendererActing.current = previousIsThisRendererActing;
    if (__DEV__) {
-      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(
@@ -136,7 +137,11 @@ function act(callback: () => Thenable) {
        called = true;
        result.then(
          () => {
-            if (actingUpdatesScopeDepth > 1) {
+            if (
+              actingUpdatesScopeDepth > 1 ||
+              (isSchedulerMocked === true &&
+                previousIsSomeRendererActing === true)
+            ) {
              onDone();
              resolve();
              return;
@@ -171,7 +176,10 @@ function act(callback: () => Thenable) {

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