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..e9661484e41 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 @@ -1513,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/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/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 6986379c0dd..50156fdbd35 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js @@ -261,28 +261,25 @@ 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]])}
- ); - } + 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`.' - ); - }); + } + 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', () => { 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..773f968322f 100644 --- a/src/shared/utils/traverseAllChildren.js +++ b/src/shared/utils/traverseAllChildren.js @@ -33,7 +33,7 @@ var SUBSEPARATOR = ':'; * pattern. */ -var didWarnAboutMaps = false; + var didWarnAboutMaps = false; /** * Generate a key string that identifies a component within a set. @@ -109,23 +109,10 @@ function traverseAllChildrenImpl( } else { var iteratorFn = getIteratorFn(children); 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 (__DEV__) { + // Warn about using Maps as children + if (iteratorFn === children.entries) { + let mapsAsChildrenAddendum = ''; if (ReactCurrentOwner.current) { var mapsAsChildrenOwnerName = ReactCurrentOwner.current.getName(); if (mapsAsChildrenOwnerName) { @@ -134,31 +121,27 @@ function traverseAllChildrenImpl( } 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', + '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; } - // 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 iterator = iteratorFn.call(children); + var step; + 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 = '';