Skip to content
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.

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

Add component stack info to key validation warnings #6799

Merged
merged 9 commits into from
May 20, 2016
5 changes: 3 additions & 2 deletions src/isomorphic/classic/element/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,11 @@ function validateExplicitKey(element, parentType) {
warning(
false,
'Each child in an array or iterator should have a unique "key" prop.' +
'%s%s%s',
'%s%s%s%s',
addenda.parentOrOwner || '',
addenda.childOwner || '',
addenda.url || ''
addenda.url || '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't need these other addenda now since the stack should cover it. Can you remove the code for them? We still will want to dedupe the warnings based on owner though as we're currently doing.

ReactComponentTreeDevtool.getCurrentStackAddendum(element)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,37 +84,81 @@ describe('ReactElementValidator', function() {
var Anonymous = React.createClass({
displayName: undefined,
render: function() {
return <div />;
return React.createElement('div');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of changing these to not use JSX, can you just make the tests like I have in other files, replacing the filename and line number with (**)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, sorry I didn't see your regexp. Sure!

},
});

var divs = [
<div />,
<div />,
React.createElement('div'),
React.createElement('div'),
];
ReactTestUtils.renderIntoDocument(<Anonymous>{divs}</Anonymous>);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: Each child in an array or iterator should have a unique ' +
'"key" prop. See https://fb.me/react-warning-keys for more information.'
'"key" prop. See https://fb.me/react-warning-keys for more information.\n' +
' in div'
);
});

it('warns for keys for arrays of elements with no owner info', function() {
spyOn(console, 'error');

var divs = [
<div />,
<div />,
React.createElement('div'),
React.createElement('div'),
];
ReactTestUtils.renderIntoDocument(<div>{divs}</div>);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: Each child in an array or iterator should have a unique ' +
'"key" prop. Check the top-level render call using <div>. See ' +
'https://fb.me/react-warning-keys for more information.'
'https://fb.me/react-warning-keys for more information.\n' +
' in div'
);
});

it('warns for keys with component stack info', function() {
spyOn(console, 'error');

var Component = React.createClass({
render: function() {
// <div>{[<div />, <div />]}</div>
return React.createElement('div', null, [
React.createElement('div'),
React.createElement('div'),
]);
},
});

var Parent = React.createClass({
render: function() {
return React.cloneElement(this.props.child);
},
});

var GrandParent = React.createClass({
render: function() {
// <Parent child={<Component />} />
return React.createElement(Parent, {
child: React.createElement(Component),
});
},
});

ReactTestUtils.renderIntoDocument(React.createElement(GrandParent));

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: Each child in an array or iterator should have a unique ' +
'"key" prop. Check the render method of `Component`. See ' +
'https://fb.me/react-warning-keys for more information.\n' +
' in div (created by Component)\n' +
' in Component (created by GrandParent)\n' +
' in Parent (created by GrandParent)\n' +
' in GrandParent'
);
});

Expand Down
6 changes: 4 additions & 2 deletions src/renderers/shared/stack/reconciler/ReactChildReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var ReactReconciler = require('ReactReconciler');

var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool');
var instantiateReactComponent = require('instantiateReactComponent');
var KeyEscapeUtils = require('KeyEscapeUtils');
var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
Expand All @@ -27,8 +28,9 @@ function instantiateChild(childInstances, child, name) {
keyUnique,
'flattenChildren(...): Encountered two children with the same key, ' +
'`%s`. Child keys must be unique; when two children share a key, only ' +
'the first child will be used.',
KeyEscapeUtils.unescape(name)
'the first child will be used.%s',
KeyEscapeUtils.unescape(name),
ReactComponentTreeDevtool.getCurrentStackAddendum(childInstances[name])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a React element; it is an internal React component instance. This is why you got the useless "in Unknown" stack frame. I think childInstances[name]._currentElement is pretty close to what you want here, but that will actually not give you a useful stack if childInstances[name] is a text component. This error message could also be misleading if the duplicate keys are on components of different types since you'd only see one of them.

Instead, I think it probably makes sense to only show the stack to the parent component – but it is actually going to be a bit complicated to thread that information through. getCurrentStackAddendum always looks at the current owner (hence its name), but during reconciliation here we shouldn't rely on the owner being set. I'm actually surprised that it even is in your test case. Let's add a new function ReactComponentTreeDevtool.getStackAddendum(id) that takes a debug ID and prints the parents of that instance. We'll want to reuse some of the code from getCurrentStackAddendum but nothing about topElement or ReactCurrentOwner.

You'll probably want to add a parameter to ReactChildReconciler.instantiateChildren that takes the debug ID of the parent component, then pass that from ReactMultiChild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it makes sense to me. I thought the issue with Unknown was about the name of the node but I actually got confused by element vs. instance here. Thanks!

);
}
if (child != null && keyUnique) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* Copyright 2013-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @emails react-core
*/

// NOTE: We're explicitly not using JSX here. This is intended to test
// the current stack addendum without having source location added by babel.

'use strict';

var React;
var ReactTestUtils;

describe('ReactChildReconciler', function() {
beforeEach(function() {
jest.resetModuleRegistry();

React = require('React');
ReactTestUtils = require('ReactTestUtils');
});

it('warns for duplicated keys', function() {
spyOn(console, 'error');

var Component = React.createClass({
render() {
// <div>{[<div key="1" />, <div key="1" />]}</div>
return React.createElement('div', null, [
React.createElement('div', { key: '1' }),
React.createElement('div', { key: '1' }),
]);
},
});

ReactTestUtils.renderIntoDocument(<Component />);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Child keys must be unique; when two children share a key, only the first child will be used.'
);
});

it('warns for duplicated keys with component stack info', function() {
spyOn(console, 'error');

var Component = React.createClass({
render: function() {
// <div>{[<div key="1" />, <div key="1" />]}</div>
return React.createElement('div', null, [
React.createElement('div', { key: '1' }),
React.createElement('div', { key: '1' }),
]);
},
});

var Parent = React.createClass({
render: function() {
return React.cloneElement(this.props.child);
},
});

var GrandParent = React.createClass({
render: function() {
// <Parent child={<Component />} />
return React.createElement(Parent, {
child: React.createElement(Component),
});
},
});

ReactTestUtils.renderIntoDocument(React.createElement(GrandParent));

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: flattenChildren(...): ' +
'Encountered two children with the same key, `1`. ' +
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.\n' +
' in Unknown\n' + // cuz we are using `ReactTestUtils.renderIntoDocument`
Copy link
Collaborator

Choose a reason for hiding this comment

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

ReactTestUtils.renderIntoDocument shouldn't affect display names; why does it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this is unrelated to ReactTestUtils.renderIntoDocument; commenting above.

' in Component (created by GrandParent)\n' +
' in Parent (created by GrandParent)\n' +
' in GrandParent'
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -148,5 +148,60 @@ describe('ReactMultiChild', function() {
expect(mockMount.mock.calls.length).toBe(2);
expect(mockUnmount.mock.calls.length).toBe(1);
});

it('should warn for duplicated keys with component stack info', function() {
// NOTE: We're explicitly not using JSX here. This is intended to test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, let's use JSX and do a regex replace to normalize it so we can check equality.

// the current stack addendum without having source location added by babel.
spyOn(console, 'error');

var container = document.createElement('div');

var WrapperComponent = React.createClass({
render: function() {
// <div>{this.props.children}</div>
return React.createElement('div', null, this.props.children);
},
});

var Parent = React.createClass({
render: function() {
// <div><WrapperComponent>{this.props.children}</WrapperComponent></div>
return React.createElement(
'div',
null,
React.createElement(WrapperComponent, null, this.props.children)
);
},
});

ReactDOM.render(
// <Parent>{[<div key="1"/>]}</Parent>,
React.createElement(Parent, null, [
React.createElement('div', { key: '1' }),
]),
container
);

ReactDOM.render(
// <Parent>{[<div key="1"/>, <div key="1"/>]}</Parent>,
React.createElement(Parent, null, [
React.createElement('div', { key: '1' }),
React.createElement('div', { key: '1' }),
]),
container
);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: flattenChildren(...): ' +
'Encountered two children with the same key, `1`. ' +
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.\n' +
' in div\n' +
' in WrapperComponent (created by Parent)\n' +
' in div (created by Parent)\n' +
' in Parent'
);
});
});
});
6 changes: 4 additions & 2 deletions src/shared/utils/flattenChildren.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

'use strict';

var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool');
var KeyEscapeUtils = require('KeyEscapeUtils');
var traverseAllChildren = require('traverseAllChildren');
var warning = require('warning');
Expand All @@ -29,8 +30,9 @@ function flattenSingleChildIntoContext(traverseContext, child, name) {
keyUnique,
'flattenChildren(...): Encountered two children with the same key, ' +
'`%s`. Child keys must be unique; when two children share a key, only ' +
'the first child will be used.',
KeyEscapeUtils.unescape(name)
'the first child will be used.%s',
KeyEscapeUtils.unescape(name),
ReactComponentTreeDevtool.getCurrentStackAddendum(result[name])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise, you'll want to thread through a debug ID for the parent component here. It looks like there are several places that use flattenChildren so let's add a test for each one to make sure that the stack makes it through correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

);
}
if (keyUnique && child != null) {
Expand Down