From 3ae94e1885b673543a30a05906c4f9a0e4b682cb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 12 Feb 2019 20:18:35 +0000 Subject: [PATCH] Fix ignored sync work in passive effects (#14799) * Fix ignored sync work in passive effects * Fix batching --- .../src/__tests__/ReactDOMHooks-test.js | 79 ++++++++++++++++++- .../src/ReactFiberScheduler.js | 4 + 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js index d317e407e2e1b..fe6caa82198da 100644 --- 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(, container2); + }); + return 1 * n; + } + + function Example2({n}) { + React.useEffect(() => { + ReactDOM.render(, container3); + }); + return 2 * n; + } + + function Example3({n}) { + return 3 * n; + } + + ReactDOM.render(, 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(, 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(, otherContainer); + }); + return null; + } + + ReactDOM.render(, container); + ReactDOM.unstable_batchedUpdates(() => { + _set(0); // Forces the effect to be flushed + expect(otherContainer.textContent).toBe(''); + ReactDOM.render(, 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 index 94426f45298de..c86b66d2b8b38 100644 --- 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 {