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

Warns on access of props.key and props.ref #5744

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 57 additions & 3 deletions src/isomorphic/classic/element/ReactElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
var ReactCurrentOwner = require('ReactCurrentOwner');

var assign = require('Object.assign');
var warning = require('warning');
var canDefineProperty = require('canDefineProperty');

// The Symbol used to tag the ReactElement type. If there is no native Symbol
Expand All @@ -29,6 +30,8 @@ var RESERVED_PROPS = {
__source: true,
};

var specialPropKeyWarningShown, specialPropRefWarningShown;

/**
* Factory method to create a new React element. This no longer adheres to
* the class pattern, so do not use new to call it. Also, no instanceof check
Expand Down Expand Up @@ -123,8 +126,15 @@ ReactElement.createElement = function(type, config, children) {
var source = null;

if (config != null) {
ref = config.ref === undefined ? null : config.ref;
key = config.key === undefined ? null : '' + config.key;
if (__DEV__) {
ref = !config.hasOwnProperty('ref') ||
Object.getOwnPropertyDescriptor(config, 'ref').get ? null : config.ref;
key = !config.hasOwnProperty('key') ||
Object.getOwnPropertyDescriptor(config, 'key').get ? null : '' + config.key;
} else {
ref = config.ref === undefined ? null : config.ref;
key = config.key === undefined ? null : '' + config.key;
}
self = config.__self === undefined ? null : config.__self;
source = config.__source === undefined ? null : config.__source;
// Remaining properties are added to a new props object
Expand Down Expand Up @@ -158,7 +168,51 @@ ReactElement.createElement = function(type, config, children) {
}
}
}

if (__DEV__) {
// Create dummy `key` and `ref` property to `props` to warn users
// against its use
if (typeof props.$$typeof === 'undefined' ||
props.$$typeof !== REACT_ELEMENT_TYPE) {
if (!props.hasOwnProperty('key')) {
Object.defineProperty(props, 'key', {
get: function() {
if (!specialPropKeyWarningShown) {
specialPropKeyWarningShown = true;
warning(
false,
'%s: `key` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)',
'displayName' in type ? type.displayName: 'Element'
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's explicitly add a return statement here. This is a getter, so we should return a value, and it makes the expected behavior a little more clear. Same with the getter for ref.

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. I'll wait for @spicyj's comment on the next line note and update this PR afterwards.

return undefined;
},
configurable: true,
});
}
if (!props.hasOwnProperty('ref')) {
Object.defineProperty(props, 'ref', {
get: function() {
if (!specialPropRefWarningShown) {
specialPropRefWarningShown = true;
warning(
false,
'%s: `ref` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)',
'displayName' in type ? type.displayName: 'Element'
);
}
return undefined;
},
configurable: true,
});
}
}
}
return ReactElement(
type,
key,
Expand Down
58 changes: 58 additions & 0 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,64 @@ describe('ReactElement', function() {
expect(element.props).toEqual(expectation);
});

it('should warn when `key` is being accessed', function() {
spyOn(console, 'error');
var container = document.createElement('div');
var Child = React.createClass({
render: function() {
return <div> {this.props.key} </div>;
},
});
var Parent = React.createClass({
render: function() {
return (
<div>
<Child key="0" />
<Child key="1" />
<Child key="2" />
</div>
);
},
});
expect(console.error.calls.length).toBe(0);
ReactDOM.render(<Parent />, container);
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Child: `key` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)'
);
});

it('should warn when `ref` is being accessed', function() {
spyOn(console, 'error');
var container = document.createElement('div');
var Child = React.createClass({
render: function() {
return <div> {this.props.ref} </div>;
},
});
var Parent = React.createClass({
render: function() {
return (
<div>
<Child ref="childElement" />
</div>
);
},
});
expect(console.error.calls.length).toBe(0);
ReactDOM.render(<Parent />, container);
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Child: `ref` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)'
);
});

it('allows a string to be passed as the type', function() {
var element = React.createFactory('div')();
expect(element.type).toBe('div');
Expand Down