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

packages/react-dom/src/client/ReactDOMOption.js

@@ -25,7 +25,7 @@ function flattenChildren(children) {
     if (child == null) {
       return;
     }
-    content += child;
+    content += (child: any);
     // Note: we don't warn about invalid children here.
     // Instead, this is done separately below so that
     // it happens during the hydration codepath too.
@@ -52,7 +52,7 @@ export function validateProps(element: Element, props: Object) {
         if (typeof child === 'string' || typeof child === 'number') {
           return;
         }
-        if (typeof child.type !== 'string') {
+        if (typeof (child: any).type !== 'string') {
           return;
         }
         if (!didWarnInvalidChild) {

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -315,11 +315,11 @@ function flattenOptionChildren(children: mixed): ?string {
   let content = '';
   // Flatten children and warn if they aren't strings or numbers;
   // invalid types are ignored.
-  React.Children.forEach(children, function(child) {
+  React.Children.forEach((children: any), function(child) {
     if (child == null) {
       return;
     }
-    content += child;
+    content += (child: any);
     if (__DEV__) {
       if (
         !didWarnInvalidOptionChildren &&

packages/react/src/ReactChildren.js

@@ -3,8 +3,12 @@
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
  */
 
+import type {ReactNodeList} from 'shared/ReactTypes';
+
 import invariant from 'shared/invariant';
 import {
   getIteratorFn,
@@ -25,13 +29,13 @@ const SUBSEPARATOR = ':';
  * @param {string} key to be escaped.
  * @return {string} the escaped key.
  */
-function escape(key) {
+function escape(key: string): string {
   const escapeRegex = /[=:]/g;
   const escaperLookup = {
     '=': '=0',
     ':': '=2',
   };
-  const escapedString = ('' + key).replace(escapeRegex, function(match) {
+  const escapedString = key.replace(escapeRegex, function(match) {
     return escaperLookup[match];
   });
 
@@ -46,33 +50,35 @@ function escape(key) {
 let didWarnAboutMaps = false;
 
 const userProvidedKeyEscapeRegex = /\/+/g;
-function escapeUserProvidedKey(text) {
-  return ('' + text).replace(userProvidedKeyEscapeRegex, '$&/');
+function escapeUserProvidedKey(text: string): string {
+  return text.replace(userProvidedKeyEscapeRegex, '$&/');
 }
 
 /**
- * Generate a key string that identifies a component within a set.
+ * Generate a key string that identifies a element within a set.
  *
- * @param {*} component A component that could contain a manual key.
+ * @param {*} element A element that could contain a manual key.
  * @param {number} index Index that is used if a manual key is not provided.
  * @return {string}
  */
-function getComponentKey(component, index) {
+function getElementKey(element: any, index: number): string {
   // Do some typechecking here since we call this blindly. We want to ensure
   // that we don't block potential future ES APIs.
-  if (
-    typeof component === 'object' &&
-    component !== null &&
-    component.key != null
-  ) {
+  if (typeof element === 'object' && element !== null && element.key != null) {
     // Explicit key
-    return escape(component.key);
+    return escape('' + element.key);
   }
   // Implicit key determined by the index in the set
   return index.toString(36);
 }
 
-function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
+function mapIntoArray(
+  children: ?ReactNodeList,
+  array: Array<React$Node>,
+  escapedPrefix: string,
+  nameSoFar: string,
+  callback: (?React$Node) => ?ReactNodeList,
+): number {
   const type = typeof children;
 
   if (type === 'undefined' || type === 'boolean') {
@@ -91,7 +97,7 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
         invokeCallback = true;
         break;
       case 'object':
-        switch (children.$$typeof) {
+        switch ((children: any).$$typeof) {
           case REACT_ELEMENT_TYPE:
           case REACT_PORTAL_TYPE:
             invokeCallback = true;
@@ -105,22 +111,24 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
     // If it's the only child, treat the name as if it was wrapped in an array
     // so that it's consistent if the number of children grows:
     let childKey =
-      nameSoFar === '' ? SEPARATOR + getComponentKey(child, 0) : nameSoFar;
+      nameSoFar === '' ? SEPARATOR + getElementKey(child, 0) : nameSoFar;
     if (Array.isArray(mappedChild)) {
       let escapedChildKey = '';
       if (childKey != null) {
         escapedChildKey = escapeUserProvidedKey(childKey) + '/';
       }
-      mapIntoArray(mappedChild, array, escapedChildKey, c => c);
+      mapIntoArray(mappedChild, array, escapedChildKey, '', c => c);
     } else if (mappedChild != null) {
       if (isValidElement(mappedChild)) {
         mappedChild = cloneAndReplaceKey(
           mappedChild,
           // Keep both the (mapped) and old keys if they differ, just as
           // traverseAllChildren used to do for objects as children
           escapedPrefix +
+            // $FlowFixMe Flow incorrectly thinks React.Portal doesn't have a key
             (mappedChild.key && (!child || child.key !== mappedChild.key)
-              ? escapeUserProvidedKey(mappedChild.key) + '/'
+              ? // $FlowFixMe Flow incorrectly thinks existing element's key can be a number
+                escapeUserProvidedKey('' + mappedChild.key) + '/'
               : '') +
             childKey,
         );
@@ -139,7 +147,7 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
   if (Array.isArray(children)) {
     for (let i = 0; i < children.length; i++) {
       child = children[i];
-      nextName = nextNamePrefix + getComponentKey(child, i);
+      nextName = nextNamePrefix + getElementKey(child, i);
       subtreeCount += mapIntoArray(
         child,
         array,
@@ -151,18 +159,21 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
   } else {
     const iteratorFn = getIteratorFn(children);
     if (typeof iteratorFn === 'function') {
+      const iterableChildren: Iterable<React$Node> & {
+        entries: any,
+      } = (children: any);
       if (disableMapsAsChildren) {
         invariant(
-          iteratorFn !== children.entries,
+          iteratorFn !== iterableChildren.entries,
           'Maps are not valid as a React child (found: %s). Consider converting ' +
             'children to an array of keyed ReactElements instead.',
-          children,
+          iterableChildren,
         );
       }
 
       if (__DEV__) {
         // Warn about using Maps as children
-        if (iteratorFn === children.entries) {
+        if (iteratorFn === iterableChildren.entries) {
           if (!didWarnAboutMaps) {
             console.warn(
               'Using Maps as children is deprecated and will be removed in ' +
@@ -174,12 +185,12 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
         }
       }
 
-      const iterator = iteratorFn.call(children);
+      const iterator = iteratorFn.call(iterableChildren);
       let step;
       let ii = 0;
       while (!(step = iterator.next()).done) {
         child = step.value;
-        nextName = nextNamePrefix + getComponentKey(child, ii++);
+        nextName = nextNamePrefix + getElementKey(child, ii++);
         subtreeCount += mapIntoArray(
           child,
           array,
@@ -196,12 +207,12 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
           'instead.' +
           ReactDebugCurrentFrame.getStackAddendum();
       }
-      const childrenString = '' + children;
+      const childrenString = '' + (children: any);
       invariant(
         false,
         'Objects are not valid as a React child (found: %s).%s',
         childrenString === '[object Object]'
-          ? 'object with keys {' + Object.keys(children).join(', ') + '}'
+          ? 'object with keys {' + Object.keys((children: any)).join(', ') + '}'
           : childrenString,
         addendum,
       );
@@ -211,6 +222,8 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
   return subtreeCount;
 }
 
+type MapFunc = (child: ?React$Node) => ?ReactNodeList;
+
 /**
  * Maps children that are typically specified as `props.children`.
  *
@@ -224,22 +237,19 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) {
  * @param {*} context Context for mapFunction.
  * @return {object} Object containing the ordered map of results.
  */
-function mapChildren(children, func, context) {
+function mapChildren(
+  children: ?ReactNodeList,
+  func: MapFunc,
+  context: mixed,
+): ?Array<React$Node> {
   if (children == null) {
     return children;
   }
   const result = [];
   let count = 0;
-  mapIntoArray(
-    children,
-    result,
-    '',
-    '',
-    function(child) {
-      return func.call(context, child, count++);
-    },
-    context,
-  );
+  mapIntoArray(children, result, '', '', function(child) {
+    return func.call(context, child, count++);
+  });
   return result;
 }
 
@@ -252,12 +262,17 @@ function mapChildren(children, func, context) {
  * @param {?*} children Children tree container.
  * @return {number} The number of children.
  */
-function countChildren(children) {
+function countChildren(children: ?ReactNodeList): number {
   let n = 0;
-  mapChildren(children, () => n++);
+  mapChildren(children, () => {
+    n++;
+    // Don't return anything
+  });
   return n;
 }
 
+type ForEachFunc = (child: ?React$Node) => void;
+
 /**
  * Iterates through children that are typically specified as `props.children`.
  *
@@ -270,7 +285,11 @@ function countChildren(children) {
  * @param {function(*, int)} forEachFunc
  * @param {*} forEachContext Context for forEachContext.
  */
-function forEachChildren(children, forEachFunc, forEachContext) {
+function forEachChildren(
+  children: ?ReactNodeList,
+  forEachFunc: ForEachFunc,
+  forEachContext: mixed,
+): void {
   mapChildren(
     children,
     function() {
@@ -287,7 +306,7 @@ function forEachChildren(children, forEachFunc, forEachContext) {
  *
  * See https://reactjs.org/docs/react-api.html#reactchildrentoarray
  */
-function toArray(children) {
+function toArray(children: ?ReactNodeList): Array<React$Node> {
   return mapChildren(children, child => child) || [];
 }
 
@@ -305,7 +324,7 @@ function toArray(children) {
  * @return {ReactElement} The first and only `ReactElement` contained in the
  * structure.
  */
-function onlyChild(children) {
+function onlyChild<T>(children: T): T {
   invariant(
     isValidElement(children),
     'React.Children.only expected to receive a single React element child.',

packages/react/src/__tests__/ReactChildren-test.js

@@ -866,6 +866,74 @@ describe('ReactChildren', () => {
     ]);
   });
 
+  it('should combine keys when map returns an array', () => {
+    const instance = (
+      <div>
+        <div key="a" />
+        {false}
+        <div key="b" />
+        <p />
+      </div>
+    );
+    const mappedChildren = React.Children.map(
+      instance.props.children,
+      // Try a few things: keyed, unkeyed, hole, and a cloned element.
+      kid => [
+        <span key="x" />,
+        null,
+        <span key="y" />,
+        kid,
+        kid && React.cloneElement(kid, {key: 'z'}),
+        <hr />,
+      ],
+    );
+    expect(mappedChildren.length).toBe(18);
+
+    // <div key="a">
+    expect(mappedChildren[0].type).toBe('span');
+    expect(mappedChildren[0].key).toBe('.$a/.$x');
+    expect(mappedChildren[1].type).toBe('span');
+    expect(mappedChildren[1].key).toBe('.$a/.$y');
+    expect(mappedChildren[2].type).toBe('div');
+    expect(mappedChildren[2].key).toBe('.$a/.$a');
+    expect(mappedChildren[3].type).toBe('div');
+    expect(mappedChildren[3].key).toBe('.$a/.$z');
+    expect(mappedChildren[4].type).toBe('hr');
+    expect(mappedChildren[4].key).toBe('.$a/.5');
+
+    // false
+    expect(mappedChildren[5].type).toBe('span');
+    expect(mappedChildren[5].key).toBe('.1/.$x');
+    expect(mappedChildren[6].type).toBe('span');
+    expect(mappedChildren[6].key).toBe('.1/.$y');
+    expect(mappedChildren[7].type).toBe('hr');
+    expect(mappedChildren[7].key).toBe('.1/.5');
+
+    // <div key="b">
+    expect(mappedChildren[8].type).toBe('span');
+    expect(mappedChildren[8].key).toBe('.$b/.$x');
+    expect(mappedChildren[9].type).toBe('span');
+    expect(mappedChildren[9].key).toBe('.$b/.$y');
+    expect(mappedChildren[10].type).toBe('div');
+    expect(mappedChildren[10].key).toBe('.$b/.$b');
+    expect(mappedChildren[11].type).toBe('div');
+    expect(mappedChildren[11].key).toBe('.$b/.$z');
+    expect(mappedChildren[12].type).toBe('hr');
+    expect(mappedChildren[12].key).toBe('.$b/.5');
+
+    // <p>
+    expect(mappedChildren[13].type).toBe('span');
+    expect(mappedChildren[13].key).toBe('.3/.$x');
+    expect(mappedChildren[14].type).toBe('span');
+    expect(mappedChildren[14].key).toBe('.3/.$y');
+    expect(mappedChildren[15].type).toBe('p');
+    expect(mappedChildren[15].key).toBe('.3/.3');
+    expect(mappedChildren[16].type).toBe('p');
+    expect(mappedChildren[16].key).toBe('.3/.$z');
+    expect(mappedChildren[17].type).toBe('hr');
+    expect(mappedChildren[17].key).toBe('.3/.5');
+  });
+
   it('should throw on object', () => {
     expect(function() {
       React.Children.forEach({a: 1, b: 2}, function() {}, null);