Skip to content

Don't diff memoized host components in completion phase #13423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 17, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 125 additions & 67 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
@@ -18,6 +18,12 @@ describe('ReactDOMFiber', () => {

beforeEach(() => {
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
document.body.removeChild(container);
container = null;
});

it('should render strings as children', () => {
@@ -205,12 +211,12 @@ describe('ReactDOMFiber', () => {
};

const assertNamespacesMatch = function(tree) {
container = document.createElement('div');
let testContainer = document.createElement('div');
svgEls = [];
htmlEls = [];
mathEls = [];

ReactDOM.render(tree, container);
ReactDOM.render(tree, testContainer);
svgEls.forEach(el => {
expect(el.namespaceURI).toBe('http://www.w3.org/2000/svg');
});
@@ -221,8 +227,8 @@ describe('ReactDOMFiber', () => {
expect(el.namespaceURI).toBe('http://www.w3.org/1998/Math/MathML');
});

ReactDOM.unmountComponentAtNode(container);
expect(container.innerHTML).toBe('');
ReactDOM.unmountComponentAtNode(testContainer);
expect(testContainer.innerHTML).toBe('');
};

it('should render one portal', () => {
@@ -874,7 +880,6 @@ describe('ReactDOMFiber', () => {

it('should not onMouseLeave when staying in the portal', () => {
const portalContainer = document.createElement('div');
document.body.appendChild(container);
document.body.appendChild(portalContainer);

let ops = [];
@@ -944,7 +949,6 @@ describe('ReactDOMFiber', () => {
'leave parent', // Only when we leave the portal does onMouseLeave fire.
]);
} finally {
document.body.removeChild(container);
document.body.removeChild(portalContainer);
}
});
@@ -987,82 +991,77 @@ describe('ReactDOMFiber', () => {
});

it('should not update event handlers until commit', () => {
document.body.appendChild(container);
try {
let ops = [];
const handlerA = () => ops.push('A');
const handlerB = () => ops.push('B');

class Example extends React.Component {
state = {flip: false, count: 0};
flip() {
this.setState({flip: true, count: this.state.count + 1});
}
tick() {
this.setState({count: this.state.count + 1});
}
render() {
const useB = !this.props.forceA && this.state.flip;
return <div onClick={useB ? handlerB : handlerA} />;
}
let ops = [];
const handlerA = () => ops.push('A');
const handlerB = () => ops.push('B');

class Example extends React.Component {
state = {flip: false, count: 0};
flip() {
this.setState({flip: true, count: this.state.count + 1});
}
tick() {
this.setState({count: this.state.count + 1});
}
render() {
const useB = !this.props.forceA && this.state.flip;
return <div onClick={useB ? handlerB : handlerA} />;
}
}

class Click extends React.Component {
constructor() {
super();
node.click();
}
render() {
return null;
}
class Click extends React.Component {
constructor() {
super();
node.click();
}
render() {
return null;
}
}

let inst;
ReactDOM.render([<Example key="a" ref={n => (inst = n)} />], container);
const node = container.firstChild;
expect(node.tagName).toEqual('DIV');
let inst;
ReactDOM.render([<Example key="a" ref={n => (inst = n)} />], container);
const node = container.firstChild;
expect(node.tagName).toEqual('DIV');

node.click();
node.click();

expect(ops).toEqual(['A']);
ops = [];
expect(ops).toEqual(['A']);
ops = [];

// Render with the other event handler.
inst.flip();
// Render with the other event handler.
inst.flip();

node.click();
node.click();

expect(ops).toEqual(['B']);
ops = [];
expect(ops).toEqual(['B']);
ops = [];

// Rerender without changing any props.
inst.tick();
// Rerender without changing any props.
inst.tick();

node.click();
node.click();

expect(ops).toEqual(['B']);
ops = [];
expect(ops).toEqual(['B']);
ops = [];

// Render a flip back to the A handler. The second component invokes the
// click handler during render to simulate a click during an aborted
// render. I use this hack because at current time we don't have a way to
// test aborted ReactDOM renders.
ReactDOM.render(
[<Example key="a" forceA={true} />, <Click key="b" />],
container,
);
// Render a flip back to the A handler. The second component invokes the
// click handler during render to simulate a click during an aborted
// render. I use this hack because at current time we don't have a way to
// test aborted ReactDOM renders.
ReactDOM.render(
[<Example key="a" forceA={true} />, <Click key="b" />],
container,
);

// Because the new click handler has not yet committed, we should still
// invoke B.
expect(ops).toEqual(['B']);
ops = [];
// Because the new click handler has not yet committed, we should still
// invoke B.
expect(ops).toEqual(['B']);
ops = [];

// Any click that happens after commit, should invoke A.
node.click();
expect(ops).toEqual(['A']);
} finally {
document.body.removeChild(container);
}
// Any click that happens after commit, should invoke A.
node.click();
expect(ops).toEqual(['A']);
});

it('should not crash encountering low-priority tree', () => {
@@ -1178,4 +1177,63 @@ describe('ReactDOMFiber', () => {
container.appendChild(fragment);
expect(container.innerHTML).toBe('<div>foo</div>');
});

// Regression test for https://github.com/facebook/react/issues/12643#issuecomment-413727104
it('should not diff memoized host components', () => {
let inputRef = React.createRef();
let didCallOnChange = false;

class Child extends React.Component {
state = {};
componentDidMount() {
document.addEventListener('click', this.update, true);
}
componentWillUnmount() {
document.removeEventListener('click', this.update, true);
}
update = () => {
// We're testing that this setState()
// doesn't cause React to commit updates
// to the input outside (which would itself
// prevent the parent's onChange parent handler
// from firing).
this.setState({});
// Note that onChange was always broken when there was an
// earlier setState() in a manual document capture phase
// listener *in the same component*. But that's very rare.
// Here we're testing that a *child* component doesn't break
// the parent if this happens.
};
render() {
return <div />;
}
}

class Parent extends React.Component {
handleChange = val => {
didCallOnChange = true;
};
render() {
return (
<div>
<Child />
<input
ref={inputRef}
type="checkbox"
checked={true}
onChange={this.handleChange}
/>
</div>
);
}
}

ReactDOM.render(<Parent />, container);
inputRef.current.dispatchEvent(
new MouseEvent('click', {
bubbles: true,
}),
);
expect(didCallOnChange).toBe(true);
});
});
25 changes: 25 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
@@ -41,6 +41,8 @@ if (__DEV__) {
function createReactNoop(reconciler: Function, useMutation: boolean) {
let scheduledCallback = null;
let instanceCounter = 0;
let hostDiffCounter = 0;
let hostUpdateCounter = 0;

function appendChildToContainerOrInstance(
parentInstance: Container | Instance,
@@ -220,6 +222,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
if (newProps === null) {
throw new Error('Should have new props');
}
hostDiffCounter++;
return UPDATE_SIGNAL;
},

@@ -303,6 +306,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
if (oldProps === null) {
throw new Error('Should have old props');
}
hostUpdateCounter++;
instance.prop = newProps.prop;
},

@@ -311,6 +315,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
oldText: string,
newText: string,
): void {
hostUpdateCounter++;
textInstance.text = newText;
},

@@ -556,6 +561,26 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
return actual !== null ? actual : [];
},

flushWithHostCounters(
fn: () => void,
): {|
hostDiffCounter: number,
hostUpdateCounter: number,
|} {
hostDiffCounter = 0;
hostUpdateCounter = 0;
try {
ReactNoop.flush();
return {
hostDiffCounter,
hostUpdateCounter,
};
} finally {
hostDiffCounter = 0;
hostUpdateCounter = 0;
}
},

expire(ms: number): Array<mixed> {
ReactNoop.advanceTime(ms);
return ReactNoop.flushExpired();
56 changes: 29 additions & 27 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
@@ -361,34 +361,36 @@ function completeWork(
// If we have an alternate, that means this is an update and we need to
// schedule a side-effect to do the updates.
const oldProps = current.memoizedProps;
// If we get updated because one of our children updated, we don't
// have newProps so we'll have to reuse them.
// TODO: Split the update API as separate for the props vs. children.
// Even better would be if children weren't special cased at all tho.
const instance: Instance = workInProgress.stateNode;
const currentHostContext = getHostContext();
// TODO: Experiencing an error where oldProps is null. Suggests a host
// component is hitting the resume path. Figure out why. Possibly
// related to `hidden`.
const updatePayload = prepareUpdate(
instance,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);
if (oldProps !== newProps) {
// If we get updated because one of our children updated, we don't
// have newProps so we'll have to reuse them.
// TODO: Split the update API as separate for the props vs. children.
// Even better would be if children weren't special cased at all tho.
const instance: Instance = workInProgress.stateNode;
const currentHostContext = getHostContext();
// TODO: Experiencing an error where oldProps is null. Suggests a host
// component is hitting the resume path. Figure out why. Possibly
// related to `hidden`.
const updatePayload = prepareUpdate(
instance,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);

updateHostComponent(
current,
workInProgress,
updatePayload,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);
updateHostComponent(
current,
workInProgress,
updatePayload,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);
}

if (current.ref !== workInProgress.ref) {
markRef(workInProgress);
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
* @jest-environment node
*/

'use strict';

let React;
let ReactNoop;

describe('ReactIncrementalUpdatesMinimalism', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactNoop = require('react-noop-renderer');
});

it('should render a simple component', () => {
function Child() {
return <div>Hello World</div>;
}

function Parent() {
return <Child />;
}

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
hostDiffCounter: 0,
hostUpdateCounter: 0,
});

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
hostDiffCounter: 1,
hostUpdateCounter: 1,
});
});

it('should not diff referentially equal host elements', () => {
function Leaf(props) {
return (
<span>
hello
<b />
{props.name}
</span>
);
}

const constEl = (
<div>
<Leaf name="world" />
</div>
);

function Child() {
return constEl;
}

function Parent() {
return <Child />;
}

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
hostDiffCounter: 0,
hostUpdateCounter: 0,
});

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
hostDiffCounter: 0,
hostUpdateCounter: 0,
});
});

it('should not diff parents of setState targets', () => {
let childInst;

function Leaf(props) {
return (
<span>
hello
<b />
{props.name}
</span>
);
}

class Child extends React.Component {
state = {name: 'Batman'};
render() {
childInst = this;
return (
<div>
<Leaf name={this.state.name} />
</div>
);
}
}

function Parent() {
return (
<section>
<div>
<Leaf name="world" />
<Child />
<hr />
<Leaf name="world" />
</div>
</section>
);
}

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
hostDiffCounter: 0,
hostUpdateCounter: 0,
});

childInst.setState({name: 'Robin'});
expect(ReactNoop.flushWithHostCounters()).toEqual({
// Child > div
// Child > Leaf > span
// Child > Leaf > span > b
hostDiffCounter: 3,
// Child > div
// Child > Leaf > span
// Child > Leaf > span > b
// Child > Leaf > span > #text
hostUpdateCounter: 4,
});

ReactNoop.render(<Parent />);
expect(ReactNoop.flushWithHostCounters()).toEqual({
// Parent > section
// Parent > section > div
// Parent > section > div > Leaf > span
// Parent > section > div > Leaf > span > b
// Parent > section > div > Child > div
// Parent > section > div > Child > div > Leaf > span
// Parent > section > div > Child > div > Leaf > span > b
// Parent > section > div > hr
// Parent > section > div > Leaf > span
// Parent > section > div > Leaf > span > b
hostDiffCounter: 10,
hostUpdateCounter: 10,
});
});
});
Original file line number Diff line number Diff line change
@@ -85,8 +85,8 @@ exports[`ReactDebugFiberPerf deduplicates lifecycle names during commit to reduc
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 3 Total)
⚛ (Calling Lifecycle Methods: 3 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 2 Total)
⚛ B.componentDidUpdate
"
`;
@@ -257,8 +257,8 @@ exports[`ReactDebugFiberPerf measures deprioritized work 1`] = `
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 3 Total)
⚛ (Calling Lifecycle Methods: 2 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
"
`;

@@ -311,8 +311,8 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = `
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 1 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
"
`;

@@ -357,8 +357,8 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = `
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 6 Total)
⚛ (Calling Lifecycle Methods: 6 Total)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have spotted this earlier. Sigh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just way more efficient now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not — I think it was still overcounting actual updates (not entirely sure why). But it's better.

⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 2 Total)
"
`;

@@ -431,8 +431,8 @@ exports[`ReactDebugFiberPerf warns on cascading renders from setState 1`] = `
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 2 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 1 Total)
"
`;