Fix Reconciling Components to Content
This fixes a reconciliation bug introduced by adffa9b0f4128ca4832ddd03fdeca2c9fe434689. The new unit test case exhibits the bug. When a component that has rendered child components is updated to render inline text, we usually: # Unmount and remove all child components. # Set the new inline text content. However, with batched child operations, we do not **remove all child components** until later. The current implementation will set the inline text content and blow away those nodes, causing a fatal when `ReactMultiChild` later tries to find and remove those nodes. This fixes the bug by ensuring that text content changes are also enqueued.
Tim Yung committed
Jul 29, 2013 at 16:11 UTC
5ef3c1b09b486d711b556655c28b93e207a90399
5 files changed
+103
-11
src/core/ReactMultiChild.js
+63
-1
@@ -78,6 +78,7 @@ function enqueueMarkup(parentID, markup, toIndex) {
78
type: ReactMultiChildUpdateTypes.INSERT_MARKUP,
79
markupIndex: markupQueue.push(markup) - 1,
80
fromIndex: null,
81
+ textContent: null,
82
toIndex: toIndex
83
});
84
}
@@ -97,6 +98,7 @@ function enqueueMove(parentID, fromIndex, toIndex) {
98
parentNode: null,
99
type: ReactMultiChildUpdateTypes.MOVE_EXISTING,
100
markupIndex: null,
101
+ textContent: null,
102
fromIndex: fromIndex,
103
toIndex: toIndex
104
});
@@ -116,11 +118,32 @@ function enqueueRemove(parentID, fromIndex) {
118
parentNode: null,
119
type: ReactMultiChildUpdateTypes.REMOVE_NODE,
120
markupIndex: null,
121
+ textContent: null,
122
fromIndex: fromIndex,
123
toIndex: null
124
});
125
}
126
127
+/**
128
+ * Enqueues setting the text content.
129
+ *
130
+ * @param {string} parentID ID of the parent component.
131
+ * @param {string} textContent Text content to set.
132
+ * @private
133
+ */
134
+function enqueueTextContent(parentID, textContent) {
135
+ // NOTE: Null values reduce hidden classes.
136
+ updateQueue.push({
137
+ parentID: parentID,
138
+ parentNode: null,
139
+ type: ReactMultiChildUpdateTypes.TEXT_CONTENT,
140
+ markupIndex: null,
141
+ textContent: textContent,
142
+ fromIndex: null,
143
+ toIndex: null
144
+ });
145
+}
146
+
147
/**
148
* Processes any enqueued updates.
149
*
@@ -192,6 +215,34 @@ var ReactMultiChild = {
215
return mountImages;
216
},
217
218
+ /**
219
+ * Replaces any rendered children with a text content string.
220
+ *
221
+ * @param {string} nextContent String of content.
222
+ * @internal
223
+ */
224
+ updateTextContent: function(nextContent) {
225
+ updateDepth++;
226
+ try {
227
+ var prevChildren = this._renderedChildren;
228
+ // Remove any rendered children.
229
+ for (var name in prevChildren) {
230
+ if (prevChildren.hasOwnProperty(name) &&
231
+ prevChildren[name]) {
232
+ this._unmountChildByName(prevChildren[name], name);
233
+ }
234
+ }
235
+ // Set new text content.
236
+ this.setTextContent(nextContent);
237
+ } catch (error) {
238
+ updateDepth--;
239
+ updateDepth || clearQueue();
240
+ throw error;
241
+ }
242
+ updateDepth--;
243
+ updateDepth || processQueue();
244
+ },
245
+
246
/**
247
* Updates the rendered children with new children.
248
*
@@ -244,8 +295,9 @@ var ReactMultiChild = {
295
prevChild._mountIndex = nextIndex;
296
} else {
297
if (prevChild) {
247
- this._unmountChildByName(prevChild, name);
298
+ // Update `lastIndex` before `_mountIndex` gets unset by unmounting.
299
lastIndex = Math.max(prevChild._mountIndex, lastIndex);
300
+ this._unmountChildByName(prevChild, name);
301
}
302
if (nextChild) {
303
this._mountChildByNameAtIndex(
@@ -321,6 +373,16 @@ var ReactMultiChild = {
373
enqueueRemove(this._rootNodeID, child._mountIndex);
374
},
375
376
+ /**
377
+ * Sets this text content string.
378
+ *
379
+ * @param {string} textContent Text content to set.
380
+ * @protected
381
+ */
382
+ setTextContent: function(textContent) {
383
+ enqueueTextContent(this._rootNodeID, textContent);
384
+ },
385
+
386
/**
387
* Mounts a child with the supplied name.
388
*
src/core/ReactMultiChildUpdateTypes.js
+2
-1
@@ -29,7 +29,8 @@ var keyMirror = require('keyMirror');
29
var ReactMultiChildUpdateTypes = keyMirror({
30
INSERT_MARKUP: null,
31
MOVE_EXISTING: null,
32
- REMOVE_NODE: null
32
+ REMOVE_NODE: null,
33
+ TEXT_CONTENT: null
34
});
35
36
module.exports = ReactMultiChildUpdateTypes;
src/core/ReactNativeComponent.js
+2
-8
@@ -324,18 +324,12 @@ ReactNativeComponent.Mixin = {
324
this.updateChildren(null, transaction);
325
}
326
if (lastUsedContent !== contentToUse) {
327
- ReactComponent.DOMIDOperations.updateTextContentByID(
328
- this._rootNodeID,
329
- '' + contentToUse
330
- );
327
+ this.updateTextContent('' + contentToUse);
328
}
329
} else {
330
var contentRemoved = lastUsedContent != null && contentToUse == null;
331
if (contentRemoved) {
335
- ReactComponent.DOMIDOperations.updateTextContentByID(
336
- this._rootNodeID,
337
- ''
338
- );
332
+ this.updateTextContent('');
333
}
334
this.updateChildren(flattenChildren(nextProps.children), transaction);
335
}
src/core/__tests__/ReactMultiChildText-test.js
+21
@@ -287,4 +287,25 @@ describe('ReactMultiChildText', function() {
287
);
288
}).toThrow();
289
});
290
+
291
+ it('should render between nested components and inline children', function() {
292
+ var container = document.createElement('div');
293
+ React.renderComponent(<div><h1><span /><span /></h1></div>, container);
294
+
295
+ expect(function() {
296
+ React.renderComponent(<div><h1>A</h1></div>, container);
297
+ }).not.toThrow();
298
+
299
+ React.renderComponent(<div><h1><span /><span /></h1></div>, container);
300
+
301
+ expect(function() {
302
+ React.renderComponent(<div><h1>{['A']}</h1></div>, container);
303
+ }).not.toThrow();
304
+
305
+ React.renderComponent(<div><h1><span /><span /></h1></div>, container);
306
+
307
+ expect(function() {
308
+ React.renderComponent(<div><h1>{['A', 'B']}</h1></div>, container);
309
+ }).not.toThrow();
310
+ });
311
});
src/dom/DOMChildrenOperations.js
+15
-1
@@ -22,6 +22,16 @@
22
var Danger = require('Danger');
23
var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes');
24
25
+var getTextContentAccessor = require('getTextContentAccessor');
26
+
27
+/**
28
+ * The DOM property to use when setting text content.
29
+ *
30
+ * @type {string}
31
+ * @private
32
+ */
33
+var textContentAccessor = getTextContentAccessor() || 'NA';
34
+
35
/**
36
* Inserts `childNode` as a child of `parentNode` at the `index`.
37
*
@@ -70,7 +80,8 @@ var DOMChildrenOperations = {
80
var updatedChildren = null;
81
82
for (var i = 0; update = updates[i]; i++) {
73
- if (update.type !== ReactMultiChildUpdateTypes.INSERT_MARKUP) {
83
+ if (update.type === ReactMultiChildUpdateTypes.MOVE_EXISTING ||
84
+ update.type === ReactMultiChildUpdateTypes.REMOVE_NODE) {
85
var updatedIndex = update.fromIndex;
86
var updatedChild = update.parentNode.childNodes[updatedIndex];
87
var parentID = update.parentID;
@@ -109,6 +120,9 @@ var DOMChildrenOperations = {
120
update.toIndex
121
);
122
break;
123
+ case ReactMultiChildUpdateTypes.TEXT_CONTENT:
124
+ update.parentNode[textContentAccessor] = update.textContent;
125
+ break;
126
case ReactMultiChildUpdateTypes.REMOVE_NODE:
127
// Already removed by the for-loop above.
128
break;