Make React.Children.map do one pass only
Changed: createFragment no longer includes nulls in its output (since toArray doesn't and they share code). Changed: React.Children.map no longer warns for clashing keys; traverseAllChildren will later when rendering. Also change the generated key format around to accomodate. My keys are probably not bulletproof but should definitely be safe against accidental collisions.
Ben Alpert committed
Aug 27, 2015 at 18:01 UTC
6013105a9cf625cac18851683adbf2fd19b6833c
4 files changed
+122
-137
src/addons/ReactFragment.js
+8
-20
@@ -11,10 +11,11 @@
11
12
'use strict';
13
14
+var ReactChildren = require('ReactChildren');
15
var ReactElement = require('ReactElement');
16
17
+var emptyFunction = require('emptyFunction');
18
var invariant = require('invariant');
17
-var traverseAllChildren = require('traverseAllChildren');
19
var warning = require('warning');
20
21
/**
@@ -27,19 +28,6 @@ var warning = require('warning');
28
29
var numericPropertyRegex = /^\d+$/;
30
30
-var userProvidedKeyEscapeRegex = /\//g;
31
-function escapeUserProvidedKey(text) {
32
- return ('' + text).replace(userProvidedKeyEscapeRegex, '//');
33
-}
34
-
35
-function processSingleChildWithContext(ctx, child, childKey) {
36
- if (ReactElement.isValidElement(child)) {
37
- child = ReactElement.cloneAndReplaceKey(child, ctx.prefix + childKey);
38
- }
39
- // For text components, leave unkeyed
40
- ctx.result.push(child);
41
-}
42
-
31
var warnedAboutNumeric = false;
32
33
var ReactFragment = {
@@ -71,10 +59,6 @@ var ReactFragment = {
59
);
60
61
var result = [];
74
- var context = {
75
- result: result,
76
- prefix: '',
77
- };
62
63
for (var key in object) {
64
if (__DEV__) {
@@ -87,8 +71,12 @@ var ReactFragment = {
71
warnedAboutNumeric = true;
72
}
73
}
90
- context.prefix = escapeUserProvidedKey(key) + '/';
91
- traverseAllChildren(object[key], processSingleChildWithContext, context);
74
+ ReactChildren.mapIntoWithKeyPrefixInternal(
75
+ object[key],
76
+ result,
77
+ key,
78
+ emptyFunction.thatReturnsArgument
79
+ );
80
}
81
82
return result;
src/isomorphic/children/ReactChildren.js
+66
-51
@@ -13,13 +13,18 @@
13
14
var PooledClass = require('PooledClass');
15
var ReactElement = require('ReactElement');
16
-var ReactFragment = require('ReactFragment');
16
17
+var emptyFunction = require('emptyFunction');
18
var traverseAllChildren = require('traverseAllChildren');
19
-var warning = require('warning');
19
20
var twoArgumentPooler = PooledClass.twoArgumentPooler;
22
-var threeArgumentPooler = PooledClass.threeArgumentPooler;
21
+var fourArgumentPooler = PooledClass.fourArgumentPooler;
22
+
23
+
24
+var userProvidedKeyEscapeRegex = /\/(?!\/)/g;
25
+function escapeUserProvidedKey(text) {
26
+ return ('' + text).replace(userProvidedKeyEscapeRegex, '//');
27
+}
28
29
30
/**
@@ -42,9 +47,9 @@ ForEachBookKeeping.prototype.destructor = function() {
47
};
48
PooledClass.addPoolingTo(ForEachBookKeeping, twoArgumentPooler);
49
45
-function forEachSingleChild(traverseContext, child, name) {
46
- var bookKeeping = traverseContext;
47
- bookKeeping.func.call(bookKeeping.context, child, bookKeeping.count++);
50
+function forEachSingleChild(bookKeeping, child, name) {
51
+ var {func, context} = bookKeeping;
52
+ func.call(context, child, bookKeeping.count++);
53
}
54
55
/**
@@ -61,7 +66,6 @@ function forEachChildren(children, forEachFunc, forEachContext) {
66
if (children == null) {
67
return children;
68
}
64
-
69
var traverseContext =
70
ForEachBookKeeping.getPooled(forEachFunc, forEachContext);
71
traverseAllChildren(children, forEachSingleChild, traverseContext);
@@ -78,40 +82,65 @@ function forEachChildren(children, forEachFunc, forEachContext) {
82
* @param {!function} mapFunction Function to perform mapping with.
83
* @param {?*} mapContext Context to perform mapping with.
84
*/
81
-function MapBookKeeping(mapResult, mapFunction, mapContext) {
85
+function MapBookKeeping(mapResult, keyPrefix, mapFunction, mapContext) {
86
this.result = mapResult;
87
+ this.keyPrefix = keyPrefix;
88
this.func = mapFunction;
89
this.context = mapContext;
90
this.count = 0;
91
}
92
MapBookKeeping.prototype.destructor = function() {
93
this.result = null;
94
+ this.keyPrefix = null;
95
this.func = null;
96
this.context = null;
97
this.count = 0;
98
};
93
-PooledClass.addPoolingTo(MapBookKeeping, threeArgumentPooler);
94
-
95
-function mapSingleChildIntoContext(traverseContext, child, name) {
96
- var bookKeeping = traverseContext;
97
- var mapResult = bookKeeping.result;
98
-
99
- var keyUnique = (mapResult[name] === undefined);
100
- if (__DEV__) {
101
- warning(
102
- keyUnique,
103
- 'ReactChildren.map(...): Encountered two children with the same key, ' +
104
- '`%s`. Child keys must be unique; when two children share a key, only ' +
105
- 'the first child will be used.',
106
- name
99
+PooledClass.addPoolingTo(MapBookKeeping, fourArgumentPooler);
100
+
101
+function mapSingleChildIntoContext(bookKeeping, child, childKey) {
102
+ var {result, keyPrefix, func, context} = bookKeeping;
103
+
104
+ var mappedChild = func.call(context, child, bookKeeping.count++);
105
+ if (Array.isArray(mappedChild)) {
106
+ mapIntoWithKeyPrefixInternal(
107
+ mappedChild,
108
+ result,
109
+ childKey,
110
+ emptyFunction.thatReturnsArgument
111
);
112
+ } else if (mappedChild != null) {
113
+ if (ReactElement.isValidElement(mappedChild)) {
114
+ mappedChild = ReactElement.cloneAndReplaceKey(
115
+ mappedChild,
116
+ // Keep both the (mapped) and old keys if they differ, just as
117
+ // traverseAllChildren used to do for objects as children
118
+ keyPrefix +
119
+ (
120
+ mappedChild !== child ?
121
+ escapeUserProvidedKey(mappedChild.key || '') + '/' :
122
+ ''
123
+ ) +
124
+ childKey
125
+ );
126
+ }
127
+ result.push(mappedChild);
128
}
129
+}
130
110
- if (keyUnique) {
111
- var mappedChild =
112
- bookKeeping.func.call(bookKeeping.context, child, bookKeeping.count++);
113
- mapResult[name] = mappedChild;
131
+function mapIntoWithKeyPrefixInternal(children, array, prefix, func, context) {
132
+ var escapedPrefix = '';
133
+ if (prefix != null) {
134
+ escapedPrefix = escapeUserProvidedKey(prefix) + '/';
135
}
136
+ var traverseContext = MapBookKeeping.getPooled(
137
+ array,
138
+ escapedPrefix,
139
+ func,
140
+ context
141
+ );
142
+ traverseAllChildren(children, mapSingleChildIntoContext, traverseContext);
143
+ MapBookKeeping.release(traverseContext);
144
}
145
146
/**
@@ -120,9 +149,6 @@ function mapSingleChildIntoContext(traverseContext, child, name) {
149
* The provided mapFunction(child, key, index) will be called for each
150
* leaf child.
151
*
123
- * TODO: This may likely break any calls to `ReactChildren.map` that were
124
- * previously relying on the fact that we guarded against null children.
125
- *
152
* @param {?*} children Children tree container.
153
* @param {function(*, int)} func The map function.
154
* @param {*} context Context for mapFunction.
@@ -132,15 +158,13 @@ function mapChildren(children, func, context) {
158
if (children == null) {
159
return children;
160
}
135
-
136
- var mapResult = {};
137
- var traverseContext = MapBookKeeping.getPooled(mapResult, func, context);
138
- traverseAllChildren(children, mapSingleChildIntoContext, traverseContext);
139
- MapBookKeeping.release(traverseContext);
140
- return ReactFragment.create(mapResult);
161
+ var result = [];
162
+ mapIntoWithKeyPrefixInternal(children, result, null, func, context);
163
+ return result;
164
}
165
166
167
+
168
function forEachSingleChildDummy(traverseContext, child, name) {
169
return null;
170
}
@@ -157,28 +181,18 @@ function countChildren(children, context) {
181
}
182
183
160
-function pushSingleChildToArray(traverseContext, child, name) {
161
- if (child == null) {
162
- return;
163
- }
164
-
165
- var result = traverseContext;
166
-
167
- if (ReactElement.isValidElement(child)) {
168
- child = ReactElement.cloneAndReplaceKey(child, name);
169
- }
170
- // For text components, leave unkeyed
171
-
172
- result.push(child);
173
-}
174
-
184
/**
185
* Flatten a children object (typically specified as `props.children`) and
186
* return an array with appropriately re-keyed children.
187
*/
188
function toArray(children) {
189
var result = [];
181
- traverseAllChildren(children, pushSingleChildToArray, result);
190
+ mapIntoWithKeyPrefixInternal(
191
+ children,
192
+ result,
193
+ null,
194
+ emptyFunction.thatReturnsArgument
195
+ );
196
return result;
197
}
198
@@ -186,6 +200,7 @@ function toArray(children) {
200
var ReactChildren = {
201
forEach: forEachChildren,
202
map: mapChildren,
203
+ mapIntoWithKeyPrefixInternal: mapIntoWithKeyPrefixInternal,
204
count: countChildren,
205
toArray: toArray,
206
};
src/isomorphic/children/__tests__/ReactChildren-test.js
+46
-64
@@ -38,7 +38,7 @@ describe('ReactChildren', function() {
38
callback.reset();
39
var mappedChildren = ReactChildren.map(instance.props.children, callback);
40
expect(callback).toHaveBeenCalledWith(simpleKid, 0);
41
- expect(mappedChildren[0]).toEqual(<span key=".$simple/.$simple" />);
41
+ expect(mappedChildren[0]).toEqual(<span key=".$simple" />);
42
});
43
44
it('should treat single arrayless child as being in array', function() {
@@ -53,7 +53,7 @@ describe('ReactChildren', function() {
53
callback.reset();
54
var mappedChildren = ReactChildren.map(instance.props.children, callback);
55
expect(callback).toHaveBeenCalledWith(simpleKid, 0);
56
- expect(mappedChildren[0]).toEqual(<span key=".0/.0" />);
56
+ expect(mappedChildren[0]).toEqual(<span key=".0" />);
57
});
58
59
it('should treat single child in array as expected', function() {
@@ -68,7 +68,7 @@ describe('ReactChildren', function() {
68
callback.reset();
69
var mappedChildren = ReactChildren.map(instance.props.children, callback);
70
expect(callback).toHaveBeenCalledWith(simpleKid, 0);
71
- expect(mappedChildren[0]).toEqual(<span key=".$simple/.$simple" />);
71
+ expect(mappedChildren[0]).toEqual(<span key=".$simple" />);
72
73
});
74
@@ -85,7 +85,7 @@ describe('ReactChildren', function() {
85
expect(ReactChildren.count(mappedChildren)).toBe(1);
86
expect(mappedChildren[0]).not.toBe(simpleKid);
87
expect(mappedChildren[0].props.children).toBe(simpleKid);
88
- expect(mappedChildren[0].key).toBe('.$simple/.0');
88
+ expect(mappedChildren[0].key).toBe('/.$simple');
89
});
90
91
it('should invoke callback with the right context', function() {
@@ -159,20 +159,19 @@ describe('ReactChildren', function() {
159
mappedChildren[2].key,
160
mappedChildren[3].key,
161
]).toEqual(
162
- ['.$keyZero/.$giraffe', '.$keyTwo/.0', '.3/.0', '.$keyFour/.$keyFour']
162
+ ['giraffe/.$keyZero', '/.$keyTwo', '/.3', 'keyFour/.$keyFour']
163
);
164
165
expect(callback).toHaveBeenCalledWith(zero, 0);
166
- expect(mappedChildren[0]).toEqual(<div key=".$keyZero/.$giraffe" />);
167
-
166
expect(callback).toHaveBeenCalledWith(one, 1);
169
- expect(mappedChildren[1]).toEqual(<div key=".$keyTwo/.0" />);
170
-
167
expect(callback).toHaveBeenCalledWith(two, 2);
172
- expect(mappedChildren[2]).toEqual(<span key=".3/.0" />);
173
-
168
expect(callback).toHaveBeenCalledWith(three, 3);
175
- expect(mappedChildren[3]).toEqual(<div key=".$keyFour/.$keyFour" />);
169
+ expect(callback).toHaveBeenCalledWith(four, 4);
170
+
171
+ expect(mappedChildren[0]).toEqual(<div key="giraffe/.$keyZero" />);
172
+ expect(mappedChildren[1]).toEqual(<div key="/.$keyTwo" />);
173
+ expect(mappedChildren[2]).toEqual(<span key="/.3" />);
174
+ expect(mappedChildren[3]).toEqual(<div key="keyFour/.$keyFour" />);
175
});
176
177
it('should be called for each child in nested structure', function() {
@@ -189,18 +188,14 @@ describe('ReactChildren', function() {
188
// 2. If grouped in an Array, the `key` prop, falling back to array index
189
190
var zeroMapped = <div key="giraffe" />; // Key should be overridden
192
- var oneMapped = null; // Key should be added even if we don't supply it!
191
var twoMapped = <div />; // Key should be added even if not supplied!
194
- var threeMapped = <span />; // Map from null to something.
192
var fourMapped = <div key="keyFour" />;
193
var fiveMapped = <div />;
194
195
var callback = jasmine.createSpy().andCallFake(function(kid, index) {
196
return index === 0 ? zeroMapped :
200
- index === 1 ? oneMapped :
201
- index === 2 ? twoMapped :
202
- index === 3 ? threeMapped :
203
- index === 4 ? fourMapped : fiveMapped;
197
+ index === 1 ? twoMapped :
198
+ index === 2 ? fourMapped : fiveMapped;
199
});
200
201
var frag = ReactFragment.create({
@@ -210,47 +205,51 @@ describe('ReactChildren', function() {
205
});
206
var instance = <div>{[frag]}</div>;
207
208
+ expect([
209
+ frag[0].key,
210
+ frag[1].key,
211
+ frag[2].key,
212
+ frag[3].key,
213
+ ]).toEqual([
214
+ 'firstHalfKey/.$keyZero',
215
+ 'firstHalfKey/.$keyTwo',
216
+ 'secondHalfKey/.$keyFour',
217
+ 'keyFive/.$keyFiveInner',
218
+ ]);
219
+
220
ReactChildren.forEach(instance.props.children, callback);
221
+ expect(callback.calls.length).toBe(4);
222
expect(callback).toHaveBeenCalledWith(frag[0], 0);
223
expect(callback).toHaveBeenCalledWith(frag[1], 1);
224
expect(callback).toHaveBeenCalledWith(frag[2], 2);
225
expect(callback).toHaveBeenCalledWith(frag[3], 3);
218
- expect(callback).toHaveBeenCalledWith(frag[4], 4);
219
- expect(callback).toHaveBeenCalledWith(frag[5], 5);
226
callback.reset();
227
228
var mappedChildren = ReactChildren.map(instance.props.children, callback);
223
- expect(callback.calls.length).toBe(6);
224
- expect(ReactChildren.count(mappedChildren)).toBe(5);
229
+ expect(callback.calls.length).toBe(4);
230
+ expect(callback).toHaveBeenCalledWith(frag[0], 0);
231
+ expect(callback).toHaveBeenCalledWith(frag[1], 1);
232
+ expect(callback).toHaveBeenCalledWith(frag[2], 2);
233
+ expect(callback).toHaveBeenCalledWith(frag[3], 3);
234
+
235
+ expect(ReactChildren.count(mappedChildren)).toBe(4);
236
// Keys default to indices.
237
expect([
238
mappedChildren[0].key,
239
mappedChildren[1].key,
240
mappedChildren[2].key,
241
mappedChildren[3].key,
231
- mappedChildren[4].key,
242
]).toEqual([
233
- '.0:$firstHalfKey//=1$keyZero/.$giraffe',
234
- '.0:$firstHalfKey//=1$keyTwo/.0',
235
- '.0:3/.0',
236
- '.0:$secondHalfKey//=1$keyFour/.$keyFour',
237
- '.0:$keyFive//=1$keyFiveInner/.0',
243
+ 'giraffe/.0:$firstHalfKey/=1$keyZero',
244
+ '/.0:$firstHalfKey/=1$keyTwo',
245
+ 'keyFour/.0:$secondHalfKey/=1$keyFour',
246
+ '/.0:$keyFive/=1$keyFiveInner',
247
]);
248
240
- expect(callback).toHaveBeenCalledWith(frag[0], 0);
241
- expect(mappedChildren[0]).toEqual(<div key=".0:$firstHalfKey//=1$keyZero/.$giraffe" />);
242
-
243
- expect(callback).toHaveBeenCalledWith(frag[1], 1);
244
- expect(mappedChildren[1]).toEqual(<div key=".0:$firstHalfKey//=1$keyTwo/.0" />);
245
-
246
- expect(callback).toHaveBeenCalledWith(frag[2], 2);
247
- expect(mappedChildren[2]).toEqual(<span key=".0:3/.0" />);
248
-
249
- expect(callback).toHaveBeenCalledWith(frag[3], 3);
250
- expect(mappedChildren[3]).toEqual(<div key=".0:$secondHalfKey//=1$keyFour/.$keyFour" />);
251
-
252
- expect(callback).toHaveBeenCalledWith(frag[4], 4);
253
- expect(mappedChildren[4]).toEqual(<div key=".0:$keyFive//=1$keyFiveInner/.0" />);
249
+ expect(mappedChildren[0]).toEqual(<div key="giraffe/.0:$firstHalfKey/=1$keyZero" />);
250
+ expect(mappedChildren[1]).toEqual(<div key="/.0:$firstHalfKey/=1$keyTwo" />);
251
+ expect(mappedChildren[2]).toEqual(<div key="keyFour/.0:$secondHalfKey/=1$keyFour" />);
252
+ expect(mappedChildren[3]).toEqual(<div key="/.0:$keyFive/=1$keyFiveInner" />);
253
});
254
255
it('should retain key across two mappings', function() {
@@ -273,15 +272,15 @@ describe('ReactChildren', function() {
272
</div>
273
);
274
276
- var expectedForcedKeys = ['.$keyZero/.$giraffe', '.$keyOne/.0'];
275
+ var expectedForcedKeys = ['giraffe/.$keyZero', '/.$keyOne'];
276
var mappedChildrenForcedKeys =
277
ReactChildren.map(forcedKeys.props.children, mapFn);
278
var mappedForcedKeys = mappedChildrenForcedKeys.map((c) => c.key);
279
expect(mappedForcedKeys).toEqual(expectedForcedKeys);
280
281
var expectedRemappedForcedKeys = [
283
- '.$=1$keyZero//=1$giraffe/.$giraffe',
284
- '.$=1$keyOne//=10/.0',
282
+ 'giraffe/.$giraffe/=1$keyZero',
283
+ '/.$/=1$keyOne',
284
];
285
var remappedChildrenForcedKeys =
286
ReactChildren.map(mappedChildrenForcedKeys, mapFn);
@@ -311,24 +310,6 @@ describe('ReactChildren', function() {
310
}).not.toThrow();
311
});
312
314
- it('should warn if key provided is a dupe with explicit key', function() {
315
- var zero = <div key="something"/>;
316
- var one = <span key="something" />;
317
-
318
- var mapFn = function(component) {
319
- return component;
320
- };
321
- var instance = (
322
- <div>{zero}{one}</div>
323
- );
324
-
325
- spyOn(console, 'error');
326
- var mapped = ReactChildren.map(instance.props.children, mapFn);
327
-
328
- expect(console.error.calls.length).toEqual(1);
329
- expect(mapped[0]).toEqual(<div key=".$something/.$something" />);
330
- });
331
-
313
it('should return 0 for null children', function() {
314
var numberOfChildren = ReactChildren.count(null);
315
expect(numberOfChildren).toBe(0);
@@ -387,11 +368,12 @@ describe('ReactChildren', function() {
368
secondHalfKey: [three, four],
369
keyFive: five,
370
}),
371
+ null,
372
]
373
}</div>
374
);
375
var numberOfChildren = ReactChildren.count(instance.props.children);
394
- expect(numberOfChildren).toBe(6);
376
+ expect(numberOfChildren).toBe(5);
377
});
378
379
it('should flatten children to an array', function() {
src/shared/utils/__tests__/traverseAllChildren-test.js
+2
-2
@@ -220,8 +220,8 @@ describe('traverseAllChildren', function() {
220
);
221
222
traverseAllChildren(instance.props.children, traverseFn, traverseContext);
223
- expect(traverseFn.calls.length).toBe(6);
224
- expect(traverseContext.length).toEqual(6);
223
+ expect(traverseFn.calls.length).toBe(4);
224
+ expect(traverseContext.length).toEqual(4);
225
expect(traverseFn).toHaveBeenCalledWith(
226
traverseContext,
227
<div key="firstHalfKey/.$keyZero" />,