Skip to content

Remove key-collision invariant. #1201

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

Closed
wants to merge 1 commit into from
Closed

Remove key-collision invariant. #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions src/core/ReactComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,21 @@ function validatePropertyKey(name) {
*/
function validateChildKeys(component) {
if (Array.isArray(component)) {
var keys = {};
for (var i = 0; i < component.length; i++) {
var child = component[i];
if (ReactComponent.isValidComponent(child)) {
if (child.props.key != null) {
if (keys[child.props.key]) {
var currentName = ReactCurrentOwner.current.constructor.displayName;
console.warn(
'Key collision detected for key "' + child.props.key + '". ' +
'Check the render method of ' + currentName + '.'
);
}

keys[child.props.key] = true;
}
validateExplicitKey(child);
}
}
Expand Down
36 changes: 36 additions & 0 deletions src/core/__tests__/ReactComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,23 @@
var React;
var ReactTestUtils;

var mocks;
var reactComponentExpect;
var warn;

describe('ReactComponent', function() {
beforeEach(function() {
React = require('React');
ReactTestUtils = require('ReactTestUtils');
reactComponentExpect = require('reactComponentExpect');
mocks = require('mocks');

warn = console.warn;
console.warn = mocks.getMockFunction();
});

afterEach(function() {
console.warn = warn;
});

it('should throw on invalid render targets', function() {
Expand Down Expand Up @@ -207,4 +217,30 @@ describe('ReactComponent', function() {
expect(root.refs.switcher.refs.box.refs.boxDiv._mountDepth).toBe(3);
expect(root.refs.child.refs.span._mountDepth).toBe(6);
});

it('should render even with key collisions', function() {
var Component = React.createClass({
render: function() {
var array = [
<span key="1"></span>,
<span key="2"></span>,
<span key="3"></span>,
<span key="4"></span>,
<span key="1"></span>
];

return <div>{array}</div>;
}
});

var instance = <Component />;
ReactTestUtils.renderIntoDocument(instance);

expect(console.warn.mock.calls.length).toBe(1);

expect(console.warn.mock.calls[0][0]).toBe(
'Key collision detected for key "1". ' +
'Check the render method of Component.'
);
});
});
14 changes: 0 additions & 14 deletions src/utils/__tests__/ReactChildren-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,18 +319,4 @@ describe('ReactChildren', function() {
ReactChildren.map(instance.props.children, mapFn);
}).not.toThrow();
});

it('should throw if key provided is a dupe with explicit key', function() {
var zero = <div key="something"/>;
var one = <div key="something" />;

var mapFn = function() {return null;};
var instance = (
<div>{zero}{one}</div>
);

expect(function() {
ReactChildren.map(instance.props.children, mapFn);
}).toThrow();
});
});
11 changes: 1 addition & 10 deletions src/utils/flattenChildren.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

"use strict";

var invariant = require('invariant');
var traverseAllChildren = require('traverseAllChildren');

/**
Expand All @@ -27,16 +26,8 @@ var traverseAllChildren = require('traverseAllChildren');
* @param {!string} name String name of key path to child.
*/
function flattenSingleChildIntoContext(traverseContext, child, name) {
// We found a component instance.
var result = traverseContext;
invariant(
!result.hasOwnProperty(name),
'flattenChildren(...): Encountered two children with the same key, `%s`. ' +
'Children keys must be unique.',
name
);
if (child != null) {
result[name] = child;
traverseContext[name] = child;
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/utils/traverseAllChildren.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,18 @@ function wrapUserProvidedKey(key) {
var traverseAllChildrenImpl =
function(children, nameSoFar, indexSoFar, callback, traverseContext) {
var subtreeCount = 0; // Count of children found in the current subtree.
var key;
if (Array.isArray(children)) {
for (var i = 0; i < children.length; i++) {
var child = children[i];
key = getComponentKey(child, i);
if (traverseContext && traverseContext.hasOwnProperty(key)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't rely on traverseContext being an object holding the children. In ReactChildren.mapChildren, traverseContext is a MapBookKeeping object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, my initial version used a separate map to keep track of collisions but @sebmarkbage was heavily opposed because of perf implications - would it be reasonable to force contexts to provide a getResultMap function or something like that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best approach may be to put the logic in flattenChildren, though there you only have the full key, not the parts. (Also your code here is wrong because you want to be checking for nameSoFar + (nameSoFar ? SUBSEPARATOR : SEPARATOR) + key, not just key.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In flattenChildren I have no way to reassign the key.

key = getComponentKey(null, i);
}
var nextName = (
nameSoFar +
(nameSoFar ? SUBSEPARATOR : SEPARATOR) +
getComponentKey(child, i)
key
);
var nextIndex = indexSoFar + subtreeCount;
subtreeCount += traverseAllChildrenImpl(
Expand Down Expand Up @@ -136,7 +141,7 @@ var traverseAllChildrenImpl =
'traverseAllChildren(...): Encountered an invalid child; DOM ' +
'elements are not valid children of React components.'
);
for (var key in children) {
for (key in children) {
if (children.hasOwnProperty(key)) {
subtreeCount += traverseAllChildrenImpl(
children[key],
Expand Down