Skip to content

Warn better for key issues #1351

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
zpao wants to merge 1 commit into from
Closed

Warn better for key issues #1351

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
144 changes: 80 additions & 64 deletions src/core/ReactDescriptor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,23 @@ var warning = require('warning');
* object keys are not valid. This allows us to keep track of children between
* updates.
*/
var ownerHasExplicitKeyWarning = {};
var ownerHasPropertyWarning = {};
var ownerHasKeyUseWarning = {
'react_key_warning': {},
'react_numeric_key_warning': {}
};
var ownerHasMonitoredObjectMap = {};

var NUMERIC_PROPERTY_REGEX = /^\d+$/;

/**
* Gets the current owner's displayName for use in warnings.
* @return {?string} Display name or undefined
*/
function getCurrentOwnerDisplayName() {
var current = ReactCurrentOwner.current;
return current && current.constructor.displayName || undefined;
}

/**
* Warn if the component doesn't have an explicit key assigned to it.
* This component is in an array. The array could grow and shrink or be
Expand All @@ -43,92 +54,96 @@ var NUMERIC_PROPERTY_REGEX = /^\d+$/;
*
* @internal
* @param {ReactComponent} component Component that requires a key.
* @param {*} parentType component's parent's type.
*/
function validateExplicitKey(component) {
function validateExplicitKey(component, parentType) {
if (component._store.validated || component.props.key != null) {
return;
}
component._store.validated = true;

// We can't provide friendly warnings for top level components.
if (!ReactCurrentOwner.current) {
warnAndMonitorForKeyUse(
'react_key_warning',
'Each child in an array should have a unique "key" prop.',
component,
parentType
);
}

/**
* Warn if the key is being defined as an object property but has an incorrect
* value.
*
* @internal
* @param {string} name Property name of the key.
* @param {ReactComponent} component Component that requires a key.
* @param {*} parentType component's parent's type.
*/
function validatePropertyKey(name, component, parentType) {
if (!NUMERIC_PROPERTY_REGEX.test(name)) {
return;
}
warnAndMonitorForKeyUse(
'react_numeric_key_warning',
'Child objects should have non-numeric keys so ordering is preserved.',
component,
parentType
);
}

/**
* Shared warning and monitoring code for the key warnings.
*
* @internal
* @param {string} warningID The id used when logging.
* @param {string} message The base warning that gets output.
* @param {ReactComponent} component Component that requires a key.
* @param {*} parentType component's parent's type.
*/
function warnAndMonitorForKeyUse(warningID, message, component, parentType) {
var ownerName = getCurrentOwnerDisplayName();
var parentName = parentType.displayName;

// Name of the component whose render method tried to pass children.
var currentName = ReactCurrentOwner.current.constructor.displayName;
if (ownerHasExplicitKeyWarning.hasOwnProperty(currentName)) {
var useName = ownerName || parentName;
var memoizer = ownerHasKeyUseWarning[warningID];
if (memoizer.hasOwnProperty(useName)) {
return;
}
ownerHasExplicitKeyWarning[currentName] = true;
memoizer[useName] = true;

var message = 'Each child in an array should have a unique "key" prop. ' +
'Check the render method of ' + currentName + '.';
message += ownerName ?
` Check the render method of ${ownerName}.` :
` Check the renderComponent call using <${parentName}>.`;

// 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 childOwnerName = null;
if (component._owner !== ReactCurrentOwner.current) {
if (component._owner && component._owner !== ReactCurrentOwner.current) {
// Name of the component that originally created this child.
childOwnerName =
component._owner &&
component._owner.constructor.displayName;

// 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.
message += ' It was passed a child from ' + childOwnerName + '.';
childOwnerName = component._owner.constructor.displayName;

message += ` It was passed a child from ${childOwnerName}.`;
}

message += ' See http://fb.me/react-warning-keys for more information.';
monitorCodeUse('react_key_warning', {
component: currentName,
monitorCodeUse(warningID, {
component: useName,
componentOwner: childOwnerName
});
console.warn(message);
}

/**
* Warn if the key is being defined as an object property but has an incorrect
* value.
*
* @internal
* @param {string} name Property name of the key.
* @param {ReactComponent} component Component that requires a key.
*/
function validatePropertyKey(name) {
// We can't provide friendly warnings for top level components.
if (!ReactCurrentOwner.current) {
return;
}

if (NUMERIC_PROPERTY_REGEX.test(name)) {
// Name of the component whose render method tried to pass children.
var currentName = ReactCurrentOwner.current.constructor.displayName;
if (ownerHasPropertyWarning.hasOwnProperty(currentName)) {
return;
}
ownerHasPropertyWarning[currentName] = true;

monitorCodeUse('react_numeric_key_warning');
console.warn(
'Child objects should have non-numeric keys so ordering is preserved. ' +
'Check the render method of ' + currentName + '. ' +
'See http://fb.me/react-warning-keys for more information.'
);
}
}

/**
* Log that we're using an object map. We're considering deprecating this
* feature and replace it with proper Map and ImmutableMap data structures.
*
* @internal
* @param {object} displayNames Contains strings with displayNames for possible
* owner components.
*/
function monitorUseOfObjectMap() {
// Name of the component whose render method tried to pass children.
// We only use this to avoid spewing the logs. We lose additional
// owner stacks but hopefully one level is enough to trace the source.
var currentName = (ReactCurrentOwner.current &&
ReactCurrentOwner.current.constructor.displayName) || '';
function monitorUseOfObjectMap(displayNames) {
var currentName = getCurrentOwnerDisplayName() || '';
if (ownerHasMonitoredObjectMap.hasOwnProperty(currentName)) {
return;
}
Expand All @@ -143,14 +158,15 @@ function monitorUseOfObjectMap() {
*
* @internal
* @param {*} component Statically passed child of any type.
* @param {*} parentType component's parent's type.
* @return {boolean}
*/
function validateChildKeys(component) {
function validateChildKeys(component, parentType) {
if (Array.isArray(component)) {
for (var i = 0; i < component.length; i++) {
var child = component[i];
if (ReactDescriptor.isValidDescriptor(child)) {
validateExplicitKey(child);
validateExplicitKey(child, parentType);
}
}
} else if (ReactDescriptor.isValidDescriptor(component)) {
Expand All @@ -159,7 +175,7 @@ function validateChildKeys(component) {
} else if (component && typeof component === 'object') {
monitorUseOfObjectMap();
for (var name in component) {
validatePropertyKey(name, component);
validatePropertyKey(name, component[name], parentType);
}
}
}
Expand Down Expand Up @@ -247,14 +263,14 @@ ReactDescriptor.createFactory = function(type) {
var childrenLength = arguments.length - 1;
if (childrenLength === 1) {
if (__DEV__) {
validateChildKeys(children);
validateChildKeys(children, type);
}
props.children = children;
} else if (childrenLength > 1) {
var childArray = Array(childrenLength);
for (var i = 0; i < childrenLength; i++) {
if (__DEV__) {
validateChildKeys(arguments[i + 1]);
validateChildKeys(arguments[i + 1], type);
}
childArray[i] = arguments[i + 1];
}
Expand Down