Fetch node to unmount separately from unmounting
My last strategy of getting each node recursively while unmounting was a pain to make work properly with ReactMount's confusing cache. Now, we get the node before unmounting anything in the subtree (and we don't try to find the nodes of descendants). This is a temporary solution and can go away when we get rid of the giant ReactMount node hash map. Fixes #5151.
Ben Alpert committed
Oct 13, 2015 at 12:07 UTC
622db4ee4fb61451b32fceb13d68a9589a2faecd
8 files changed
+79
-19
src/renderers/dom/client/__tests__/ReactMount-test.js
+30
@@ -245,4 +245,34 @@ describe('ReactMount', function() {
245
'render the new components instead of calling ReactDOM.render.'
246
);
247
});
248
+
249
+ it('should not crash in node cache when unmounting', function() {
250
+ var Component = React.createClass({
251
+ render: function() {
252
+ // Add refs to some nodes so that they get traversed and cached
253
+ return (
254
+ <div ref="a">
255
+ <div ref="b">b</div>
256
+ {this.props.showC && <div>c</div>}
257
+ </div>
258
+ );
259
+ },
260
+ });
261
+
262
+ var container = document.createElement('container');
263
+
264
+ ReactDOM.render(<div><Component showC={false} /></div>, container);
265
+
266
+ // Right now, A and B are in the cache. When we add C, it won't get added to
267
+ // the cache (assuming markup-string mode).
268
+ ReactDOM.render(<div><Component showC={true} /></div>, container);
269
+
270
+ // Remove A, B, and C. Unmounting C shouldn't cause B to get recached.
271
+ ReactDOM.render(<div></div>, container);
272
+
273
+ // Add them back -- this shouldn't cause a cached node collision.
274
+ ReactDOM.render(<div><Component showC={true} /></div>, container);
275
+
276
+ ReactDOM.unmountComponentAtNode(container);
277
+ });
278
});
src/renderers/dom/shared/ReactDOMComponent.js
+6
-5
@@ -1068,6 +1068,10 @@ ReactDOMComponent.Mixin = {
1068
}
1069
},
1070
1071
+ getNativeNode: function() {
1072
+ return getNode(this);
1073
+ },
1074
+
1075
/**
1076
* Destroys all event registrations for this instance. Does not remove from
1077
* the DOM. That must be done by the parent.
@@ -1112,19 +1116,16 @@ ReactDOMComponent.Mixin = {
1116
break;
1117
}
1118
1115
- var nativeNode = getNode(this);
1116
- this._nativeNode = null;
1119
if (this._nodeHasLegacyProperties) {
1118
- nativeNode._reactInternalComponent = null;
1120
+ this._nativeNode._reactInternalComponent = null;
1121
}
1122
+ this._nativeNode = null;
1123
1124
this.unmountChildren();
1125
ReactBrowserEventEmitter.deleteAllListeners(this._rootNodeID);
1126
ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID);
1127
this._rootNodeID = null;
1128
this._wrapperState = null;
1126
-
1127
- return nativeNode;
1129
},
1130
1131
getPublicInstance: function() {
src/renderers/dom/shared/ReactDOMTextComponent.js
+13
-7
@@ -23,6 +23,14 @@ var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
23
var setTextContent = require('setTextContent');
24
var validateDOMNesting = require('validateDOMNesting');
25
26
+function getNode(inst) {
27
+ if (inst._nativeNode) {
28
+ return inst._nativeNode;
29
+ } else {
30
+ return inst._nativeNode = ReactMount.getNode(inst._rootNodeID);
31
+ }
32
+}
33
+
34
/**
35
* Text nodes violate a couple assumptions that React makes about components:
36
*
@@ -133,20 +141,18 @@ assign(ReactDOMTextComponent.prototype, {
141
// and/or updateComponent to do the actual update for consistency with
142
// other component types?
143
this._stringText = nextStringText;
136
- var node = this._nativeNode;
137
- if (!node) {
138
- node = this._nativeNode = ReactMount.getNode(this._rootNodeID);
139
- }
140
- DOMChildrenOperations.updateTextContent(node, nextStringText);
144
+ DOMChildrenOperations.updateTextContent(getNode(this), nextStringText);
145
}
146
}
147
},
148
149
+ getNativeNode: function() {
150
+ return getNode(this);
151
+ },
152
+
153
unmountComponent: function() {
146
- var node = this._nativeNode || ReactMount.getNode(this._rootNodeID);
154
this._nativeNode = null;
155
ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID);
149
- return node;
156
},
157
158
});
src/renderers/shared/reconciler/ReactCompositeComponent.js
+13
-5
@@ -309,6 +309,10 @@ var ReactCompositeComponentMixin = {
309
return markup;
310
},
311
312
+ getNativeNode: function() {
313
+ return ReactReconciler.getNativeNode(this._renderedComponent);
314
+ },
315
+
316
/**
317
* Releases any resources allocated by `mountComponent`.
318
*
@@ -322,8 +326,7 @@ var ReactCompositeComponentMixin = {
326
inst.componentWillUnmount();
327
}
328
325
- var unmountedNativeNode =
326
- ReactReconciler.unmountComponent(this._renderedComponent);
329
+ ReactReconciler.unmountComponent(this._renderedComponent);
330
this._renderedComponent = null;
331
this._instance = null;
332
@@ -352,7 +355,6 @@ var ReactCompositeComponentMixin = {
355
// TODO: inst.props = null;
356
// TODO: inst.state = null;
357
// TODO: inst.context = null;
355
- return unmountedNativeNode;
358
},
359
360
/**
@@ -741,8 +743,14 @@ var ReactCompositeComponentMixin = {
743
this._processChildContext(context)
744
);
745
} else {
744
- var oldNativeNode =
745
- ReactReconciler.unmountComponent(prevComponentInstance);
746
+ // TODO: This is currently necessary due to the unfortunate caching
747
+ // that ReactMount does which makes it exceedingly difficult to unmount
748
+ // a set of siblings without accidentally repopulating the node cache (see
749
+ // #5151). Once ReactMount no longer stores the nodes by ID, this method
750
+ // can go away.
751
+ var oldNativeNode = ReactReconciler.getNativeNode(prevComponentInstance);
752
+
753
+ ReactReconciler.unmountComponent(prevComponentInstance);
754
755
this._renderedComponent = this._instantiateReactComponent(
756
nextRenderedElement
src/renderers/shared/reconciler/ReactEmptyComponent.js
+4
-2
@@ -53,12 +53,14 @@ assign(ReactEmptyComponent.prototype, {
53
},
54
receiveComponent: function() {
55
},
56
+ getNativeNode: function() {
57
+ return ReactReconciler.getNativeNode(this._renderedComponent);
58
+ },
59
unmountComponent: function(rootID, transaction, context) {
57
- var nativeNode = ReactReconciler.unmountComponent(this._renderedComponent);
60
+ ReactReconciler.unmountComponent(this._renderedComponent);
61
ReactEmptyComponentRegistry.deregisterNullComponentID(this._rootNodeID);
62
this._rootNodeID = null;
63
this._renderedComponent = null;
61
- return nativeNode;
64
},
65
});
66
src/renderers/shared/reconciler/ReactReconciler.js
+8
@@ -57,6 +57,14 @@ var ReactReconciler = {
57
return markup;
58
},
59
60
+ /**
61
+ * Returns a value that can be passed to
62
+ * ReactComponentEnvironment.replaceNodeWithMarkup.
63
+ */
64
+ getNativeNode: function(internalInstance) {
65
+ return internalInstance.getNativeNode();
66
+ },
67
+
68
/**
69
* Releases any resources allocated by `mountComponent`.
70
*
src/renderers/shared/reconciler/instantiateReactComponent.js
+1
@@ -105,6 +105,7 @@ function instantiateReactComponent(node) {
105
typeof instance.construct === 'function' &&
106
typeof instance.mountComponent === 'function' &&
107
typeof instance.receiveComponent === 'function' &&
108
+ typeof instance.getNativeNode === 'function' &&
109
typeof instance.unmountComponent === 'function',
110
'Only React Components can be mounted.'
111
);
src/test/ReactTestUtils.js
+4
@@ -386,6 +386,10 @@ NoopInternalComponent.prototype = {
386
this._currentElement = element;
387
},
388
389
+ getNativeNode: function() {
390
+ return undefined;
391
+ },
392
+
393
unmountComponent: function() {
394
},
395