Fix a mistake in ReactChildren refactor (#18380)
* Regression test for map() returning an array * Add forgotten argument This fixes the bug. * Remove unused arg and retval These aren't directly observable. The arg wasn't used, it's accidental and I forgot to remove. The retval was triggering a codepath that was unnecessary (pushing to array) so I removed that too. * Flowify ReactChildren * Tighten up types * Rename getComponentKey to getElementKey
Dan Abramov committed
Mar 25, 2020 at 09:20 UTC
6498f62edcdf0bcc02ed96efdad92b7e89330d8a
4 files changed
+133
-46
packages/react-dom/src/client/ReactDOMOption.js
+2
-2
@@ -25,7 +25,7 @@ function flattenChildren(children) {
25
if (child == null) {
26
return;
27
}
28
- content += child;
28
+ content += (child: any);
29
// Note: we don't warn about invalid children here.
30
// Instead, this is done separately below so that
31
// it happens during the hydration codepath too.
@@ -52,7 +52,7 @@ export function validateProps(element: Element, props: Object) {
52
if (typeof child === 'string' || typeof child === 'number') {
53
return;
54
}
55
- if (typeof child.type !== 'string') {
55
+ if (typeof (child: any).type !== 'string') {
56
return;
57
}
58
if (!didWarnInvalidChild) {
packages/react-dom/src/server/ReactPartialRenderer.js
+2
-2
@@ -315,11 +315,11 @@ function flattenOptionChildren(children: mixed): ?string {
315
let content = '';
316
// Flatten children and warn if they aren't strings or numbers;
317
// invalid types are ignored.
318
- React.Children.forEach(children, function(child) {
318
+ React.Children.forEach((children: any), function(child) {
319
if (child == null) {
320
return;
321
}
322
- content += child;
322
+ content += (child: any);
323
if (__DEV__) {
324
if (
325
!didWarnInvalidOptionChildren &&
packages/react/src/ReactChildren.js
+61
-42
@@ -3,8 +3,12 @@
3
*
4
* This source code is licensed under the MIT license found in the
5
* LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @flow
8
*/
9
10
+import type {ReactNodeList} from 'shared/ReactTypes';
11
+
12
import invariant from 'shared/invariant';
13
import {
14
getIteratorFn,
@@ -25,13 +29,13 @@ const SUBSEPARATOR = ':';
29
* @param {string} key to be escaped.
30
* @return {string} the escaped key.
31
*/
28
-function escape(key) {
32
+function escape(key: string): string {
33
const escapeRegex = /[=:]/g;
34
const escaperLookup = {
35
'=': '=0',
36
':': '=2',
37
};
34
- const escapedString = ('' + key).replace(escapeRegex, function(match) {
38
+ const escapedString = key.replace(escapeRegex, function(match) {
39
return escaperLookup[match];
40
});
41
@@ -46,33 +50,35 @@ function escape(key) {
50
let didWarnAboutMaps = false;
51
52
const userProvidedKeyEscapeRegex = /\/+/g;
49
-function escapeUserProvidedKey(text) {
50
- return ('' + text).replace(userProvidedKeyEscapeRegex, '$&/');
53
+function escapeUserProvidedKey(text: string): string {
54
+ return text.replace(userProvidedKeyEscapeRegex, '$&/');
55
}
56
57
/**
54
- * Generate a key string that identifies a component within a set.
58
+ * Generate a key string that identifies a element within a set.
59
*
56
- * @param {*} component A component that could contain a manual key.
60
+ * @param {*} element A element that could contain a manual key.
61
* @param {number} index Index that is used if a manual key is not provided.
62
* @return {string}
63
*/
60
-function getComponentKey(component, index) {
64
+function getElementKey(element: any, index: number): string {
65
// Do some typechecking here since we call this blindly. We want to ensure
66
// that we don't block potential future ES APIs.
63
- if (
64
- typeof component === 'object' &&
65
- component !== null &&
66
- component.key != null
67
- ) {
67
+ if (typeof element === 'object' && element !== null && element.key != null) {
68
// Explicit key
69
- return escape(component.key);
69
+ return escape('' + element.key);
70
}
71
// Implicit key determined by the index in the set
72
return index.toString(36);
73
}
74
75
-function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
75
+function mapIntoArray(
76
+ children: ?ReactNodeList,
77
+ array: Array<React$Node>,
78
+ escapedPrefix: string,
79
+ nameSoFar: string,
80
+ callback: (?React$Node) => ?ReactNodeList,
81
+): number {
82
const type = typeof children;
83
84
if (type === 'undefined' || type === 'boolean') {
@@ -91,7 +97,7 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
97
invokeCallback = true;
98
break;
99
case 'object':
94
- switch (children.$$typeof) {
100
+ switch ((children: any).$$typeof) {
101
case REACT_ELEMENT_TYPE:
102
case REACT_PORTAL_TYPE:
103
invokeCallback = true;
@@ -105,13 +111,13 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
111
// If it's the only child, treat the name as if it was wrapped in an array
112
// so that it's consistent if the number of children grows:
113
let childKey =
108
- nameSoFar === '' ? SEPARATOR + getComponentKey(child, 0) : nameSoFar;
114
+ nameSoFar === '' ? SEPARATOR + getElementKey(child, 0) : nameSoFar;
115
if (Array.isArray(mappedChild)) {
116
let escapedChildKey = '';
117
if (childKey != null) {
118
escapedChildKey = escapeUserProvidedKey(childKey) + '/';
119
}
114
- mapIntoArray(mappedChild, array, escapedChildKey, c => c);
120
+ mapIntoArray(mappedChild, array, escapedChildKey, '', c => c);
121
} else if (mappedChild != null) {
122
if (isValidElement(mappedChild)) {
123
mappedChild = cloneAndReplaceKey(
@@ -119,8 +125,10 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
125
// Keep both the (mapped) and old keys if they differ, just as
126
// traverseAllChildren used to do for objects as children
127
escapedPrefix +
128
+ // $FlowFixMe Flow incorrectly thinks React.Portal doesn't have a key
129
(mappedChild.key && (!child || child.key !== mappedChild.key)
123
- ? escapeUserProvidedKey(mappedChild.key) + '/'
130
+ ? // $FlowFixMe Flow incorrectly thinks existing element's key can be a number
131
+ escapeUserProvidedKey('' + mappedChild.key) + '/'
132
: '') +
133
childKey,
134
);
@@ -139,7 +147,7 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
147
if (Array.isArray(children)) {
148
for (let i = 0; i < children.length; i++) {
149
child = children[i];
142
- nextName = nextNamePrefix + getComponentKey(child, i);
150
+ nextName = nextNamePrefix + getElementKey(child, i);
151
subtreeCount += mapIntoArray(
152
child,
153
array,
@@ -151,18 +159,21 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
159
} else {
160
const iteratorFn = getIteratorFn(children);
161
if (typeof iteratorFn === 'function') {
162
+ const iterableChildren: Iterable<React$Node> & {
163
+ entries: any,
164
+ } = (children: any);
165
if (disableMapsAsChildren) {
166
invariant(
156
- iteratorFn !== children.entries,
167
+ iteratorFn !== iterableChildren.entries,
168
'Maps are not valid as a React child (found: %s). Consider converting ' +
169
'children to an array of keyed ReactElements instead.',
159
- children,
170
+ iterableChildren,
171
);
172
}
173
174
if (__DEV__) {
175
// Warn about using Maps as children
165
- if (iteratorFn === children.entries) {
176
+ if (iteratorFn === iterableChildren.entries) {
177
if (!didWarnAboutMaps) {
178
console.warn(
179
'Using Maps as children is deprecated and will be removed in ' +
@@ -174,12 +185,12 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
185
}
186
}
187
177
- const iterator = iteratorFn.call(children);
188
+ const iterator = iteratorFn.call(iterableChildren);
189
let step;
190
let ii = 0;
191
while (!(step = iterator.next()).done) {
192
child = step.value;
182
- nextName = nextNamePrefix + getComponentKey(child, ii++);
193
+ nextName = nextNamePrefix + getElementKey(child, ii++);
194
subtreeCount += mapIntoArray(
195
child,
196
array,
@@ -196,12 +207,12 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
207
'instead.' +
208
ReactDebugCurrentFrame.getStackAddendum();
209
}
199
- const childrenString = '' + children;
210
+ const childrenString = '' + (children: any);
211
invariant(
212
false,
213
'Objects are not valid as a React child (found: %s).%s',
214
childrenString === '[object Object]'
204
- ? 'object with keys {' + Object.keys(children).join(', ') + '}'
215
+ ? 'object with keys {' + Object.keys((children: any)).join(', ') + '}'
216
: childrenString,
217
addendum,
218
);
@@ -211,6 +222,8 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
222
return subtreeCount;
223
}
224
225
+type MapFunc = (child: ?React$Node) => ?ReactNodeList;
226
+
227
/**
228
* Maps children that are typically specified as `props.children`.
229
*
@@ -224,22 +237,19 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
237
* @param {*} context Context for mapFunction.
238
* @return {object} Object containing the ordered map of results.
239
*/
227
-function mapChildren(children, func, context) {
240
+function mapChildren(
241
+ children: ?ReactNodeList,
242
+ func: MapFunc,
243
+ context: mixed,
244
+): ?Array<React$Node> {
245
if (children == null) {
246
return children;
247
}
248
const result = [];
249
let count = 0;
233
- mapIntoArray(
234
- children,
235
- result,
236
- '',
237
- '',
238
- function(child) {
239
- return func.call(context, child, count++);
240
- },
241
- context,
242
- );
250
+ mapIntoArray(children, result, '', '', function(child) {
251
+ return func.call(context, child, count++);
252
+ });
253
return result;
254
}
255
@@ -252,12 +262,17 @@ function mapChildren(children, func, context) {
262
* @param {?*} children Children tree container.
263
* @return {number} The number of children.
264
*/
255
-function countChildren(children) {
265
+function countChildren(children: ?ReactNodeList): number {
266
let n = 0;
257
- mapChildren(children, () => n++);
267
+ mapChildren(children, () => {
268
+ n++;
269
+ // Don't return anything
270
+ });
271
return n;
272
}
273
274
+type ForEachFunc = (child: ?React$Node) => void;
275
+
276
/**
277
* Iterates through children that are typically specified as `props.children`.
278
*
@@ -270,7 +285,11 @@ function countChildren(children) {
285
* @param {function(*, int)} forEachFunc
286
* @param {*} forEachContext Context for forEachContext.
287
*/
273
-function forEachChildren(children, forEachFunc, forEachContext) {
288
+function forEachChildren(
289
+ children: ?ReactNodeList,
290
+ forEachFunc: ForEachFunc,
291
+ forEachContext: mixed,
292
+): void {
293
mapChildren(
294
children,
295
function() {
@@ -287,7 +306,7 @@ function forEachChildren(children, forEachFunc, forEachContext) {
306
*
307
* See https://reactjs.org/docs/react-api.html#reactchildrentoarray
308
*/
290
-function toArray(children) {
309
+function toArray(children: ?ReactNodeList): Array<React$Node> {
310
return mapChildren(children, child => child) || [];
311
}
312
@@ -305,7 +324,7 @@ function toArray(children) {
324
* @return {ReactElement} The first and only `ReactElement` contained in the
325
* structure.
326
*/
308
-function onlyChild(children) {
327
+function onlyChild<T>(children: T): T {
328
invariant(
329
isValidElement(children),
330
'React.Children.only expected to receive a single React element child.',
packages/react/src/__tests__/ReactChildren-test.js
+68
@@ -866,6 +866,74 @@ describe('ReactChildren', () => {
866
]);
867
});
868
869
+ it('should combine keys when map returns an array', () => {
870
+ const instance = (
871
+ <div>
872
+ <div key="a" />
873
+ {false}
874
+ <div key="b" />
875
+ <p />
876
+ </div>
877
+ );
878
+ const mappedChildren = React.Children.map(
879
+ instance.props.children,
880
+ // Try a few things: keyed, unkeyed, hole, and a cloned element.
881
+ kid => [
882
+ <span key="x" />,
883
+ null,
884
+ <span key="y" />,
885
+ kid,
886
+ kid && React.cloneElement(kid, {key: 'z'}),
887
+ <hr />,
888
+ ],
889
+ );
890
+ expect(mappedChildren.length).toBe(18);
891
+
892
+ // <div key="a">
893
+ expect(mappedChildren[0].type).toBe('span');
894
+ expect(mappedChildren[0].key).toBe('.$a/.$x');
895
+ expect(mappedChildren[1].type).toBe('span');
896
+ expect(mappedChildren[1].key).toBe('.$a/.$y');
897
+ expect(mappedChildren[2].type).toBe('div');
898
+ expect(mappedChildren[2].key).toBe('.$a/.$a');
899
+ expect(mappedChildren[3].type).toBe('div');
900
+ expect(mappedChildren[3].key).toBe('.$a/.$z');
901
+ expect(mappedChildren[4].type).toBe('hr');
902
+ expect(mappedChildren[4].key).toBe('.$a/.5');
903
+
904
+ // false
905
+ expect(mappedChildren[5].type).toBe('span');
906
+ expect(mappedChildren[5].key).toBe('.1/.$x');
907
+ expect(mappedChildren[6].type).toBe('span');
908
+ expect(mappedChildren[6].key).toBe('.1/.$y');
909
+ expect(mappedChildren[7].type).toBe('hr');
910
+ expect(mappedChildren[7].key).toBe('.1/.5');
911
+
912
+ // <div key="b">
913
+ expect(mappedChildren[8].type).toBe('span');
914
+ expect(mappedChildren[8].key).toBe('.$b/.$x');
915
+ expect(mappedChildren[9].type).toBe('span');
916
+ expect(mappedChildren[9].key).toBe('.$b/.$y');
917
+ expect(mappedChildren[10].type).toBe('div');
918
+ expect(mappedChildren[10].key).toBe('.$b/.$b');
919
+ expect(mappedChildren[11].type).toBe('div');
920
+ expect(mappedChildren[11].key).toBe('.$b/.$z');
921
+ expect(mappedChildren[12].type).toBe('hr');
922
+ expect(mappedChildren[12].key).toBe('.$b/.5');
923
+
924
+ // <p>
925
+ expect(mappedChildren[13].type).toBe('span');
926
+ expect(mappedChildren[13].key).toBe('.3/.$x');
927
+ expect(mappedChildren[14].type).toBe('span');
928
+ expect(mappedChildren[14].key).toBe('.3/.$y');
929
+ expect(mappedChildren[15].type).toBe('p');
930
+ expect(mappedChildren[15].key).toBe('.3/.3');
931
+ expect(mappedChildren[16].type).toBe('p');
932
+ expect(mappedChildren[16].key).toBe('.3/.$z');
933
+ expect(mappedChildren[17].type).toBe('hr');
934
+ expect(mappedChildren[17].key).toBe('.3/.5');
935
+ });
936
+
937
it('should throw on object', () => {
938
expect(function() {
939
React.Children.forEach({a: 1, b: 2}, function() {}, null);