From 344e56c89273c7b01da78467ddc88b63328cab8e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 24 Mar 2020 19:59:38 +0000 Subject: [PATCH 1/5] Regression test for map() returning an array --- .../react/src/__tests__/ReactChildren-test.js | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/packages/react/src/__tests__/ReactChildren-test.js b/packages/react/src/__tests__/ReactChildren-test.js index 06aaf633f3d7d..1a7b74745fe6f 100644 --- a/packages/react/src/__tests__/ReactChildren-test.js +++ b/packages/react/src/__tests__/ReactChildren-test.js @@ -866,6 +866,74 @@ describe('ReactChildren', () => { ]); }); + it('should combine keys when map returns an array', () => { + const instance = ( +
+
+ {false} +
+

+

+ ); + const mappedChildren = React.Children.map( + instance.props.children, + // Try a few things: keyed, unkeyed, hole, and a cloned element. + kid => [ + , + null, + , + kid, + kid && React.cloneElement(kid, {key: 'z'}), +
, + ], + ); + expect(mappedChildren.length).toBe(18); + + //
+ 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'); + + //
+ 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'); + + //

+ 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); From 20fc0bd3d8bac9c3869fba55ad3cf48996bdba8e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 24 Mar 2020 20:07:20 +0000 Subject: [PATCH 2/5] Add forgotten argument This fixes the bug. --- packages/react/src/ReactChildren.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js index d2fce3f1ee3be..c03e044025952 100644 --- a/packages/react/src/ReactChildren.js +++ b/packages/react/src/ReactChildren.js @@ -111,7 +111,7 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) { 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( From 574ce5d4c4d3794ae4bef61fd7d01a17f9e55ca6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 24 Mar 2020 20:08:30 +0000 Subject: [PATCH 3/5] 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. --- packages/react/src/ReactChildren.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js index c03e044025952..65cd5470aa313 100644 --- a/packages/react/src/ReactChildren.js +++ b/packages/react/src/ReactChildren.js @@ -230,16 +230,9 @@ function mapChildren(children, func, context) { } 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; } @@ -254,7 +247,10 @@ function mapChildren(children, func, context) { */ function countChildren(children) { let n = 0; - mapChildren(children, () => n++); + mapChildren(children, () => { + n++; + // Don't return anything + }); return n; } From f66dc559599e3f4e7b786f4937a370296c55e844 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 24 Mar 2020 20:42:57 +0000 Subject: [PATCH 4/5] Flowify ReactChildren --- .../react-dom/src/client/ReactDOMOption.js | 4 +- .../src/server/ReactPartialRenderer.js | 4 +- packages/react/src/ReactChildren.js | 61 +++++++++++++------ 3 files changed, 48 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMOption.js b/packages/react-dom/src/client/ReactDOMOption.js index a06eb826767e6..fbbce1290ca39 100644 --- a/packages/react-dom/src/client/ReactDOMOption.js +++ b/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) { diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 4c3335d062125..139c2f21e6dea 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/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 && diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js index 65cd5470aa313..71ffb11ee2f04 100644 --- a/packages/react/src/ReactChildren.js +++ b/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,7 +29,7 @@ const SUBSEPARATOR = ':'; * @param {string} key to be escaped. * @return {string} the escaped key. */ -function escape(key) { +function escape(key: any): string { const escapeRegex = /[=:]/g; const escaperLookup = { '=': '=0', @@ -46,7 +50,7 @@ function escape(key) { let didWarnAboutMaps = false; const userProvidedKeyEscapeRegex = /\/+/g; -function escapeUserProvidedKey(text) { +function escapeUserProvidedKey(text: string): string { return ('' + text).replace(userProvidedKeyEscapeRegex, '$&/'); } @@ -57,7 +61,7 @@ function escapeUserProvidedKey(text) { * @param {number} index Index that is used if a manual key is not provided. * @return {string} */ -function getComponentKey(component, index) { +function getComponentKey(component: mixed, 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 ( @@ -72,7 +76,13 @@ function getComponentKey(component, index) { return index.toString(36); } -function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) { +function mapIntoArray( + children: ?ReactNodeList, + array: Array, + escapedPrefix: string, + nameSoFar: string, + callback: (?React$Node) => ?ReactNodeList, +): number { const type = typeof children; if (type === 'undefined' || type === 'boolean') { @@ -91,7 +101,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; @@ -119,8 +129,10 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) { // 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, ); @@ -151,18 +163,21 @@ function mapIntoArray(children, array, escapedPrefix, nameSoFar, callback) { } else { const iteratorFn = getIteratorFn(children); if (typeof iteratorFn === 'function') { + const iterableChildren: Iterable & { + 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,7 +189,7 @@ 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) { @@ -196,12 +211,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 +226,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,7 +241,11 @@ 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 { if (children == null) { return children; } @@ -245,7 +266,7 @@ 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++; @@ -254,6 +275,8 @@ function countChildren(children) { return n; } +type ForEachFunc = (child: ?React$Node) => void; + /** * Iterates through children that are typically specified as `props.children`. * @@ -266,7 +289,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() { @@ -283,7 +310,7 @@ function forEachChildren(children, forEachFunc, forEachContext) { * * See https://reactjs.org/docs/react-api.html#reactchildrentoarray */ -function toArray(children) { +function toArray(children: ?ReactNodeList): Array { return mapChildren(children, child => child) || []; } @@ -301,7 +328,7 @@ function toArray(children) { * @return {ReactElement} The first and only `ReactElement` contained in the * structure. */ -function onlyChild(children) { +function onlyChild(children: T): T { invariant( isValidElement(children), 'React.Children.only expected to receive a single React element child.', From 664b9369305c1aad91e860ba5f9b4bdfae71fde5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 24 Mar 2020 20:53:03 +0000 Subject: [PATCH 5/5] Revert "Rename internal fields (#18377)" This reverts commit 2ba43edc2675380a0f2222f351475bf9d750c6a9. --- packages/react-dom/src/client/ReactDOMComponentTree.js | 8 ++++---- packages/shared/ReactInstanceMap.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index b99b3cd540658..a5eae37042800 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -30,10 +30,10 @@ import {getParentSuspenseInstance} from './ReactDOMHostConfig'; const randomKey = Math.random() .toString(36) .slice(2); -const internalInstanceKey = '__reactFiber$' + randomKey; -const internalEventHandlersKey = '__reactEvents$' + randomKey; -const internalContainerInstanceKey = '__reactContainer$' + randomKey; -const internalEventListenersKey = '__reactListeners$' + randomKey; +const internalInstanceKey = '__reactInternalInstance$' + randomKey; +const internalEventHandlersKey = '__reactEventHandlers$' + randomKey; +const internalContainerInstanceKey = '__reactContainere$' + randomKey; +const internalEventListenersKey = '__reactEventListeners$' + randomKey; export function precacheFiberNode( hostInst: Fiber, diff --git a/packages/shared/ReactInstanceMap.js b/packages/shared/ReactInstanceMap.js index 863d520a1a11d..60ca4a0426efd 100644 --- a/packages/shared/ReactInstanceMap.js +++ b/packages/shared/ReactInstanceMap.js @@ -21,17 +21,17 @@ * supported we can rename it. */ export function remove(key) { - key._reactInternals = undefined; + key._reactInternalFiber = undefined; } export function get(key) { - return key._reactInternals; + return key._reactInternalFiber; } export function has(key) { - return key._reactInternals !== undefined; + return key._reactInternalFiber !== undefined; } export function set(key, value) { - key._reactInternals = value; + key._reactInternalFiber = value; }