Skip to content

Commit

Permalink
Merge pull request facebook#6880 from gaearon/clone-key-ref-props-2
Browse files Browse the repository at this point in the history 
Fix issues introduced by createElement() warning
  • Loading branch information
@gaearon
gaearon committed May 26, 2016
2 parents 21d271f + 58c9fda commit 2d74280
Show file tree
Hide file tree
Showing 3 changed files with 260 additions and 87 deletions.
114 changes: 75 additions & 39 deletions src/isomorphic/classic/element/ReactElement.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,30 @@ var RESERVED_PROPS = {

var specialPropKeyWarningShown, specialPropRefWarningShown;

function hasValidRef(config) {
if (__DEV__) {
if (hasOwnProperty.call(config, 'ref')) {
var getter = Object.getOwnPropertyDescriptor(config, 'ref').get;
if (getter && getter.isReactWarning) {
return false;
}
}
}
return config.ref !== undefined;
}

function hasValidKey(config) {
if (__DEV__) {
if (hasOwnProperty.call(config, 'key')) {
var getter = Object.getOwnPropertyDescriptor(config, 'key').get;
if (getter && getter.isReactWarning) {
return false;
}
}
}
return config.key !== undefined;
}

/**
* 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 @@ -138,14 +162,15 @@ ReactElement.createElement = function(type, config, children) {
'React.createElement(...): Expected props argument to be a plain object. ' +
'Properties defined in its prototype chain will be ignored.'
);
ref = !hasOwnProperty.call(config, 'ref') ||
Object.getOwnPropertyDescriptor(config, 'ref').get ? null : config.ref;
key = !hasOwnProperty.call(config, 'key') ||
Object.getOwnPropertyDescriptor(config, 'key').get ? null : '' + config.key;
} else {
ref = config.ref === undefined ? null : config.ref;
key = config.key === undefined ? null : '' + config.key;
}

if (hasValidRef(config)) {
ref = config.ref;
}
if (hasValidKey(config)) {
key = '' + 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 @@ -180,45 +205,54 @@ ReactElement.createElement = function(type, config, children) {
}
}
if (__DEV__) {
// Create dummy `key` and `ref` property to `props` to warn users
// against its use
var displayName = typeof type === 'function' ?
(type.displayName || type.name || 'Unknown') :
type;

// Create dummy `key` and `ref` property to `props` to warn users against its use
function warnAboutAccessingKey() {
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
);
}
return undefined;
}
warnAboutAccessingKey.isReactWarning = true;

function warnAboutAccessingRef() {
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
);
}
return undefined;
}
warnAboutAccessingRef.isReactWarning = true;

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)',
typeof type === 'function' && 'displayName' in type ? type.displayName : 'Element'
);
}
return undefined;
},
get: warnAboutAccessingKey,
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)',
typeof type === 'function' && 'displayName' in type ? type.displayName : 'Element'
);
}
return undefined;
},
get: warnAboutAccessingRef,
configurable: true,
});
}
Expand Down Expand Up @@ -297,14 +331,16 @@ ReactElement.cloneElement = function(element, config, children) {
'Properties defined in its prototype chain will be ignored.'
);
}
if (config.ref !== undefined) {

if (hasValidRef(config)) {
// Silently steal the ref from the parent.
ref = config.ref;
owner = ReactCurrentOwner.current;
}
if (config.key !== undefined) {
if (hasValidKey(config)) {
key = '' + config.key;
}

// Remaining properties override existing props
var defaultProps;
if (element.type && element.type.defaultProps) {
Expand Down
142 changes: 94 additions & 48 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
var expectation = {};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({});
});

it('should warn when `key` is being accessed', function() {
it('should warn when `key` is being accessed on createClass element', function() {
spyOn(console, 'error');
var container = document.createElement('div');
var Child = React.createClass({
Expand Down Expand Up @@ -87,6 +87,50 @@ describe('ReactElement', function() {
);
});

it('should warn when `key` is being accessed on ES class element', function() {
spyOn(console, 'error');
var container = document.createElement('div');
class Child extends React.Component {
render() {
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.count()).toBe(0);
ReactDOM.render(<Parent />, container);
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(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 `key` is being accessed on a host element', function() {
spyOn(console, 'error');
var element = <div key="3" />;
expect(console.error.calls.count()).toBe(0);
void element.props.key;
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toContain(
'div: `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');
Expand Down Expand Up @@ -120,9 +164,9 @@ describe('ReactElement', function() {
expect(element.type).toBe('div');
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
var expectation = {};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({});
});

it('returns an immutable element', function() {
Expand Down Expand Up @@ -165,9 +209,45 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe('12');
expect(element.ref).toBe('34');
var expectation = {foo:'56'};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({foo: '56'});
});

it('extracts null key and ref', function() {
var element = React.createFactory(ComponentClass)({
key: null,
ref: null,
foo: '12',
});
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe('null');
expect(element.ref).toBe(null);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({foo: '12'});
});

it('ignores undefined key and ref', function() {
var props = {
foo: '56',
key: undefined,
ref: undefined,
};
var element = React.createFactory(ComponentClass)(props);
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({foo: '56'});
});

it('ignores key and ref warning getters', function() {
var elementA = React.createElement('div');
var elementB = React.createElement('div', elementA.props);
expect(elementB.key).toBe(null);
expect(elementB.ref).toBe(null);
});

it('coerces the key to a string', function() {
Expand All @@ -178,9 +258,9 @@ describe('ReactElement', function() {
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe('12');
expect(element.ref).toBe(null);
var expectation = {foo:'56'};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
expect(Object.isFrozen(element)).toBe(true);
expect(Object.isFrozen(element.props)).toBe(true);
expect(element.props).toEqual({foo: '56'});
});

it('preserves the owner on the element', function() {
Expand Down Expand Up @@ -229,18 +309,6 @@ describe('ReactElement', function() {
expect(console.error.calls.count()).toBe(0);
});

it('overrides children if undefined is provided as an argument', function() {
var element = React.createElement(ComponentClass, {
children: 'text',
}, undefined);
expect(element.props.children).toBe(undefined);

var element2 = React.cloneElement(React.createElement(ComponentClass, {
children: 'text',
}), {}, undefined);
expect(element2.props.children).toBe(undefined);
});

it('merges rest arguments onto the children prop in an array', function() {
spyOn(console, 'error');
var a = 1;
Expand Down Expand Up @@ -370,29 +438,6 @@ describe('ReactElement', function() {
expect(inst2.props.prop).toBe(null);
});

it('should normalize props with default values in cloning', function() {
var Component = React.createClass({
getDefaultProps: function() {
return {prop: 'testKey'};
},
render: function() {
return <span />;
},
});

var instance = React.createElement(Component);
var clonedInstance = React.cloneElement(instance, {prop: undefined});
expect(clonedInstance.props.prop).toBe('testKey');
var clonedInstance2 = React.cloneElement(instance, {prop: null});
expect(clonedInstance2.props.prop).toBe(null);

var instance2 = React.createElement(Component, {prop: 'newTestKey'});
var cloneInstance3 = React.cloneElement(instance2, {prop: undefined});
expect(cloneInstance3.props.prop).toBe('testKey');
var cloneInstance4 = React.cloneElement(instance2, {});
expect(cloneInstance4.props.prop).toBe('newTestKey');
});

it('throws when changing a prop (in dev) after element creation', function() {
var Outer = React.createClass({
render: function() {
Expand Down Expand Up @@ -583,4 +628,5 @@ describe('comparing jsx vs .createFactory() vs .createElement()', function() {
expect(Child.mock.calls[0][0]).toEqual({ foo: 'foo value', children: 'children value' });
});
});

});
Loading

0 comments on commit 2d74280

Please sign in to comment.