Fix ignored sync work in passive effects (facebook#14799)

gaearon · web-flow · commit 3ae94e1885b6 · 2019-02-12T20:18:35.000Z
* Fix ignored sync work in passive effects

* Fix batching
diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js
@@ -12,7 +12,7 @@
 let React;
 let ReactDOM;
 
-describe('ReactDOMSuspensePlaceholder', () => {
+describe('ReactDOMHooks', () => {
   let container;
 
   beforeEach(() => {
@@ -29,6 +29,83 @@ describe('ReactDOMSuspensePlaceholder', () => {
     document.body.removeChild(container);
   });
 
+  it('can ReactDOM.render() from useEffect', () => {
+    let container2 = document.createElement('div');
+    let container3 = document.createElement('div');
+
+    function Example1({n}) {
+      React.useEffect(() => {
+        ReactDOM.render(<Example2 n={n} />, container2);
+      });
+      return 1 * n;
+    }
+
+    function Example2({n}) {
+      React.useEffect(() => {
+        ReactDOM.render(<Example3 n={n} />, container3);
+      });
+      return 2 * n;
+    }
+
+    function Example3({n}) {
+      return 3 * n;
+    }
+
+    ReactDOM.render(<Example1 n={1} />, container);
+    expect(container.textContent).toBe('1');
+    expect(container2.textContent).toBe('');
+    expect(container3.textContent).toBe('');
+    jest.runAllTimers();
+    expect(container.textContent).toBe('1');
+    expect(container2.textContent).toBe('2');
+    expect(container3.textContent).toBe('3');
+
+    ReactDOM.render(<Example1 n={2} />, container);
+    expect(container.textContent).toBe('2');
+    expect(container2.textContent).toBe('2'); // Not flushed yet
+    expect(container3.textContent).toBe('3'); // Not flushed yet
+    jest.runAllTimers();
+    expect(container.textContent).toBe('2');
+    expect(container2.textContent).toBe('4');
+    expect(container3.textContent).toBe('6');
+  });
+
+  it('can batch synchronous work inside effects with other work', () => {
+    let otherContainer = document.createElement('div');
+
+    let calledA = false;
+    function A() {
+      calledA = true;
+      return 'A';
+    }
+
+    let calledB = false;
+    function B() {
+      calledB = true;
+      return 'B';
+    }
+
+    let _set;
+    function Foo() {
+      _set = React.useState(0)[1];
+      React.useEffect(() => {
+        ReactDOM.render(<A />, otherContainer);
+      });
+      return null;
+    }
+
+    ReactDOM.render(<Foo />, container);
+    ReactDOM.unstable_batchedUpdates(() => {
+      _set(0); // Forces the effect to be flushed
+      expect(otherContainer.textContent).toBe('');
+      ReactDOM.render(<B />, otherContainer);
+      expect(otherContainer.textContent).toBe('');
+    });
+    expect(otherContainer.textContent).toBe('B');
+    expect(calledA).toBe(false); // It was in a batch
+    expect(calledB).toBe(true);
+  });
+
   it('should not bail out when an update is scheduled from within an event handler', () => {
     const {createRef, useCallback, useState} = React;
 
diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -575,6 +575,10 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
   if (rootExpirationTime !== NoWork) {
     requestWork(root, rootExpirationTime);
   }
+  // Flush any sync work that was scheduled by effects
+  if (!isBatchingUpdates && !isRendering) {
+    performSyncWork();
+  }
 }
 
 function isAlreadyFailedLegacyErrorBoundary(instance: mixed): boolean {

Original file line number	Diff line number	Diff line change
`@@ -575,6 +575,10 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {`
`575`	`575`	`if (rootExpirationTime !== NoWork) {`
`576`	`576`	`requestWork(root, rootExpirationTime);`
`577`	`577`	`}`
	`578`	`+ // Flush any sync work that was scheduled by effects`
	`579`	`+ if (!isBatchingUpdates && !isRendering) {`
	`580`	`+ performSyncWork();`
	`581`	`+ }`
`578`	`582`	`}`
`579`	`583`
`580`	`584`	`function isAlreadyFailedLegacyErrorBoundary(instance: mixed): boolean {`