Explicit and Implicit Keys Need Separate Namespaces
There are certain cases where you can end up with a collision with an implicit key (array index) if your explicit key prop is a number. They should use different namespaces. Therefore I wrap explicit keys in curlies and implicit array indices in brackets. I added a simple test case, but another case came up on the mailing list. Where undefined entries in an array actually results in an entry and therefore an implicit key.
Sebastian Markbage committed
Aug 23, 2013 at 13:56 UTC
61b38b9f053a5cf32ff868974357884c1bcb5d3a
6 files changed
+51
-38
src/core/ReactComponent.js
+2
-2
@@ -177,10 +177,10 @@ var ReactComponent = {
177
getKey: function(component, index) {
178
if (component && component.props && component.props.key != null) {
179
// Explicit key
180
- return '' + component.props.key;
180
+ return '{' + component.props.key + '}';
181
}
182
// Implicit key determined by the index in the set
183
- return '' + index;
183
+ return '[' + index + ']';
184
},
185
186
/**
src/core/__tests__/ReactIdentity-test.js
+23
-10
@@ -71,10 +71,10 @@ describe('ReactIdentity', function() {
71
React.renderComponent(instance, document.createElement('div'));
72
var node = instance.getDOMNode();
73
reactComponentExpect(instance).toBeDOMComponentWithChildCount(4);
74
- checkId(node.childNodes[0], '.r[0].[apple]');
75
- checkId(node.childNodes[1], '.r[0].[banana]');
76
- checkId(node.childNodes[2], '.r[0].[0]');
77
- checkId(node.childNodes[3], '.r[0].[123]');
74
+ checkId(node.childNodes[0], '.r[0].{apple}');
75
+ checkId(node.childNodes[1], '.r[0].{banana}');
76
+ checkId(node.childNodes[2], '.r[0].{0}');
77
+ checkId(node.childNodes[3], '.r[0].{123}');
78
});
79
80
it('should use instance identity', function() {
@@ -96,12 +96,12 @@ describe('ReactIdentity', function() {
96
var node = instance.getDOMNode();
97
reactComponentExpect(instance).toBeDOMComponentWithChildCount(3);
98
99
- checkId(node.childNodes[0], '.r[0].[wrap1]');
100
- checkId(node.childNodes[0].firstChild, '.r[0].[wrap1].[squirrel]');
101
- checkId(node.childNodes[1], '.r[0].[wrap2]');
102
- checkId(node.childNodes[1].firstChild, '.r[0].[wrap2].[bunny]');
99
+ checkId(node.childNodes[0], '.r[0].{wrap1}');
100
+ checkId(node.childNodes[0].firstChild, '.r[0].{wrap1}.{squirrel}');
101
+ checkId(node.childNodes[1], '.r[0].{wrap2}');
102
+ checkId(node.childNodes[1].firstChild, '.r[0].{wrap2}.{bunny}');
103
checkId(node.childNodes[2], '.r[0].[2]');
104
- checkId(node.childNodes[2].firstChild, '.r[0].[2].[chipmunk]');
104
+ checkId(node.childNodes[2].firstChild, '.r[0].[2].{chipmunk}');
105
});
106
107
function renderAComponentWithKeyIntoContainer(key, container) {
@@ -116,7 +116,7 @@ describe('ReactIdentity', function() {
116
expect(span1.getDOMNode()).not.toBe(null);
117
expect(span2.getDOMNode()).not.toBe(null);
118
119
- checkId(span1.getDOMNode(), '.r[0].[' + key + ']');
119
+ checkId(span1.getDOMNode(), '.r[0].{' + key + '}');
120
checkId(span2.getDOMNode(), '.r[0].[1]{' + key + '}');
121
}
122
@@ -291,4 +291,17 @@ describe('ReactIdentity', function() {
291
292
});
293
294
+ it('should not allow implicit and explicit keys to collide', function() {
295
+ var component =
296
+ <div>
297
+ <span />
298
+ <span key="0" />
299
+ </div>;
300
+
301
+ expect(function() {
302
+ React.renderComponent(component, document.createElement('div'));
303
+ }).not.toThrow();
304
+ });
305
+
306
+
307
});
src/utils/__tests__/ReactChildren-test.js
+9
-9
@@ -92,7 +92,7 @@ describe('ReactChildren', function() {
92
expect(mappedKeys.length).toBe(1);
93
expect(mappedChildren[mappedKeys[0]]).not.toBe(simpleKid);
94
expect(mappedChildren[mappedKeys[0]].props.children).toBe(simpleKid);
95
- expect(mappedKeys[0]).toBe('[simple]');
95
+ expect(mappedKeys[0]).toBe('{simple}');
96
});
97
98
it('should invoke callback with the right context', function() {
@@ -162,7 +162,7 @@ describe('ReactChildren', function() {
162
expect(mappedKeys.length).toBe(5);
163
// Keys default to indices.
164
expect(mappedKeys).toEqual(
165
- ['[keyZero]', '[1]', '[keyTwo]', '[3]', '[keyFour]']
165
+ ['{keyZero}', '[1]', '{keyTwo}', '[3]', '{keyFour}']
166
);
167
168
expect(callback).toHaveBeenCalledWith(zero, 0);
@@ -236,11 +236,11 @@ describe('ReactChildren', function() {
236
expect(mappedKeys.length).toBe(6);
237
// Keys default to indices.
238
expect(mappedKeys).toEqual([
239
- '[0]{firstHalfKey}[keyZero]',
239
+ '[0]{firstHalfKey}{keyZero}',
240
'[0]{firstHalfKey}[1]',
241
- '[0]{firstHalfKey}[keyTwo]',
241
+ '[0]{firstHalfKey}{keyTwo}',
242
'[0]{secondHalfKey}[0]',
243
- '[0]{secondHalfKey}[keyFour]',
243
+ '[0]{secondHalfKey}{keyFour}',
244
'[0]{keyFive}'
245
]);
246
@@ -283,13 +283,13 @@ describe('ReactChildren', function() {
283
</div>
284
);
285
286
- var expectedForcedKeys = ['[keyZero]', '[keyOne]'];
286
+ var expectedForcedKeys = ['{keyZero}', '{keyOne}'];
287
var mappedChildrenForcedKeys =
288
ReactChildren.map(forcedKeys.props.children, mapFn);
289
var mappedForcedKeys = Object.keys(mappedChildrenForcedKeys);
290
expect(mappedForcedKeys).toEqual(expectedForcedKeys);
291
292
- var expectedRemappedForcedKeys = ['{[keyZero]}', '{[keyOne]}'];
292
+ var expectedRemappedForcedKeys = ['{{keyZero}}', '{{keyOne}}'];
293
var remappedChildrenForcedKeys =
294
ReactChildren.map(mappedChildrenForcedKeys, mapFn);
295
expect(
@@ -298,7 +298,7 @@ describe('ReactChildren', function() {
298
299
});
300
301
- it('should throw if key provided is a dupe with array key', function() {
301
+ it('should not throw if key provided is a dupe with array key', function() {
302
var zero = <div />;
303
var one = <div key="0" />;
304
@@ -315,7 +315,7 @@ describe('ReactChildren', function() {
315
316
expect(function() {
317
ReactChildren.map(instance.props.children, mapFn);
318
- }).toThrow();
318
+ }).not.toThrow();
319
});
320
321
it('should throw if key provided is a dupe with explicit key', function() {
src/utils/__tests__/sliceChildren-test.js
+6
-6
@@ -68,9 +68,9 @@ describe('sliceChildren', function() {
68
];
69
var children = renderAndSlice(fullSet, 0);
70
expect(children).toEqual({
71
- '[A]': fullSet[0],
72
- '[B]': fullSet[1],
73
- '[C]': fullSet[2]
71
+ '{A}': fullSet[0],
72
+ '{B}': fullSet[1],
73
+ '{C}': fullSet[2]
74
});
75
});
76
@@ -82,8 +82,8 @@ describe('sliceChildren', function() {
82
];
83
var children = renderAndSlice(fullSet, 1);
84
expect(children).toEqual({
85
- '[B]': fullSet[1],
86
- '[C]': fullSet[2]
85
+ '{B}': fullSet[1],
86
+ '{C}': fullSet[2]
87
});
88
});
89
@@ -96,7 +96,7 @@ describe('sliceChildren', function() {
96
];
97
var children = renderAndSlice(fullSet, 1, 2);
98
expect(children).toEqual({
99
- '[B]': fullSet[1]
99
+ '{B}': fullSet[1]
100
});
101
});
102
src/utils/__tests__/traverseAllChildren-test.js
+9
-9
@@ -43,7 +43,7 @@ describe('traverseAllChildren', function() {
43
expect(traverseFn).toHaveBeenCalledWith(
44
traverseContext,
45
simpleKid,
46
- '[simple]',
46
+ '{simple}',
47
0
48
);
49
expect(traverseContext.length).toEqual(1);
@@ -114,21 +114,21 @@ describe('traverseAllChildren', function() {
114
expect(traverseFn).toHaveBeenCalledWith(
115
traverseContext,
116
zero,
117
- '[keyZero]',
117
+ '{keyZero}',
118
0
119
);
120
expect(traverseFn).toHaveBeenCalledWith(traverseContext, one, '[1]', 1);
121
expect(traverseFn).toHaveBeenCalledWith(
122
traverseContext,
123
two,
124
- '[keyTwo]',
124
+ '{keyTwo}',
125
2
126
);
127
expect(traverseFn).toHaveBeenCalledWith(traverseContext, three, '[3]', 3);
128
expect(traverseFn).toHaveBeenCalledWith(
129
traverseContext,
130
four,
131
- '[keyFour]',
131
+ '{keyFour}',
132
4
133
);
134
});
@@ -170,7 +170,7 @@ describe('traverseAllChildren', function() {
170
expect(traverseFn).toHaveBeenCalledWith(
171
traverseContext,
172
zero,
173
- '[0]{firstHalfKey}[keyZero]',
173
+ '[0]{firstHalfKey}{keyZero}',
174
0
175
);
176
@@ -180,7 +180,7 @@ describe('traverseAllChildren', function() {
180
expect(traverseFn).toHaveBeenCalledWith(
181
traverseContext,
182
two,
183
- '[0]{firstHalfKey}[keyTwo]',
183
+ '[0]{firstHalfKey}{keyTwo}',
184
2
185
);
186
@@ -190,7 +190,7 @@ describe('traverseAllChildren', function() {
190
expect(traverseFn).toHaveBeenCalledWith(
191
traverseContext,
192
four,
193
- '[0]{secondHalfKey}[keyFour]',
193
+ '[0]{secondHalfKey}{keyFour}',
194
4
195
);
196
@@ -219,13 +219,13 @@ describe('traverseAllChildren', function() {
219
expect(traverseFn).toHaveBeenCalledWith(
220
traverseContext,
221
zeroForceKey,
222
- '[keyZero]',
222
+ '{keyZero}',
223
0
224
);
225
expect(traverseFn).toHaveBeenCalledWith(
226
traverseContext,
227
oneForceKey,
228
- '[keyOne]',
228
+ '{keyOne}',
229
1
230
);
231
});
src/utils/traverseAllChildren.js
+2
-2
@@ -46,7 +46,7 @@ var traverseAllChildrenImpl =
46
if (Array.isArray(children)) {
47
for (var i = 0; i < children.length; i++) {
48
var child = children[i];
49
- var nextName = nameSoFar + '[' + ReactComponent.getKey(child, i) + ']';
49
+ var nextName = nameSoFar + ReactComponent.getKey(child, i);
50
var nextIndex = indexSoFar + subtreeCount;
51
subtreeCount += traverseAllChildrenImpl(
52
child,
@@ -62,7 +62,7 @@ var traverseAllChildrenImpl =
62
// If it's the only child, treat the name as if it was wrapped in an array
63
// so that it's consistent if the number of children grows
64
var storageName = isOnlyChild ?
65
- '[' + ReactComponent.getKey(children, 0) + ']' :
65
+ ReactComponent.getKey(children, 0):
66
nameSoFar;
67
if (children === null || children === undefined || type === 'boolean') {
68
// All of the above are perceived as null.