Include Ownership in the Ship of Theseus
When we determine whether a React component should be updated (as opposed to destroyed or replaced), we currently only look at whether they share the same constructor. This adds a check for whether they share the same owner component. I've also consolidated this logic (I cannot believe this was not already done).
Tim Yung committed
Nov 5, 2013 at 16:55 UTC
e78d580c06ddb5cdcc1db1fd94007c2b7a55d871
5 files changed
+219
-69
src/core/ReactCompositeComponent.js
+7
-6
@@ -30,6 +30,7 @@ var keyMirror = require('keyMirror');
30
var merge = require('merge');
31
var mixInto = require('mixInto');
32
var objMap = require('objMap');
33
+var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
34
35
/**
36
* Policies that describe methods in `ReactCompositeComponentInterface`.
@@ -789,15 +790,15 @@ var ReactCompositeComponentMixin = {
790
'updateComponent',
791
function(transaction, prevProps, prevState) {
792
ReactComponent.Mixin.updateComponent.call(this, transaction, prevProps);
792
- var currentComponent = this._renderedComponent;
793
+ var prevComponent = this._renderedComponent;
794
var nextComponent = this._renderValidatedComponent();
794
- if (currentComponent.constructor === nextComponent.constructor) {
795
- currentComponent.receiveProps(nextComponent.props, transaction);
795
+ if (shouldUpdateReactComponent(prevComponent, nextComponent)) {
796
+ prevComponent.receiveProps(nextComponent.props, transaction);
797
} else {
798
// These two IDs are actually the same! But nothing should rely on that.
799
var thisID = this._rootNodeID;
799
- var currentComponentID = currentComponent._rootNodeID;
800
- currentComponent.unmountComponent();
800
+ var prevComponentID = prevComponent._rootNodeID;
801
+ prevComponent.unmountComponent();
802
this._renderedComponent = nextComponent;
803
var nextMarkup = nextComponent.mountComponent(
804
thisID,
@@ -805,7 +806,7 @@ var ReactCompositeComponentMixin = {
806
this._mountDepth + 1
807
);
808
ReactComponent.DOMIDOperations.dangerouslyReplaceNodeWithMarkupByID(
808
- currentComponentID,
809
+ prevComponentID,
810
nextMarkup
811
);
812
}
src/core/ReactMount.js
+6
-5
@@ -25,6 +25,7 @@ var $ = require('$');
25
var getReactRootElementInContainer = require('getReactRootElementInContainer');
26
var invariant = require('invariant');
27
var nodeContains = require('nodeContains');
28
+var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
29
30
var SEPARATOR = ReactInstanceHandles.SEPARATOR;
31
@@ -304,12 +305,12 @@ var ReactMount = {
305
* @return {ReactComponent} Component instance rendered in `container`.
306
*/
307
renderComponent: function(nextComponent, container, callback) {
307
- var registeredComponent = instancesByReactRootID[getReactRootID(container)];
308
+ var prevComponent = instancesByReactRootID[getReactRootID(container)];
309
309
- if (registeredComponent) {
310
- if (registeredComponent.constructor === nextComponent.constructor) {
310
+ if (prevComponent) {
311
+ if (shouldUpdateReactComponent(prevComponent, nextComponent)) {
312
return ReactMount._updateRootComponent(
312
- registeredComponent,
313
+ prevComponent,
314
nextComponent,
315
container,
316
callback
@@ -323,7 +324,7 @@ var ReactMount = {
324
var containerHasReactMarkup =
325
reactRootElement && ReactMount.isRenderedByReact(reactRootElement);
326
326
- var shouldReuseMarkup = containerHasReactMarkup && !registeredComponent;
327
+ var shouldReuseMarkup = containerHasReactMarkup && !prevComponent;
328
329
var component = ReactMount._renderNewRootComponent(
330
nextComponent,
src/core/ReactMultiChild.js
+2
-14
@@ -23,19 +23,7 @@ var ReactComponent = require('ReactComponent');
23
var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes');
24
25
var flattenChildren = require('flattenChildren');
26
-
27
-/**
28
- * Given a `curChild` and `newChild`, determines if `curChild` should be
29
- * updated as opposed to being destroyed or replaced.
30
- *
31
- * @param {?ReactComponent} curChild
32
- * @param {?ReactComponent} newChild
33
- * @return {boolean} True if `curChild` should be updated with `newChild`.
34
- * @protected
35
- */
36
-function shouldUpdateChild(curChild, newChild) {
37
- return curChild && newChild && curChild.constructor === newChild.constructor;
38
-}
26
+var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
27
28
/**
29
* Updating children of a component may trigger recursive updates. The depth is
@@ -294,7 +282,7 @@ var ReactMultiChild = {
282
}
283
var prevChild = prevChildren && prevChildren[name];
284
var nextChild = nextChildren[name];
297
- if (shouldUpdateChild(prevChild, nextChild)) {
285
+ if (shouldUpdateReactComponent(prevChild, nextChild)) {
286
this.moveChild(prevChild, nextIndex, lastIndex);
287
lastIndex = Math.max(prevChild._mountIndex, lastIndex);
288
prevChild.receiveProps(nextChild.props, transaction);
src/core/__tests__/ReactMultiChild-test.js
+151
-44
@@ -17,61 +17,168 @@
17
* @emails react-core
18
*/
19
20
+var mocks = require('mocks');
21
+
22
describe('ReactMultiChild', function() {
23
var React;
22
- var setInnerHTML;
23
-
24
- // Only run this test suite if `Element.prototype.innerHTML` can be spied on.
25
- var innerHTMLDescriptor = Object.getOwnPropertyDescriptor(
26
- Element.prototype,
27
- 'innerHTML'
28
- );
29
- if (!innerHTMLDescriptor) {
30
- return;
31
- }
24
25
beforeEach(function() {
26
require('mock-modules').dumpCache();
27
React = require('React');
28
+ });
29
+
30
+ describe('reconciliation', function() {
31
+ it('should update children when possible', function() {
32
+ var container = document.createElement('div');
33
+
34
+ var mockMount = mocks.getMockFunction();
35
+ var mockUpdate = mocks.getMockFunction();
36
+ var mockUnmount = mocks.getMockFunction();
37
+
38
+ var MockComponent = React.createClass({
39
+ componentDidMount: mockMount,
40
+ componentDidUpdate: mockUpdate,
41
+ componentWillUnmount: mockUnmount,
42
+ render: function() {
43
+ return <span />;
44
+ }
45
+ });
46
+
47
+ expect(mockMount.mock.calls.length).toBe(0);
48
+ expect(mockUpdate.mock.calls.length).toBe(0);
49
+ expect(mockUnmount.mock.calls.length).toBe(0);
50
+
51
+ React.renderComponent(<div><MockComponent /></div>, container);
52
+
53
+ expect(mockMount.mock.calls.length).toBe(1);
54
+ expect(mockUpdate.mock.calls.length).toBe(0);
55
+ expect(mockUnmount.mock.calls.length).toBe(0);
56
+
57
+ React.renderComponent(<div><MockComponent /></div>, container);
58
+
59
+ expect(mockMount.mock.calls.length).toBe(1);
60
+ expect(mockUpdate.mock.calls.length).toBe(1);
61
+ expect(mockUnmount.mock.calls.length).toBe(0);
62
+ });
63
+
64
+ it('should replace children with different constructors', function() {
65
+ var container = document.createElement('div');
66
+
67
+ var mockMount = mocks.getMockFunction();
68
+ var mockUnmount = mocks.getMockFunction();
69
+
70
+ var MockComponent = React.createClass({
71
+ componentDidMount: mockMount,
72
+ componentWillUnmount: mockUnmount,
73
+ render: function() {
74
+ return <span />;
75
+ }
76
+ });
77
+
78
+ expect(mockMount.mock.calls.length).toBe(0);
79
+ expect(mockUnmount.mock.calls.length).toBe(0);
80
+
81
+ React.renderComponent(<div><MockComponent /></div>, container);
82
+
83
+ expect(mockMount.mock.calls.length).toBe(1);
84
+ expect(mockUnmount.mock.calls.length).toBe(0);
85
+
86
+ React.renderComponent(<div><span /></div>, container);
87
37
- Object.defineProperty(Element.prototype, 'innerHTML', {
38
- set: setInnerHTML = jasmine.createSpy().andCallFake(
39
- innerHTMLDescriptor.set
40
- )
88
+ expect(mockMount.mock.calls.length).toBe(1);
89
+ expect(mockUnmount.mock.calls.length).toBe(1);
90
+ });
91
+
92
+ it('should replace children with different owners', function() {
93
+ var container = document.createElement('div');
94
+
95
+ var mockMount = mocks.getMockFunction();
96
+ var mockUnmount = mocks.getMockFunction();
97
+
98
+ var MockComponent = React.createClass({
99
+ componentDidMount: mockMount,
100
+ componentWillUnmount: mockUnmount,
101
+ render: function() {
102
+ return <span />;
103
+ }
104
+ });
105
+
106
+ var WrapperComponent = React.createClass({
107
+ render: function() {
108
+ return this.props.children || <MockComponent />;
109
+ }
110
+ });
111
+
112
+ expect(mockMount.mock.calls.length).toBe(0);
113
+ expect(mockUnmount.mock.calls.length).toBe(0);
114
+
115
+ React.renderComponent(<WrapperComponent />, container);
116
+
117
+ expect(mockMount.mock.calls.length).toBe(1);
118
+ expect(mockUnmount.mock.calls.length).toBe(0);
119
+
120
+ React.renderComponent(
121
+ <WrapperComponent><MockComponent /></WrapperComponent>,
122
+ container
123
+ );
124
+
125
+ expect(mockMount.mock.calls.length).toBe(2);
126
+ expect(mockUnmount.mock.calls.length).toBe(1);
127
});
128
});
129
44
- it('should only set `innerHTML` once on update', function() {
45
- var container = document.createElement('div');
130
+ describe('innerHTML', function() {
131
+ var setInnerHTML;
132
47
- React.renderComponent(
48
- <div>
49
- <p><span /></p>
50
- <p><span /></p>
51
- <p><span /></p>
52
- </div>,
53
- container
54
- );
55
- // Warm the cache used by `getMarkupWrap`.
56
- React.renderComponent(
57
- <div>
58
- <p><span /><span /></p>
59
- <p><span /><span /></p>
60
- <p><span /><span /></p>
61
- </div>,
62
- container
63
- );
64
- expect(setInnerHTML).toHaveBeenCalled();
65
- var callCountOnMount = setInnerHTML.callCount;
66
-
67
- React.renderComponent(
68
- <div>
69
- <p><span /><span /><span /></p>
70
- <p><span /><span /><span /></p>
71
- <p><span /><span /><span /></p>
72
- </div>,
73
- container
133
+ // Only run this suite if `Element.prototype.innerHTML` can be spied on.
134
+ var innerHTMLDescriptor = Object.getOwnPropertyDescriptor(
135
+ Element.prototype,
136
+ 'innerHTML'
137
);
75
- expect(setInnerHTML.callCount).toBe(callCountOnMount + 1);
138
+ if (!innerHTMLDescriptor) {
139
+ return;
140
+ }
141
+
142
+ beforeEach(function() {
143
+ Object.defineProperty(Element.prototype, 'innerHTML', {
144
+ set: setInnerHTML = jasmine.createSpy().andCallFake(
145
+ innerHTMLDescriptor.set
146
+ )
147
+ });
148
+ });
149
+
150
+ it('should only set `innerHTML` once on update', function() {
151
+ var container = document.createElement('div');
152
+
153
+ React.renderComponent(
154
+ <div>
155
+ <p><span /></p>
156
+ <p><span /></p>
157
+ <p><span /></p>
158
+ </div>,
159
+ container
160
+ );
161
+ // Warm the cache used by `getMarkupWrap`.
162
+ React.renderComponent(
163
+ <div>
164
+ <p><span /><span /></p>
165
+ <p><span /><span /></p>
166
+ <p><span /><span /></p>
167
+ </div>,
168
+ container
169
+ );
170
+ expect(setInnerHTML).toHaveBeenCalled();
171
+ var callCountOnMount = setInnerHTML.callCount;
172
+
173
+ React.renderComponent(
174
+ <div>
175
+ <p><span /><span /><span /></p>
176
+ <p><span /><span /><span /></p>
177
+ <p><span /><span /><span /></p>
178
+ </div>,
179
+ container
180
+ );
181
+ expect(setInnerHTML.callCount).toBe(callCountOnMount + 1);
182
+ });
183
});
184
});
src/core/shouldUpdateReactComponent.js
new
+53
@@ -0,0 +1,53 @@
1
+/**
2
+ * Copyright 2013 Facebook, Inc.
3
+ *
4
+ * Licensed under the Apache License, Version 2.0 (the "License");
5
+ * you may not use this file except in compliance with the License.
6
+ * You may obtain a copy of the License at
7
+ *
8
+ * http://www.apache.org/licenses/LICENSE-2.0
9
+ *
10
+ * Unless required by applicable law or agreed to in writing, software
11
+ * distributed under the License is distributed on an "AS IS" BASIS,
12
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13
+ * See the License for the specific language governing permissions and
14
+ * limitations under the License.
15
+ *
16
+ * @providesModule shouldUpdateReactComponent
17
+ * @typechecks static-only
18
+ */
19
+
20
+/**
21
+ * Given a `prevComponent` and `nextComponent`, determines if `prevComponent`
22
+ * should be updated as opposed to being destroyed or replaced.
23
+ *
24
+ * @param {?object} prevComponent
25
+ * @param {?object} nextComponent
26
+ * @return {boolean} True if `prevComponent` should be updated.
27
+ * @protected
28
+ */
29
+function shouldUpdateReactComponent(prevComponent, nextComponent) {
30
+ // TODO: Remove warning after a release.
31
+ if (prevComponent && nextComponent &&
32
+ prevComponent.constructor === nextComponent.constructor) {
33
+ if (prevComponent.props.__owner__ === nextComponent.props.__owner__) {
34
+ return true;
35
+ } else {
36
+ if (__DEV__) {
37
+ if (prevComponent.state) {
38
+ console.warn(
39
+ 'A recent change to React has been found to impact your code. ' +
40
+ 'A mounted component will now be unmounted and replaced by a ' +
41
+ 'component (of the same class) if their owners are different. ' +
42
+ 'Previously, ownership was not considered when updating.',
43
+ prevComponent,
44
+ nextComponent
45
+ );
46
+ }
47
+ }
48
+ }
49
+ }
50
+ return false;
51
+}
52
+
53
+module.exports = shouldUpdateReactComponent;