diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 64ac67125d38b..4e815449247af 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -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 @@ -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 @@ -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, }); } @@ -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) { diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 67bad42f3e31f..353989ad6b089 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -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({ @@ -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
{this.props.key}
; + } + } + var Parent = React.createClass({ + render: function() { + return ( +
+ + + +
+ ); + }, + }); + expect(console.error.calls.count()).toBe(0); + ReactDOM.render(, 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 =
; + 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'); @@ -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() { @@ -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() { @@ -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() { @@ -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; @@ -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 ; - }, - }); - - 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() { @@ -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' }); }); }); + }); diff --git a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js index 1414bdce34f84..df62052aeedda 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js @@ -16,11 +16,20 @@ var ReactDOM; var ReactTestUtils; describe('ReactElementClone', function() { + var ComponentClass; beforeEach(function() { React = require('React'); ReactDOM = require('ReactDOM'); ReactTestUtils = require('ReactTestUtils'); + + // NOTE: We're explicitly not using JSX here. This is intended to test + // classic JS without JSX. + ComponentClass = React.createClass({ + render: function() { + return React.createElement('div'); + }, + }); }); it('should clone a DOM component with new props', function() { @@ -160,6 +169,18 @@ describe('ReactElementClone', function() { ]); }); + it('should override 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('should support keys and refs', function() { var Parent = React.createClass({ render: function() { @@ -213,6 +234,29 @@ describe('ReactElementClone', function() { ); }); + it('should normalize props with default values', function() { + var Component = React.createClass({ + getDefaultProps: function() { + return {prop: 'testKey'}; + }, + render: function() { + return ; + }, + }); + + 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('warns for keys for arrays of elements in rest args', function() { spyOn(console, 'error'); @@ -283,4 +327,51 @@ describe('ReactElementClone', function() { ); }); + it('should ignore key and ref warning getters', function() { + var elementA = React.createElement('div'); + var elementB = React.cloneElement(elementA, elementA.props); + expect(elementB.key).toBe(null); + expect(elementB.ref).toBe(null); + }); + + it('should ignore undefined key and ref', function() { + var element = React.createFactory(ComponentClass)({ + key: '12', + ref: '34', + foo: '56', + }); + var props = { + key: undefined, + ref: undefined, + foo: 'ef', + }; + var clone = React.cloneElement(element, props); + expect(clone.type).toBe(ComponentClass); + expect(clone.key).toBe('12'); + expect(clone.ref).toBe('34'); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); + expect(clone.props).toEqual({foo: 'ef'}); + }); + + it('should extract null key and ref', function() { + var element = React.createFactory(ComponentClass)({ + key: '12', + ref: '34', + foo: '56', + }); + var props = { + key: null, + ref: null, + foo: 'ef', + }; + var clone = React.cloneElement(element, props); + expect(clone.type).toBe(ComponentClass); + expect(clone.key).toBe('null'); + expect(clone.ref).toBe(null); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); + expect(clone.props).toEqual({foo: 'ef'}); + }); + });