Skip to content

Commit

Permalink
Fix ignored sync work in passive effects (#14799)
Browse files Browse the repository at this point in the history
* Fix ignored sync work in passive effects

* Fix batching
  • Loading branch information
gaearon authored Feb 12, 2019
1 parent f3a1495 commit 3ae94e1
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 1 deletion.
79 changes: 78 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMHooks-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
let React;
let ReactDOM;

describe('ReactDOMSuspensePlaceholder', () => {
describe('ReactDOMHooks', () => {
let container;

beforeEach(() => {
Expand All @@ -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;

Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 3ae94e1

Please sign in to comment.