From 94e843955dd4d1d8d9e6267d70681f09f0dfb96c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 6 Feb 2017 15:30:45 -0800 Subject: [PATCH 1/2] Remove experimental support for maps as children --- scripts/fiber/tests-failing.txt | 3 - scripts/fiber/tests-passing-except-dev.txt | 3 - scripts/fiber/tests-passing.txt | 1 - .../children/__tests__/ReactChildren-test.js | 64 +------------------ .../shared/__tests__/ReactMultiChild-test.js | 22 ------- .../__tests__/ReactMultiChildText-test.js | 34 ---------- src/shared/utils/traverseAllChildren.js | 61 +++--------------- 7 files changed, 11 insertions(+), 177 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 1319d7e65f4..25536706d1b 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -47,8 +47,5 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js * should carry through each of the phases of setup -src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js -* should reorder keyed text nodes - src/renderers/shared/shared/__tests__/refs-test.js * Should increase refs with an increase in divs diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index decc5427129..dab94d2b081 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -111,8 +111,5 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should warn about `setState` in getChildContext * should disallow nested render calls -src/renderers/shared/shared/__tests__/ReactMultiChild-test.js -* should warn for using maps as children with owner info - src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should warn for childContextTypes on a functional component diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 37234bda2fc..d2011467d42 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -124,7 +124,6 @@ src/isomorphic/children/__tests__/ReactChildren-test.js * should retain key across two mappings * should be called for each child in an iterable without keys * should be called for each child in an iterable with keys -* should use keys from entry iterables * should not enumerate enumerable numbers (#4776) * should allow extension of native prototypes * should pass key to returned component diff --git a/src/isomorphic/children/__tests__/ReactChildren-test.js b/src/isomorphic/children/__tests__/ReactChildren-test.js index 08758feacdb..6a2eaff7035 100644 --- a/src/isomorphic/children/__tests__/ReactChildren-test.js +++ b/src/isomorphic/children/__tests__/ReactChildren-test.js @@ -105,7 +105,7 @@ describe('ReactChildren', () => { expect(callback).toHaveBeenCalledWith(one, 1); expect(callback).toHaveBeenCalledWith(two, 2); expect(callback).toHaveBeenCalledWith(three, 3); - expect(callback).toHaveBeenCalledWith(four, 4); + expect(callback).toHaveBeenCalledWith(four, 4); callback.calls.reset(); } @@ -365,68 +365,6 @@ describe('ReactChildren', () => { ]); }); - it('should use keys from entry iterables', () => { - spyOn(console, 'error'); - - var threeDivEntryIterable = { - '@@iterator': function() { - var i = 0; - return { - next: function() { - if (i++ < 3) { - return {value: ['#' + i,
], done: false}; - } else { - return {value: undefined, done: true}; - } - }, - }; - }, - }; - threeDivEntryIterable.entries = threeDivEntryIterable['@@iterator']; - - var context = {}; - var callback = - jasmine.createSpy().and.callFake(function(kid) { - expect(this).toBe(context); - return kid; - }); - - var instance = ( -
- {threeDivEntryIterable} -
- ); - - function assertCalls() { - expect(callback.calls.count()).toBe(3); - // TODO: why - expect(callback).toHaveBeenCalledWith(
, 0); - expect(callback).toHaveBeenCalledWith(
, 1); - expect(callback).toHaveBeenCalledWith(
, 2); - - callback.calls.reset(); - } - - React.Children.forEach(instance.props.children, callback, context); - assertCalls(); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Using Maps as children is not yet fully supported. It is an ' + - 'experimental feature that might be removed. Convert it to a sequence ' + - '/ iterable of keyed ReactElements instead.' - ); - console.error.calls.reset(); - - var mappedChildren = React.Children.map(instance.props.children, callback, context); - assertCalls(); - expect(mappedChildren).toEqual([ -
, -
, -
, - ]); - expectDev(console.error.calls.count()).toBe(0); - }); - it('should not enumerate enumerable numbers (#4776)', () => { /*eslint-disable no-extend-native */ Number.prototype['@@iterator'] = function() { diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js index 6986379c0dd..83e521e6c8c 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js @@ -261,28 +261,6 @@ describe('ReactMultiChild', () => { ' in Parent (at **)' ); }); - - it('should warn for using maps as children with owner info', () => { - spyOn(console, 'error'); - - class Parent extends React.Component { - render() { - return ( -
{new Map([['foo', 0], ['bar', 1]])}
- ); - } - } - - var container = document.createElement('div'); - ReactDOM.render(, container); - - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: Using Maps as children is not yet fully supported. It is an ' + - 'experimental feature that might be removed. Convert it to a sequence ' + - '/ iterable of keyed ReactElements instead.\n\nCheck the render method of `Parent`.' - ); - }); }); it('should reorder bailed-out children', () => { diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js index 8b0a180fb19..2a63cb5e121 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js @@ -220,38 +220,4 @@ describe('ReactMultiChildText', () => { ReactTestUtils.renderIntoDocument(

{['A', 'B']}

); }).not.toThrow(); }); - - it('should reorder keyed text nodes', () => { - spyOn(console, 'error'); - - var container = document.createElement('div'); - ReactDOM.render( -
{new Map([['a', 'alpha'], ['b', 'beta']])}
, - container - ); - - var childNodes = container.firstChild.childNodes; - var alpha1 = childNodes[0]; - var alpha2 = childNodes[1]; - var alpha3 = childNodes[2]; - var beta1 = childNodes[3]; - var beta2 = childNodes[4]; - var beta3 = childNodes[5]; - - ReactDOM.render( -
{new Map([['b', 'beta'], ['a', 'alpha']])}
, - container - ); - - childNodes = container.firstChild.childNodes; - expect(childNodes[0]).toBe(beta1); - expect(childNodes[1]).toBe(beta2); - expect(childNodes[2]).toBe(beta3); - expect(childNodes[3]).toBe(alpha1); - expect(childNodes[4]).toBe(alpha2); - expect(childNodes[5]).toBe(alpha3); - - // Using Maps as children gives a single warning - expectDev(console.error.calls.count()).toBe(1); - }); }); diff --git a/src/shared/utils/traverseAllChildren.js b/src/shared/utils/traverseAllChildren.js index d2a62a458f4..b466187f5b6 100644 --- a/src/shared/utils/traverseAllChildren.js +++ b/src/shared/utils/traverseAllChildren.js @@ -17,7 +17,6 @@ var REACT_ELEMENT_TYPE = require('ReactElementSymbol'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); var KeyEscapeUtils = require('KeyEscapeUtils'); -var warning = require('warning'); var SEPARATOR = '.'; var SUBSEPARATOR = ':'; @@ -33,8 +32,6 @@ var SUBSEPARATOR = ':'; * pattern. */ -var didWarnAboutMaps = false; - /** * Generate a key string that identifies a component within a set. * @@ -111,54 +108,16 @@ function traverseAllChildrenImpl( if (iteratorFn) { var iterator = iteratorFn.call(children); var step; - if (iteratorFn !== children.entries) { - var ii = 0; - while (!(step = iterator.next()).done) { - child = step.value; - nextName = nextNamePrefix + getComponentKey(child, ii++); - subtreeCount += traverseAllChildrenImpl( - child, - nextName, - callback, - traverseContext - ); - } - } else { - if (__DEV__) { - var mapsAsChildrenAddendum = ''; - if (ReactCurrentOwner.current) { - var mapsAsChildrenOwnerName = ReactCurrentOwner.current.getName(); - if (mapsAsChildrenOwnerName) { - mapsAsChildrenAddendum = '\n\nCheck the render method of `' + mapsAsChildrenOwnerName + '`.'; - } - } - warning( - didWarnAboutMaps, - 'Using Maps as children is not yet fully supported. It is an ' + - 'experimental feature that might be removed. Convert it to a ' + - 'sequence / iterable of keyed ReactElements instead.%s', - mapsAsChildrenAddendum - ); - didWarnAboutMaps = true; - } - // Iterator will provide entry [k,v] tuples rather than values. - while (!(step = iterator.next()).done) { - var entry = step.value; - if (entry) { - child = entry[1]; - nextName = ( - nextNamePrefix + - KeyEscapeUtils.escape(entry[0]) + SUBSEPARATOR + - getComponentKey(child, 0) - ); - subtreeCount += traverseAllChildrenImpl( - child, - nextName, - callback, - traverseContext - ); - } - } + var ii = 0; + while (!(step = iterator.next()).done) { + child = step.value; + nextName = nextNamePrefix + getComponentKey(child, ii++); + subtreeCount += traverseAllChildrenImpl( + child, + nextName, + callback, + traverseContext + ); } } else if (type === 'object') { var addendum = ''; From 14962f8b40373bc5029dda8fc9c940d90321e8fa Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 6 Feb 2017 17:07:19 -0800 Subject: [PATCH 2/2] Continue warning if a Map is rendered as a child The entries of a map are a two-element array of [key, value], which React will treat as fragments with children. This is unlikely to ever be the intended behavior, so we warn. --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactChildFiber.js | 24 +++++++++++++++++++ .../shared/__tests__/ReactMultiChild-test.js | 19 +++++++++++++++ src/shared/utils/traverseAllChildren.js | 24 +++++++++++++++++++ 4 files changed, 68 insertions(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index d2011467d42..e9661484e41 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1512,6 +1512,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js * should replace children with different keys * should warn for duplicated array keys with component stack info * should warn for duplicated iterable keys with component stack info +* should warn for using maps as children with owner info * should reorder bailed-out children * prepares new children before unmounting old diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index bc0139c88fc..6aa4456afdd 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -42,6 +42,7 @@ if (__DEV__) { var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber'); var { getComponentName } = require('ReactFiberTreeReflection'); var warning = require('warning'); + var didWarnAboutMaps = false; } const { @@ -802,6 +803,29 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } if (__DEV__) { + // Warn about using Maps as children + if (typeof newChildrenIterable.entries === 'function') { + const possibleMap = (newChildrenIterable : any); + if (possibleMap.entries === iteratorFn) { + let mapsAsChildrenAddendum = ''; + const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; + if (owner && typeof owner.tag === 'number') { + const mapsAsChildrenOwnerName = getComponentName((owner : any)); + if (mapsAsChildrenOwnerName) { + mapsAsChildrenAddendum = '\n\nCheck the render method of `' + mapsAsChildrenOwnerName + '`.'; + } + } + warning( + didWarnAboutMaps, + 'Using Maps as children is unsupported and will likely yield ' + + 'unexpected results. Convert it to a sequence/iterable of keyed ' + + 'ReactElements instead.%s', + mapsAsChildrenAddendum + ); + didWarnAboutMaps = true; + } + } + // First, validate keys. // We'll get a different iterator later for the main pass. const newChildren = iteratorFn.call(newChildrenIterable); diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js index 83e521e6c8c..50156fdbd35 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js @@ -263,6 +263,25 @@ describe('ReactMultiChild', () => { }); }); + it('should warn for using maps as children with owner info', () => { + spyOn(console, 'error'); + class Parent extends React.Component { + render() { + return ( +
{new Map([['foo', 0], ['bar', 1]])}
+ ); + } + } + var container = document.createElement('div'); + ReactDOM.render(, container); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: Using Maps as children is unsupported and will likely yield ' + + 'unexpected results. Convert it to a sequence/iterable of keyed ' + + 'ReactElements instead.\n\nCheck the render method of `Parent`.' + ); + }); + it('should reorder bailed-out children', () => { spyOn(console, 'error'); diff --git a/src/shared/utils/traverseAllChildren.js b/src/shared/utils/traverseAllChildren.js index b466187f5b6..773f968322f 100644 --- a/src/shared/utils/traverseAllChildren.js +++ b/src/shared/utils/traverseAllChildren.js @@ -17,6 +17,7 @@ var REACT_ELEMENT_TYPE = require('ReactElementSymbol'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); var KeyEscapeUtils = require('KeyEscapeUtils'); +var warning = require('warning'); var SEPARATOR = '.'; var SUBSEPARATOR = ':'; @@ -32,6 +33,8 @@ var SUBSEPARATOR = ':'; * pattern. */ + var didWarnAboutMaps = false; + /** * Generate a key string that identifies a component within a set. * @@ -106,6 +109,27 @@ function traverseAllChildrenImpl( } else { var iteratorFn = getIteratorFn(children); if (iteratorFn) { + if (__DEV__) { + // Warn about using Maps as children + if (iteratorFn === children.entries) { + let mapsAsChildrenAddendum = ''; + if (ReactCurrentOwner.current) { + var mapsAsChildrenOwnerName = ReactCurrentOwner.current.getName(); + if (mapsAsChildrenOwnerName) { + mapsAsChildrenAddendum = '\n\nCheck the render method of `' + mapsAsChildrenOwnerName + '`.'; + } + } + warning( + didWarnAboutMaps, + 'Using Maps as children is unsupported and will likely yield ' + + 'unexpected results. Convert it to a sequence/iterable of keyed ' + + 'ReactElements instead.%s', + mapsAsChildrenAddendum + ); + didWarnAboutMaps = true; + } + } + var iterator = iteratorFn.call(children); var step; var ii = 0;