Skip to content

Remove experimental support for maps as children #8936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acdlite merged 2 commits into from
Feb 7, 2017
Merged

Remove experimental support for maps as children #8936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94e8439
Remove experimental support for maps as children
acdlite Feb 6, 2017
14962f8
Continue warning if a Map is rendered as a child
acdlite Feb 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 0 additions & 3 deletions scripts/fiber/tests-passing-except-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
64 changes: 1 addition & 63 deletions src/isomorphic/children/__tests__/ReactChildren-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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, <div />], 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 = (
<div>
{threeDivEntryIterable}
</div>
);

function assertCalls() {
expect(callback.calls.count()).toBe(3);
// TODO: why
expect(callback).toHaveBeenCalledWith(<div />, 0);
expect(callback).toHaveBeenCalledWith(<div />, 1);
expect(callback).toHaveBeenCalledWith(<div />, 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([
<div key=".$#1:0" />,
<div key=".$#2:0" />,
<div key=".$#3:0" />,
]);
expectDev(console.error.calls.count()).toBe(0);
});

it('should not enumerate enumerable numbers (#4776)', () => {
/*eslint-disable no-extend-native */
Number.prototype['@@iterator'] = function() {
Expand Down
24 changes: 24 additions & 0 deletions src/renderers/shared/fiber/ReactChildFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ if (__DEV__) {
var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber');
var { getComponentName } = require('ReactFiberTreeReflection');
var warning = require('warning');
var didWarnAboutMaps = false;
}

const {
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 17 additions & 20 deletions src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div>{new Map([['foo', 0], ['bar', 1]])}</div>
);
}
it('should warn for using maps as children with owner info', () => {
spyOn(console, 'error');
class Parent extends React.Component {
render() {
return (
<div>{new Map([['foo', 0], ['bar', 1]])}</div>
);
}

var container = document.createElement('div');
ReactDOM.render(<Parent />, 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(<Parent />, 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', () => {
Expand Down
34 changes: 0 additions & 34 deletions src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,38 +220,4 @@ describe('ReactMultiChildText', () => {
ReactTestUtils.renderIntoDocument(<div><h1>{['A', 'B']}</h1></div>);
}).not.toThrow();
});

it('should reorder keyed text nodes', () => {
spyOn(console, 'error');

var container = document.createElement('div');
ReactDOM.render(
<div>{new Map([['a', 'alpha'], ['b', 'beta']])}</div>,
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(
<div>{new Map([['b', 'beta'], ['a', 'alpha']])}</div>,
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);
});
});
61 changes: 22 additions & 39 deletions src/shared/utils/traverseAllChildren.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var SUBSEPARATOR = ':';
* pattern.
*/

var didWarnAboutMaps = false;
var didWarnAboutMaps = false;

/**
* Generate a key string that identifies a component within a set.
Expand Down Expand Up @@ -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) {
Expand All @@ -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 = '';
Expand Down