@samitouri / QOS-React / commits / ba8f24ba99

Prepare new composite child before removing old (#8572)

This matches what we do in Fiber -- and doing it this way is the only way we can prepare new views in the background before unmounting old ones. In particular, this breaks this pattern: ```js class Child1 extends React.Component { render() { ... } componentWillMount() { this.props.registerChild(this); } componentWillUnmount() { this.props.unregisterChild(); } } class Child2 extends React.Component { render() { ... } componentWillMount() { this.props.registerChild(this); } componentWillUnmount() { this.props.unregisterChild(); } } class Parent extends React.Component { render() { return ( showChild1 ? <Child1 registerChild={(child) => this.registered = child} unregisterChild={() => this.registered = null} /> : <Child2 registerChild={(child) => this.registered = child} unregisterChild={() => this.registered = null} /> ); } } ``` Previously, `this.registered` would always be set -- now, after a rerender, `this.registered` gets stuck at null because the old child's componentWillUnmount runs *after* the new child's componentWillMount. A correct fix here is to use componentDidMount rather than componentWillMount. (In general, componentWillMount should not have side effects.) If Parent stored a list or set of registered children instead, there would also be no issue.

Ben Alpert committed Dec 14, 2016 at 11:14 UTC ba8f24ba9937c6cec70e4cb00aa2ded02f7d5cdc
7 files changed +155 -8
scripts/fiber/tests-passing.txt
+2
@@ -1318,6 +1318,7 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
1318 * should support objects with prototypes as state
1319 * should not warn about unmounting during unmounting
1320 * should only call componentWillUnmount once
1321 +* prepares new child before unmounting old
1322
1323 src/renderers/shared/shared/__tests__/ReactCompositeComponentDOMMinimalism-test.js
1324 * should not render extra nodes for non-interpolated text
@@ -1405,6 +1406,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
1406 * should NOT replace children with different owners
1407 * should replace children with different keys
1408 * should reorder bailed-out children
1409 +* prepares new children before unmounting old
1410
1411 src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js
1412 * should reset internal state if removed then readded in an array
src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
+41
@@ -1358,4 +1358,45 @@ describe('ReactCompositeComponent', () => {
1358 expect(count).toBe(1);
1359 });
1360
1361 + it('prepares new child before unmounting old', () => {
1362 + var log = [];
1363 +
1364 + class Spy extends React.Component {
1365 + componentWillMount() {
1366 + log.push(this.props.name + ' componentWillMount');
1367 + }
1368 + render() {
1369 + log.push(this.props.name + ' render');
1370 + return <div />;
1371 + }
1372 + componentDidMount() {
1373 + log.push(this.props.name + ' componentDidMount');
1374 + }
1375 + componentWillUnmount() {
1376 + log.push(this.props.name + ' componentWillUnmount');
1377 + }
1378 + }
1379 +
1380 + class Wrapper extends React.Component {
1381 + render() {
1382 + return <Spy key={this.props.name} name={this.props.name} />;
1383 + }
1384 + }
1385 +
1386 + var container = document.createElement('div');
1387 + ReactDOM.render(<Wrapper name="A" />, container);
1388 + ReactDOM.render(<Wrapper name="B" />, container);
1389 +
1390 + expect(log).toEqual([
1391 + 'A componentWillMount',
1392 + 'A render',
1393 + 'A componentDidMount',
1394 +
1395 + 'B componentWillMount',
1396 + 'B render',
1397 + 'A componentWillUnmount',
1398 + 'B componentDidMount',
1399 + ]);
1400 + });
1401 +
1402 });
src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
+76
@@ -18,11 +18,13 @@ describe('ReactMultiChild', () => {
18
19 var React;
20 var ReactDOM;
21 + var ReactDOMFeatureFlags;
22
23 beforeEach(() => {
24 jest.resetModuleRegistry();
25 React = require('React');
26 ReactDOM = require('ReactDOM');
27 + ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
28 });
29
30 describe('reconciliation', () => {
@@ -254,4 +256,78 @@ describe('ReactMultiChild', () => {
256 ReactDOM.render(<Letters letters="EHCjpdTUuiybDvhRJwZt" />, container);
257 expect(container.textContent).toBe('EHCjpdTUuiybDvhRJwZt');
258 });
259 +
260 + it('prepares new children before unmounting old', () => {
261 + var log = [];
262 +
263 + class Spy extends React.Component {
264 + componentWillMount() {
265 + log.push(this.props.name + ' componentWillMount');
266 + }
267 + render() {
268 + log.push(this.props.name + ' render');
269 + return <div />;
270 + }
271 + componentDidMount() {
272 + log.push(this.props.name + ' componentDidMount');
273 + }
274 + componentWillUnmount() {
275 + log.push(this.props.name + ' componentWillUnmount');
276 + }
277 + }
278 +
279 + // These are reference-unequal so they will be swapped even if they have
280 + // matching keys
281 + var SpyA = (props) => <Spy {...props} />;
282 + var SpyB = (props) => <Spy {...props} />;
283 +
284 + var container = document.createElement('div');
285 + ReactDOM.render(
286 + <div>
287 + <SpyA key="one" name="oneA" />
288 + <SpyA key="two" name="twoA" />
289 + </div>,
290 + container
291 + );
292 + ReactDOM.render(
293 + <div>
294 + <SpyB key="one" name="oneB" />
295 + <SpyB key="two" name="twoB" />
296 + </div>,
297 + container
298 + );
299 +
300 + expect(log).toEqual([
301 + 'oneA componentWillMount',
302 + 'oneA render',
303 + 'twoA componentWillMount',
304 + 'twoA render',
305 + 'oneA componentDidMount',
306 + 'twoA componentDidMount',
307 +
308 + ...(
309 + ReactDOMFeatureFlags.useFiber ?
310 + [
311 + 'oneB componentWillMount',
312 + 'oneB render',
313 + 'twoB componentWillMount',
314 + 'twoB render',
315 + 'oneA componentWillUnmount',
316 + 'twoA componentWillUnmount',
317 + ] :
318 + [
319 + 'oneB componentWillMount',
320 + 'oneB render',
321 + 'oneA componentWillUnmount',
322 + 'twoB componentWillMount',
323 + 'twoB render',
324 + 'twoA componentWillUnmount',
325 + ]
326 + ),
327 +
328 + 'oneB componentDidMount',
329 + 'twoB componentDidMount',
330 + ]);
331 + });
332 +
333 });
src/renderers/shared/stack/reconciler/ReactChildReconciler.js
+17 -2
@@ -11,10 +11,11 @@
11
12 'use strict';
13
14 +var KeyEscapeUtils = require('KeyEscapeUtils');
15 +var ReactFeatureFlags = require('ReactFeatureFlags');
16 var ReactReconciler = require('ReactReconciler');
17
18 var instantiateReactComponent = require('instantiateReactComponent');
17 -var KeyEscapeUtils = require('KeyEscapeUtils');
19 var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
20 var traverseAllChildren = require('traverseAllChildren');
21 var warning = require('warning');
@@ -144,7 +145,10 @@ var ReactChildReconciler = {
145 );
146 nextChildren[name] = prevChild;
147 } else {
147 - if (prevChild) {
148 + if (
149 + !ReactFeatureFlags.prepareNewChildrenBeforeUnmountInStack &&
150 + prevChild
151 + ) {
152 removedNodes[name] = ReactReconciler.getHostNode(prevChild);
153 ReactReconciler.unmountComponent(
154 prevChild,
@@ -166,6 +170,17 @@ var ReactChildReconciler = {
170 selfDebugID
171 );
172 mountImages.push(nextChildMountImage);
173 + if (
174 + ReactFeatureFlags.prepareNewChildrenBeforeUnmountInStack &&
175 + prevChild
176 + ) {
177 + removedNodes[name] = ReactReconciler.getHostNode(prevChild);
178 + ReactReconciler.unmountComponent(
179 + prevChild,
180 + false, /* safely */
181 + false /* skipLifecycle */
182 + );
183 + }
184 }
185 }
186 // Unmount children that are no longer present.
src/renderers/shared/stack/reconciler/ReactCompositeComponent.js
+17 -5
@@ -15,6 +15,7 @@ var React = require('React');
15 var ReactComponentEnvironment = require('ReactComponentEnvironment');
16 var ReactCurrentOwner = require('ReactCurrentOwner');
17 var ReactErrorUtils = require('ReactErrorUtils');
18 +var ReactFeatureFlags = require('ReactFeatureFlags');
19 var ReactInstanceMap = require('ReactInstanceMap');
20 var ReactInstrumentation = require('ReactInstrumentation');
21 var ReactNodeTypes = require('ReactNodeTypes');
@@ -1109,11 +1110,14 @@ var ReactCompositeComponent = {
1110 );
1111 } else {
1112 var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance);
1112 - ReactReconciler.unmountComponent(
1113 - prevComponentInstance,
1114 - safely,
1115 - false /* skipLifecycle */
1116 - );
1113 +
1114 + if (!ReactFeatureFlags.prepareNewChildrenBeforeUnmountInStack) {
1115 + ReactReconciler.unmountComponent(
1116 + prevComponentInstance,
1117 + safely,
1118 + false /* skipLifecycle */
1119 + );
1120 + }
1121
1122 var nodeType = ReactNodeTypes.getType(nextRenderedElement);
1123 this._renderedNodeType = nodeType;
@@ -1132,6 +1136,14 @@ var ReactCompositeComponent = {
1136 debugID
1137 );
1138
1139 + if (ReactFeatureFlags.prepareNewChildrenBeforeUnmountInStack) {
1140 + ReactReconciler.unmountComponent(
1141 + prevComponentInstance,
1142 + safely,
1143 + false /* skipLifecycle */
1144 + );
1145 + }
1146 +
1147 if (__DEV__) {
1148 if (debugID !== 0) {
1149 var childDebugIDs = child._debugID !== 0 ? [child._debugID] : [];
src/renderers/shared/utils/ReactFeatureFlags.js
+1
@@ -17,6 +17,7 @@ var ReactFeatureFlags = {
17 // render (both initial renders and updates). Useful when looking at prod-mode
18 // timeline profiles in Chrome, for example.
19 logTopLevelRenders: false,
20 + prepareNewChildrenBeforeUnmountInStack: true,
21 };
22
23 module.exports = ReactFeatureFlags;
src/renderers/testing/__tests__/ReactTestRenderer-test.js
+1 -1
@@ -200,8 +200,8 @@ describe('ReactTestRenderer', () => {
200 expect(log).toEqual([
201 'render Foo',
202 'mount Foo',
203 - 'unmount Foo',
203 'render Bar',
204 + 'unmount Foo',
205 'mount Bar',
206 'unmount Bar',
207 ]);