@samitouri / QOS-React / commits / d2f5c3fbc2

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

* Add a regression test for 12643#issuecomment-413727104 * Don't diff memoized host components * Add regression tests for noop renderer * No early return * Strengthen the test for host siblings * Flow types

Dan Abramov committed Aug 17, 2018 at 18:13 UTC d2f5c3fbc211a2ff257c43c879f3213d95f31222
5 files changed +345 -104
packages/react-dom/src/__tests__/ReactDOMFiber-test.js
+125 -67
@@ -18,6 +18,12 @@ describe('ReactDOMFiber', () => {
18
19 beforeEach(() => {
20 container = document.createElement('div');
21 + document.body.appendChild(container);
22 + });
23 +
24 + afterEach(() => {
25 + document.body.removeChild(container);
26 + container = null;
27 });
28
29 it('should render strings as children', () => {
@@ -205,12 +211,12 @@ describe('ReactDOMFiber', () => {
211 };
212
213 const assertNamespacesMatch = function(tree) {
208 - container = document.createElement('div');
214 + let testContainer = document.createElement('div');
215 svgEls = [];
216 htmlEls = [];
217 mathEls = [];
218
213 - ReactDOM.render(tree, container);
219 + ReactDOM.render(tree, testContainer);
220 svgEls.forEach(el => {
221 expect(el.namespaceURI).toBe('http://www.w3.org/2000/svg');
222 });
@@ -221,8 +227,8 @@ describe('ReactDOMFiber', () => {
227 expect(el.namespaceURI).toBe('http://www.w3.org/1998/Math/MathML');
228 });
229
224 - ReactDOM.unmountComponentAtNode(container);
225 - expect(container.innerHTML).toBe('');
230 + ReactDOM.unmountComponentAtNode(testContainer);
231 + expect(testContainer.innerHTML).toBe('');
232 };
233
234 it('should render one portal', () => {
@@ -874,7 +880,6 @@ describe('ReactDOMFiber', () => {
880
881 it('should not onMouseLeave when staying in the portal', () => {
882 const portalContainer = document.createElement('div');
877 - document.body.appendChild(container);
883 document.body.appendChild(portalContainer);
884
885 let ops = [];
@@ -944,7 +949,6 @@ describe('ReactDOMFiber', () => {
949 'leave parent', // Only when we leave the portal does onMouseLeave fire.
950 ]);
951 } finally {
947 - document.body.removeChild(container);
952 document.body.removeChild(portalContainer);
953 }
954 });
@@ -987,82 +991,77 @@ describe('ReactDOMFiber', () => {
991 });
992
993 it('should not update event handlers until commit', () => {
990 - document.body.appendChild(container);
991 - try {
992 - let ops = [];
993 - const handlerA = () => ops.push('A');
994 - const handlerB = () => ops.push('B');
995 -
996 - class Example extends React.Component {
997 - state = {flip: false, count: 0};
998 - flip() {
999 - this.setState({flip: true, count: this.state.count + 1});
1000 - }
1001 - tick() {
1002 - this.setState({count: this.state.count + 1});
1003 - }
1004 - render() {
1005 - const useB = !this.props.forceA && this.state.flip;
1006 - return <div onClick={useB ? handlerB : handlerA} />;
1007 - }
994 + let ops = [];
995 + const handlerA = () => ops.push('A');
996 + const handlerB = () => ops.push('B');
997 +
998 + class Example extends React.Component {
999 + state = {flip: false, count: 0};
1000 + flip() {
1001 + this.setState({flip: true, count: this.state.count + 1});
1002 }
1003 + tick() {
1004 + this.setState({count: this.state.count + 1});
1005 + }
1006 + render() {
1007 + const useB = !this.props.forceA && this.state.flip;
1008 + return <div onClick={useB ? handlerB : handlerA} />;
1009 + }
1010 + }
1011
1010 - class Click extends React.Component {
1011 - constructor() {
1012 - super();
1013 - node.click();
1014 - }
1015 - render() {
1016 - return null;
1017 - }
1012 + class Click extends React.Component {
1013 + constructor() {
1014 + super();
1015 + node.click();
1016 }
1017 + render() {
1018 + return null;
1019 + }
1020 + }
1021
1020 - let inst;
1021 - ReactDOM.render([<Example key="a" ref={n => (inst = n)} />], container);
1022 - const node = container.firstChild;
1023 - expect(node.tagName).toEqual('DIV');
1022 + let inst;
1023 + ReactDOM.render([<Example key="a" ref={n => (inst = n)} />], container);
1024 + const node = container.firstChild;
1025 + expect(node.tagName).toEqual('DIV');
1026
1025 - node.click();
1027 + node.click();
1028
1027 - expect(ops).toEqual(['A']);
1028 - ops = [];
1029 + expect(ops).toEqual(['A']);
1030 + ops = [];
1031
1030 - // Render with the other event handler.
1031 - inst.flip();
1032 + // Render with the other event handler.
1033 + inst.flip();
1034
1033 - node.click();
1035 + node.click();
1036
1035 - expect(ops).toEqual(['B']);
1036 - ops = [];
1037 + expect(ops).toEqual(['B']);
1038 + ops = [];
1039
1038 - // Rerender without changing any props.
1039 - inst.tick();
1040 + // Rerender without changing any props.
1041 + inst.tick();
1042
1041 - node.click();
1043 + node.click();
1044
1043 - expect(ops).toEqual(['B']);
1044 - ops = [];
1045 + expect(ops).toEqual(['B']);
1046 + ops = [];
1047
1046 - // Render a flip back to the A handler. The second component invokes the
1047 - // click handler during render to simulate a click during an aborted
1048 - // render. I use this hack because at current time we don't have a way to
1049 - // test aborted ReactDOM renders.
1050 - ReactDOM.render(
1051 - [<Example key="a" forceA={true} />, <Click key="b" />],
1052 - container,
1053 - );
1048 + // Render a flip back to the A handler. The second component invokes the
1049 + // click handler during render to simulate a click during an aborted
1050 + // render. I use this hack because at current time we don't have a way to
1051 + // test aborted ReactDOM renders.
1052 + ReactDOM.render(
1053 + [<Example key="a" forceA={true} />, <Click key="b" />],
1054 + container,
1055 + );
1056
1055 - // Because the new click handler has not yet committed, we should still
1056 - // invoke B.
1057 - expect(ops).toEqual(['B']);
1058 - ops = [];
1057 + // Because the new click handler has not yet committed, we should still
1058 + // invoke B.
1059 + expect(ops).toEqual(['B']);
1060 + ops = [];
1061
1060 - // Any click that happens after commit, should invoke A.
1061 - node.click();
1062 - expect(ops).toEqual(['A']);
1063 - } finally {
1064 - document.body.removeChild(container);
1065 - }
1062 + // Any click that happens after commit, should invoke A.
1063 + node.click();
1064 + expect(ops).toEqual(['A']);
1065 });
1066
1067 it('should not crash encountering low-priority tree', () => {
@@ -1178,4 +1177,63 @@ describe('ReactDOMFiber', () => {
1177 container.appendChild(fragment);
1178 expect(container.innerHTML).toBe('<div>foo</div>');
1179 });
1180 +
1181 + // Regression test for https://github.com/facebook/react/issues/12643#issuecomment-413727104
1182 + it('should not diff memoized host components', () => {
1183 + let inputRef = React.createRef();
1184 + let didCallOnChange = false;
1185 +
1186 + class Child extends React.Component {
1187 + state = {};
1188 + componentDidMount() {
1189 + document.addEventListener('click', this.update, true);
1190 + }
1191 + componentWillUnmount() {
1192 + document.removeEventListener('click', this.update, true);
1193 + }
1194 + update = () => {
1195 + // We're testing that this setState()
1196 + // doesn't cause React to commit updates
1197 + // to the input outside (which would itself
1198 + // prevent the parent's onChange parent handler
1199 + // from firing).
1200 + this.setState({});
1201 + // Note that onChange was always broken when there was an
1202 + // earlier setState() in a manual document capture phase
1203 + // listener *in the same component*. But that's very rare.
1204 + // Here we're testing that a *child* component doesn't break
1205 + // the parent if this happens.
1206 + };
1207 + render() {
1208 + return <div />;
1209 + }
1210 + }
1211 +
1212 + class Parent extends React.Component {
1213 + handleChange = val => {
1214 + didCallOnChange = true;
1215 + };
1216 + render() {
1217 + return (
1218 + <div>
1219 + <Child />
1220 + <input
1221 + ref={inputRef}
1222 + type="checkbox"
1223 + checked={true}
1224 + onChange={this.handleChange}
1225 + />
1226 + </div>
1227 + );
1228 + }
1229 + }
1230 +
1231 + ReactDOM.render(<Parent />, container);
1232 + inputRef.current.dispatchEvent(
1233 + new MouseEvent('click', {
1234 + bubbles: true,
1235 + }),
1236 + );
1237 + expect(didCallOnChange).toBe(true);
1238 + });
1239 });
packages/react-noop-renderer/src/createReactNoop.js
+25
@@ -41,6 +41,8 @@ if (__DEV__) {
41 function createReactNoop(reconciler: Function, useMutation: boolean) {
42 let scheduledCallback = null;
43 let instanceCounter = 0;
44 + let hostDiffCounter = 0;
45 + let hostUpdateCounter = 0;
46
47 function appendChildToContainerOrInstance(
48 parentInstance: Container | Instance,
@@ -220,6 +222,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
222 if (newProps === null) {
223 throw new Error('Should have new props');
224 }
225 + hostDiffCounter++;
226 return UPDATE_SIGNAL;
227 },
228
@@ -303,6 +306,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
306 if (oldProps === null) {
307 throw new Error('Should have old props');
308 }
309 + hostUpdateCounter++;
310 instance.prop = newProps.prop;
311 },
312
@@ -311,6 +315,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
315 oldText: string,
316 newText: string,
317 ): void {
318 + hostUpdateCounter++;
319 textInstance.text = newText;
320 },
321
@@ -556,6 +561,26 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
561 return actual !== null ? actual : [];
562 },
563
564 + flushWithHostCounters(
565 + fn: () => void,
566 + ): {|
567 + hostDiffCounter: number,
568 + hostUpdateCounter: number,
569 + |} {
570 + hostDiffCounter = 0;
571 + hostUpdateCounter = 0;
572 + try {
573 + ReactNoop.flush();
574 + return {
575 + hostDiffCounter,
576 + hostUpdateCounter,
577 + };
578 + } finally {
579 + hostDiffCounter = 0;
580 + hostUpdateCounter = 0;
581 + }
582 + },
583 +
584 expire(ms: number): Array<mixed> {
585 ReactNoop.advanceTime(ms);
586 return ReactNoop.flushExpired();
packages/react-reconciler/src/ReactFiberCompleteWork.js
+29 -27
@@ -361,34 +361,36 @@ function completeWork(
361 // If we have an alternate, that means this is an update and we need to
362 // schedule a side-effect to do the updates.
363 const oldProps = current.memoizedProps;
364 - // If we get updated because one of our children updated, we don't
365 - // have newProps so we'll have to reuse them.
366 - // TODO: Split the update API as separate for the props vs. children.
367 - // Even better would be if children weren't special cased at all tho.
368 - const instance: Instance = workInProgress.stateNode;
369 - const currentHostContext = getHostContext();
370 - // TODO: Experiencing an error where oldProps is null. Suggests a host
371 - // component is hitting the resume path. Figure out why. Possibly
372 - // related to `hidden`.
373 - const updatePayload = prepareUpdate(
374 - instance,
375 - type,
376 - oldProps,
377 - newProps,
378 - rootContainerInstance,
379 - currentHostContext,
380 - );
364 + if (oldProps !== newProps) {
365 + // If we get updated because one of our children updated, we don't
366 + // have newProps so we'll have to reuse them.
367 + // TODO: Split the update API as separate for the props vs. children.
368 + // Even better would be if children weren't special cased at all tho.
369 + const instance: Instance = workInProgress.stateNode;
370 + const currentHostContext = getHostContext();
371 + // TODO: Experiencing an error where oldProps is null. Suggests a host
372 + // component is hitting the resume path. Figure out why. Possibly
373 + // related to `hidden`.
374 + const updatePayload = prepareUpdate(
375 + instance,
376 + type,
377 + oldProps,
378 + newProps,
379 + rootContainerInstance,
380 + currentHostContext,
381 + );
382
382 - updateHostComponent(
383 - current,
384 - workInProgress,
385 - updatePayload,
386 - type,
387 - oldProps,
388 - newProps,
389 - rootContainerInstance,
390 - currentHostContext,
391 - );
383 + updateHostComponent(
384 + current,
385 + workInProgress,
386 + updatePayload,
387 + type,
388 + oldProps,
389 + newProps,
390 + rootContainerInstance,
391 + currentHostContext,
392 + );
393 + }
394
395 if (current.ref !== workInProgress.ref) {
396 markRef(workInProgress);
packages/react-reconciler/src/__tests__/ReactIncrementalUpdatesMinimalism-test.js new
+156
@@ -0,0 +1,156 @@
1 +/**
2 + * Copyright (c) 2013-present, Facebook, Inc.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + *
7 + * @emails react-core
8 + * @jest-environment node
9 + */
10 +
11 +'use strict';
12 +
13 +let React;
14 +let ReactNoop;
15 +
16 +describe('ReactIncrementalUpdatesMinimalism', () => {
17 + beforeEach(() => {
18 + jest.resetModules();
19 + React = require('react');
20 + ReactNoop = require('react-noop-renderer');
21 + });
22 +
23 + it('should render a simple component', () => {
24 + function Child() {
25 + return <div>Hello World</div>;
26 + }
27 +
28 + function Parent() {
29 + return <Child />;
30 + }
31 +
32 + ReactNoop.render(<Parent />);
33 + expect(ReactNoop.flushWithHostCounters()).toEqual({
34 + hostDiffCounter: 0,
35 + hostUpdateCounter: 0,
36 + });
37 +
38 + ReactNoop.render(<Parent />);
39 + expect(ReactNoop.flushWithHostCounters()).toEqual({
40 + hostDiffCounter: 1,
41 + hostUpdateCounter: 1,
42 + });
43 + });
44 +
45 + it('should not diff referentially equal host elements', () => {
46 + function Leaf(props) {
47 + return (
48 + <span>
49 + hello
50 + <b />
51 + {props.name}
52 + </span>
53 + );
54 + }
55 +
56 + const constEl = (
57 + <div>
58 + <Leaf name="world" />
59 + </div>
60 + );
61 +
62 + function Child() {
63 + return constEl;
64 + }
65 +
66 + function Parent() {
67 + return <Child />;
68 + }
69 +
70 + ReactNoop.render(<Parent />);
71 + expect(ReactNoop.flushWithHostCounters()).toEqual({
72 + hostDiffCounter: 0,
73 + hostUpdateCounter: 0,
74 + });
75 +
76 + ReactNoop.render(<Parent />);
77 + expect(ReactNoop.flushWithHostCounters()).toEqual({
78 + hostDiffCounter: 0,
79 + hostUpdateCounter: 0,
80 + });
81 + });
82 +
83 + it('should not diff parents of setState targets', () => {
84 + let childInst;
85 +
86 + function Leaf(props) {
87 + return (
88 + <span>
89 + hello
90 + <b />
91 + {props.name}
92 + </span>
93 + );
94 + }
95 +
96 + class Child extends React.Component {
97 + state = {name: 'Batman'};
98 + render() {
99 + childInst = this;
100 + return (
101 + <div>
102 + <Leaf name={this.state.name} />
103 + </div>
104 + );
105 + }
106 + }
107 +
108 + function Parent() {
109 + return (
110 + <section>
111 + <div>
112 + <Leaf name="world" />
113 + <Child />
114 + <hr />
115 + <Leaf name="world" />
116 + </div>
117 + </section>
118 + );
119 + }
120 +
121 + ReactNoop.render(<Parent />);
122 + expect(ReactNoop.flushWithHostCounters()).toEqual({
123 + hostDiffCounter: 0,
124 + hostUpdateCounter: 0,
125 + });
126 +
127 + childInst.setState({name: 'Robin'});
128 + expect(ReactNoop.flushWithHostCounters()).toEqual({
129 + // Child > div
130 + // Child > Leaf > span
131 + // Child > Leaf > span > b
132 + hostDiffCounter: 3,
133 + // Child > div
134 + // Child > Leaf > span
135 + // Child > Leaf > span > b
136 + // Child > Leaf > span > #text
137 + hostUpdateCounter: 4,
138 + });
139 +
140 + ReactNoop.render(<Parent />);
141 + expect(ReactNoop.flushWithHostCounters()).toEqual({
142 + // Parent > section
143 + // Parent > section > div
144 + // Parent > section > div > Leaf > span
145 + // Parent > section > div > Leaf > span > b
146 + // Parent > section > div > Child > div
147 + // Parent > section > div > Child > div > Leaf > span
148 + // Parent > section > div > Child > div > Leaf > span > b
149 + // Parent > section > div > hr
150 + // Parent > section > div > Leaf > span
151 + // Parent > section > div > Leaf > span > b
152 + hostDiffCounter: 10,
153 + hostUpdateCounter: 10,
154 + });
155 + });
156 +});
packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap
+10 -10
@@ -85,8 +85,8 @@ exports[`ReactDebugFiberPerf deduplicates lifecycle names during commit to reduc
85
86 ⚛ (Committing Changes)
87 ⚛ (Committing Snapshot Effects: 0 Total)
88 - ⚛ (Committing Host Effects: 3 Total)
89 - ⚛ (Calling Lifecycle Methods: 3 Total)
88 + ⚛ (Committing Host Effects: 2 Total)
89 + ⚛ (Calling Lifecycle Methods: 2 Total)
90 ⚛ B.componentDidUpdate
91 "
92 `;
@@ -257,8 +257,8 @@ exports[`ReactDebugFiberPerf measures deprioritized work 1`] = `
257
258 ⚛ (Committing Changes)
259 ⚛ (Committing Snapshot Effects: 0 Total)
260 - ⚛ (Committing Host Effects: 3 Total)
261 - ⚛ (Calling Lifecycle Methods: 2 Total)
260 + ⚛ (Committing Host Effects: 1 Total)
261 + ⚛ (Calling Lifecycle Methods: 0 Total)
262 "
263 `;
264
@@ -311,8 +311,8 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = `
311
312 ⚛ (Committing Changes)
313 ⚛ (Committing Snapshot Effects: 0 Total)
314 - ⚛ (Committing Host Effects: 2 Total)
315 - ⚛ (Calling Lifecycle Methods: 1 Total)
314 + ⚛ (Committing Host Effects: 1 Total)
315 + ⚛ (Calling Lifecycle Methods: 0 Total)
316 "
317 `;
318
@@ -357,8 +357,8 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = `
357
358 ⚛ (Committing Changes)
359 ⚛ (Committing Snapshot Effects: 0 Total)
360 - ⚛ (Committing Host Effects: 6 Total)
361 - ⚛ (Calling Lifecycle Methods: 6 Total)
360 + ⚛ (Committing Host Effects: 2 Total)
361 + ⚛ (Calling Lifecycle Methods: 2 Total)
362 "
363 `;
364
@@ -431,8 +431,8 @@ exports[`ReactDebugFiberPerf warns on cascading renders from setState 1`] = `
431
432 ⚛ (Committing Changes)
433 ⚛ (Committing Snapshot Effects: 0 Total)
434 - ⚛ (Committing Host Effects: 2 Total)
435 - ⚛ (Calling Lifecycle Methods: 2 Total)
434 + ⚛ (Committing Host Effects: 1 Total)
435 + ⚛ (Calling Lifecycle Methods: 1 Total)
436 "
437 `;
438