Skip to content

Commit

Permalink
Add component stack info to key validation warnings (facebook#6799)
Browse files Browse the repository at this point in the history 
* Add component stack info to key validation warnings

* Add `ReactComponentTreeDevtool.getStackAddendumByID`
  • Loading branch information
@keyz
keyz committed May 20, 2016
1 parent 6a3e9d5 commit 47e49ae
Show file tree
Hide file tree
Showing 11 changed files with 375 additions and 101 deletions.
8 changes: 7 additions & 1 deletion src/addons/transitions/ReactTransitionChildMapping.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@ var ReactTransitionChildMapping = {
* simple syntactic sugar around flattenChildren().
*
* @param {*} children `this.props.children`
* @param {number=} selfDebugID Optional debugID of the current internal instance.
* @return {object} Mapping of key to child
*/
getChildMapping: function(children) {
getChildMapping: function(children, selfDebugID) {
if (!children) {
return children;
}

if (__DEV__) {
return flattenChildren(children, selfDebugID);
}

return flattenChildren(children);
},

Expand Down
58 changes: 46 additions & 12 deletions src/addons/transitions/ReactTransitionGroup.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

var React = require('React');
var ReactInstanceMap = require('ReactInstanceMap');
var ReactTransitionChildMapping = require('ReactTransitionChildMapping');

var emptyFunction = require('emptyFunction');
Expand All @@ -38,6 +39,7 @@ var ReactTransitionGroup = React.createClass({

getInitialState: function() {
return {
// TODO: can we get useful debug information to show at this point?
children: ReactTransitionChildMapping.getChildMapping(this.props.children),
};
},
Expand All @@ -58,9 +60,17 @@ var ReactTransitionGroup = React.createClass({
},

componentWillReceiveProps: function(nextProps) {
var nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children
);
var nextChildMapping;
if (__DEV__) {
nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children,
ReactInstanceMap.get(this)._debugID
);
} else {
nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children
);
}
var prevChildMapping = this.state.children;

this.setState({
Expand Down Expand Up @@ -123,9 +133,17 @@ var ReactTransitionGroup = React.createClass({

delete this.currentlyTransitioningKeys[key];

var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactInstanceMap.get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}

if (!currentChildMapping || !currentChildMapping.hasOwnProperty(key)) {
// This was removed before it had fully appeared. Remove it.
Expand Down Expand Up @@ -155,9 +173,17 @@ var ReactTransitionGroup = React.createClass({

delete this.currentlyTransitioningKeys[key];

var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactInstanceMap.get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}

if (!currentChildMapping || !currentChildMapping.hasOwnProperty(key)) {
// This was removed before it had fully entered. Remove it.
Expand Down Expand Up @@ -188,9 +214,17 @@ var ReactTransitionGroup = React.createClass({

delete this.currentlyTransitioningKeys[key];

var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactInstanceMap.get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}

if (currentChildMapping && currentChildMapping.hasOwnProperty(key)) {
// This entered again before it fully left. Add it again.
Expand Down
33 changes: 33 additions & 0 deletions src/addons/transitions/__tests__/ReactTransitionGroup-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ var ReactTransitionGroup;
describe('ReactTransitionGroup', function() {
var container;

function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

beforeEach(function() {
React = require('React');
ReactDOM = require('ReactDOM');
Expand Down Expand Up @@ -269,4 +273,33 @@ describe('ReactTransitionGroup', function() {
'willLeave2', 'didLeave2', 'willUnmount0', 'willUnmount1', 'willUnmount2',
]);
});

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

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

ReactDOM.render(<Component />, container);

expect(console.error.argsForCall.length).toBe(2);
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.'
);
expect(normalizeCodeLocInfo(console.error.argsForCall[1][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 ReactTransitionGroup (at **)\n' +
' in Component (at **)'
);
});
});
79 changes: 31 additions & 48 deletions src/isomorphic/classic/element/ReactElementValidator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,25 @@ var ownerHasKeyUseWarning = {};

var loggedTypeFailures = {};

function getCurrentComponentErrorInfo(parentType) {
var info = getDeclarationErrorAddendum();

if (!info) {
var parentName = typeof parentType === 'string' ?
parentType : parentType.displayName || parentType.name;
if (parentName) {
info = ` Check the top-level render call using <${parentName}>.`;
}
}
return info;
}

/**
* Warn if the element doesn't have an explicit key assigned to it.
* This element is in an array. The array could grow and shrink or be
* reordered. All children that haven't already been validated are required to
* have a "key" property assigned to it.
* have a "key" property assigned to it. Error statuses are cached so a warning
* will only be shown once.
*
* @internal
* @param {ReactElement} element Element that requires a key.
Expand All @@ -64,67 +78,36 @@ function validateExplicitKey(element, parentType) {
}
element._store.validated = true;

var addenda = getAddendaForKeyUse('uniqueKey', element, parentType);
if (addenda === null) {
// we already showed the warning
return;
}
warning(
false,
'Each child in an array or iterator should have a unique "key" prop.' +
'%s%s%s',
addenda.parentOrOwner || '',
addenda.childOwner || '',
addenda.url || ''
var memoizer = ownerHasKeyUseWarning.uniqueKey || (
ownerHasKeyUseWarning.uniqueKey = {}
);
}

/**
* Shared warning and monitoring code for the key warnings.
*
* @internal
* @param {string} messageType A key used for de-duping warnings.
* @param {ReactElement} element Component that requires a key.
* @param {*} parentType element's parent's type.
* @returns {?object} A set of addenda to use in the warning message, or null
* if the warning has already been shown before (and shouldn't be shown again).
*/
function getAddendaForKeyUse(messageType, element, parentType) {
var addendum = getDeclarationErrorAddendum();
if (!addendum) {
var parentName = typeof parentType === 'string' ?
parentType : parentType.displayName || parentType.name;
if (parentName) {
addendum = ` Check the top-level render call using <${parentName}>.`;
}
}

var memoizer = ownerHasKeyUseWarning[messageType] || (
ownerHasKeyUseWarning[messageType] = {}
);
if (memoizer[addendum]) {
return null;
var currentComponentErrorInfo = getCurrentComponentErrorInfo(parentType);
if (memoizer[currentComponentErrorInfo]) {
return;
}
memoizer[addendum] = true;

var addenda = {
parentOrOwner: addendum,
url: ' See https://fb.me/react-warning-keys for more information.',
childOwner: null,
};
memoizer[currentComponentErrorInfo] = true;

// Usually the current owner is the offender, but if it accepts children as a
// property, it may be the creator of the child that's responsible for
// assigning it a key.
var childOwner = '';
if (element &&
element._owner &&
element._owner !== ReactCurrentOwner.current) {
// Give the component that originally created this child.
addenda.childOwner =
childOwner =
` It was passed a child from ${element._owner.getName()}.`;
}

return addenda;
warning(
false,
'Each child in an array or iterator should have a unique "key" prop.' +
'%s%s See https://fb.me/react-warning-keys for more information.%s',
currentComponentErrorInfo,
childOwner,
ReactComponentTreeDevtool.getCurrentStackAddendum(element)
);
}

/**
Expand Down
49 changes: 45 additions & 4 deletions src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ var ReactDOM;
var ReactTestUtils;

describe('ReactElementValidator', function() {
function normalizeCodeLocInfo(str) {
return str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

var ComponentClass;

beforeEach(function() {
Expand Down Expand Up @@ -95,9 +99,10 @@ describe('ReactElementValidator', function() {
ReactTestUtils.renderIntoDocument(<Anonymous>{divs}</Anonymous>);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
expect(normalizeCodeLocInfo(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 (at **)'
);
});

Expand All @@ -111,10 +116,46 @@ describe('ReactElementValidator', function() {
ReactTestUtils.renderIntoDocument(<div>{divs}</div>);

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
expect(normalizeCodeLocInfo(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 (at **)'
);
});

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

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

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

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

ReactTestUtils.renderIntoDocument(<GrandParent />);

expect(console.error.argsForCall.length).toBe(1);
expect(normalizeCodeLocInfo(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 (at **)\n' +
' in Component (at **)\n' +
' in Parent (at **)\n' +
' in GrandParent (at **)'
);
});

Expand Down
Loading

0 comments on commit 47e49ae

Please sign in to comment.