From 94d0dc68c89c0292cd430976a7dafb95f8d13cf4 Mon Sep 17 00:00:00 2001 From: Eric Matthys Date: Fri, 8 Apr 2016 07:46:33 -0600 Subject: [PATCH 01/10] Do not clone key and ref getters --- .../classic/element/ReactElement.js | 38 ++++++--- .../element/__tests__/ReactElement-test.js | 80 +++++++++++++++++++ 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index b0a6648870de7..fe3c6b6a2c57f 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -32,6 +32,19 @@ var RESERVED_PROPS = { var specialPropKeyWarningShown, specialPropRefWarningShown; +function isValidConfigRefOrKey(config, name) { + if (__DEV__) { + return hasOwnProperty.call(config, name) && + !Object.getOwnPropertyDescriptor(config, name).get; + } + + return config[name] !== undefined; +} + +function getConfigKey(config) { + return '' + config.key; +} + /** * 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 +151,16 @@ 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 (isValidConfigRefOrKey(config, 'ref')) { + ref = config.ref; + } + + if (isValidConfigRefOrKey(config, 'key')) { + key = getConfigKey(config); + } + self = config.__self === undefined ? null : config.__self; source = config.__source === undefined ? null : config.__source; // Remaining properties are added to a new props object @@ -297,14 +312,17 @@ ReactElement.cloneElement = function(element, config, children) { 'Properties defined in its prototype chain will be ignored.' ); } - if (config.ref !== undefined) { + + if (isValidConfigRefOrKey(config, 'ref')) { // Silently steal the ref from the parent. ref = config.ref; owner = ReactCurrentOwner.current; } - if (config.key !== undefined) { - key = '' + config.key; + + if (isValidConfigRefOrKey(config, 'key')) { + key = getConfigKey(config); } + // 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..9f2922c5dc1f0 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -170,6 +170,86 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); + it('should not extract key and ref getters from the config when creating an element', function() { + var props = { + foo: '56', + }; + + Object.defineProperty(props, 'key', { + get: function() { + return '12'; + }, + }); + + Object.defineProperty(props, 'ref', { + get: function() { + return '34'; + }, + }); + + var element = React.createFactory(ComponentClass)(props); + expect(element.type).toBe(ComponentClass); + expect(element.key).toBe(null); + expect(element.ref).toBe(null); + var expectation = {foo:'56'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + }); + + it('should not extract key and ref getters from the config when cloning an element', function() { + var element = React.createFactory(ComponentClass)({ + key: '12', + ref: '34', + foo: '56', + }); + + var props = { + foo: 'ef', + }; + + Object.defineProperty(props, 'key', { + get: function() { + return 'ab'; + }, + }); + + Object.defineProperty(props, 'ref', { + get: function() { + return 'cd'; + }, + }); + + var clone = React.cloneElement(element, props); + expect(clone.type).toBe(ComponentClass); + expect(clone.key).toBe('12'); + expect(clone.ref).toBe('34'); + var expectation = {foo:'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + + it('should allow null key and ref values when cloning an element', 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); + var expectation = {foo:'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + it('coerces the key to a string', function() { var element = React.createFactory(ComponentClass)({ key: 12, From f846edcbea793370caceb59659fcc4091a9c310f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 25 May 2016 23:17:20 +0100 Subject: [PATCH 02/10] Remove indirection when determining valid config and ref --- .../classic/element/ReactElement.js | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index fe3c6b6a2c57f..49039be3df783 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -32,17 +32,20 @@ var RESERVED_PROPS = { var specialPropKeyWarningShown, specialPropRefWarningShown; -function isValidConfigRefOrKey(config, name) { +function hasValidRef(config) { if (__DEV__) { - return hasOwnProperty.call(config, name) && - !Object.getOwnPropertyDescriptor(config, name).get; + return hasOwnProperty.call(config, 'ref') && + !Object.getOwnPropertyDescriptor(config, 'ref').get; } - - return config[name] !== undefined; + return config.ref !== undefined; } -function getConfigKey(config) { - return '' + config.key; +function hasValidKey(config) { + if (__DEV__) { + return hasOwnProperty.call(config, 'key') && + !Object.getOwnPropertyDescriptor(config, 'key').get; + } + return config.key !== undefined; } /** @@ -153,12 +156,11 @@ ReactElement.createElement = function(type, config, children) { ); } - if (isValidConfigRefOrKey(config, 'ref')) { + if (hasValidRef(config)) { ref = config.ref; } - - if (isValidConfigRefOrKey(config, 'key')) { - key = getConfigKey(config); + if (hasValidKey(config)) { + key = '' + config.key; } self = config.__self === undefined ? null : config.__self; @@ -313,14 +315,13 @@ ReactElement.cloneElement = function(element, config, children) { ); } - if (isValidConfigRefOrKey(config, 'ref')) { + if (hasValidRef(config)) { // Silently steal the ref from the parent. ref = config.ref; owner = ReactCurrentOwner.current; } - - if (isValidConfigRefOrKey(config, 'key')) { - key = getConfigKey(config); + if (hasValidKey(config)) { + key = '' + config.key; } // Remaining properties override existing props From 15cd66b91b4145e32a917c120537ed40309b68fa Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 25 May 2016 23:21:29 +0100 Subject: [PATCH 03/10] Tweak whitespace --- .../classic/element/__tests__/ReactElement-test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 9f2922c5dc1f0..85165e762ef84 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -165,7 +165,7 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); - var expectation = {foo:'56'}; + var expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); }); @@ -191,7 +191,7 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); expect(element.ref).toBe(null); - var expectation = {foo:'56'}; + var expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); }); @@ -223,7 +223,7 @@ describe('ReactElement', function() { expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); expect(clone.ref).toBe('34'); - var expectation = {foo:'ef'}; + var expectation = {foo: 'ef'}; Object.freeze(expectation); expect(clone.props).toEqual(expectation); }); @@ -245,7 +245,7 @@ describe('ReactElement', function() { expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('null'); expect(clone.ref).toBe(null); - var expectation = {foo:'ef'}; + var expectation = {foo: 'ef'}; Object.freeze(expectation); expect(clone.props).toEqual(expectation); }); @@ -258,7 +258,7 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); expect(element.ref).toBe(null); - var expectation = {foo:'56'}; + var expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); }); From 1b802fbd65cede34e4a1ed36f8f2e780ca9409e5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 25 May 2016 23:40:59 +0100 Subject: [PATCH 04/10] Add a test verifying undefined key and ref are ignored It currently fails in `createElement` because of #6879 which was introduced in #5744. It also fails in `cloneElement` because the code with that bug was extracted and shared in 94d0dc68c89c0292cd430976a7dafb95f8d13cf4. --- .../element/__tests__/ReactElement-test.js | 60 +++++++++++++++---- 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 85165e762ef84..4700c6b406cce 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -170,23 +170,49 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('should not extract key and ref getters from the config when creating an element', function() { + it('extracts null key and ref values when creating an element', 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); + var expectation = {foo: '12'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + }); + + it('ignores undefined key and ref when creating an element', 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); + var expectation = {foo: '56'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + }); + it('ignores key and ref getters when creating an element', function() { + var props = { + foo: '56', + }; Object.defineProperty(props, 'key', { get: function() { return '12'; }, }); - Object.defineProperty(props, 'ref', { get: function() { return '34'; }, }); - var element = React.createFactory(ComponentClass)(props); expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); @@ -196,29 +222,25 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('should not extract key and ref getters from the config when cloning an element', function() { + it('ignores key and ref getters when cloning an element', function() { var element = React.createFactory(ComponentClass)({ key: '12', ref: '34', foo: '56', }); - var props = { foo: 'ef', }; - Object.defineProperty(props, 'key', { get: function() { return 'ab'; }, }); - Object.defineProperty(props, 'ref', { get: function() { return 'cd'; }, }); - var clone = React.cloneElement(element, props); expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); @@ -228,19 +250,37 @@ describe('ReactElement', function() { expect(clone.props).toEqual(expectation); }); - it('should allow null key and ref values when cloning an element', function() { + it('ignores undefined key and ref values when cloning an element', 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'); + var expectation = {foo: 'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + it('extracts null key and ref values when cloning an element', 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'); From c77411b1af3abe62d35c981089dca6e4624cf44c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 00:21:16 +0100 Subject: [PATCH 05/10] Short-circuit the check just for getters This way in other cases both DEV and PROD falls through to the check for undefined. This fixes #6879 and a similar bug introduced for cloneElement() in 94d0dc68c89c0292cd430976a7dafb95f8d13cf4. --- src/isomorphic/classic/element/ReactElement.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 49039be3df783..224d4f2126c37 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -34,16 +34,20 @@ var specialPropKeyWarningShown, specialPropRefWarningShown; function hasValidRef(config) { if (__DEV__) { - return hasOwnProperty.call(config, 'ref') && - !Object.getOwnPropertyDescriptor(config, 'ref').get; + if (hasOwnProperty.call(config, 'ref') && + Object.getOwnPropertyDescriptor(config, 'ref').get) { + return false; + } } return config.ref !== undefined; } function hasValidKey(config) { if (__DEV__) { - return hasOwnProperty.call(config, 'key') && - !Object.getOwnPropertyDescriptor(config, 'key').get; + if (hasOwnProperty.call(config, 'key') && + Object.getOwnPropertyDescriptor(config, 'key').get) { + return false; + } } return config.key !== undefined; } From d9ae319821b566ec1de2a21079babe95faaafa3e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 00:27:57 +0100 Subject: [PATCH 06/10] Move tests concerned with cloning into ReactElementClone-test --- .../element/__tests__/ReactElement-test.js | 110 +---------------- .../__tests__/ReactElementClone-test.js | 112 ++++++++++++++++++ 2 files changed, 116 insertions(+), 106 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 4700c6b406cce..955902d34c71c 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -170,7 +170,7 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('extracts null key and ref values when creating an element', function() { + it('extracts null key and ref', function() { var element = React.createFactory(ComponentClass)({ key: null, ref: null, @@ -184,7 +184,7 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('ignores undefined key and ref when creating an element', function() { + it('ignores undefined key and ref', function() { var props = { foo: '56', key: undefined, @@ -199,7 +199,7 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('ignores key and ref getters when creating an element', function() { + it('ignores key and ref getters', function() { var props = { foo: '56', }; @@ -222,74 +222,6 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('ignores key and ref getters when cloning an element', function() { - var element = React.createFactory(ComponentClass)({ - key: '12', - ref: '34', - foo: '56', - }); - var props = { - foo: 'ef', - }; - Object.defineProperty(props, 'key', { - get: function() { - return 'ab'; - }, - }); - Object.defineProperty(props, 'ref', { - get: function() { - return 'cd'; - }, - }); - var clone = React.cloneElement(element, props); - expect(clone.type).toBe(ComponentClass); - expect(clone.key).toBe('12'); - expect(clone.ref).toBe('34'); - var expectation = {foo: 'ef'}; - Object.freeze(expectation); - expect(clone.props).toEqual(expectation); - }); - - it('ignores undefined key and ref values when cloning an element', 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'); - var expectation = {foo: 'ef'}; - Object.freeze(expectation); - expect(clone.props).toEqual(expectation); - }); - - it('extracts null key and ref values when cloning an element', 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); - var expectation = {foo: 'ef'}; - Object.freeze(expectation); - expect(clone.props).toEqual(expectation); - }); - it('coerces the key to a string', function() { var element = React.createFactory(ComponentClass)({ key: 12, @@ -349,18 +281,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; @@ -490,29 +410,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() { @@ -703,4 +600,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 ab57c8cae2fe4..2c05d7e0f18ae 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() { @@ -155,6 +164,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() { @@ -208,6 +229,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'); @@ -278,4 +322,72 @@ describe('ReactElementClone', function() { ); }); + it('should ignore key and ref getters', function() { + var element = React.createFactory(ComponentClass)({ + key: '12', + ref: '34', + foo: '56', + }); + var props = { + foo: 'ef', + }; + Object.defineProperty(props, 'key', { + get: function() { + return 'ab'; + }, + }); + Object.defineProperty(props, 'ref', { + get: function() { + return 'cd'; + }, + }); + var clone = React.cloneElement(element, props); + expect(clone.type).toBe(ComponentClass); + expect(clone.key).toBe('12'); + expect(clone.ref).toBe('34'); + var expectation = {foo: 'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + + 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'); + var expectation = {foo: 'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + + 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); + var expectation = {foo: 'ef'}; + Object.freeze(expectation); + expect(clone.props).toEqual(expectation); + }); + }); From a432afa750f0b59412c79c0ca9db4df42a0f8491 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 01:01:13 +0100 Subject: [PATCH 07/10] Ignore specifically React warning getter in ReactElement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t want to have different behavior in development and production. Previously, we used to ignore getters on key/ref in dev, but we’d read them in prod. Now, we only ignore the getters that we *know* we created so the production logic doesn’t differ. --- .../classic/element/ReactElement.js | 81 ++++++++++--------- .../element/__tests__/ReactElement-test.js | 26 ++---- .../__tests__/ReactElementClone-test.js | 31 ++----- 3 files changed, 55 insertions(+), 83 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 224d4f2126c37..d0c46cbbb932a 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -34,9 +34,11 @@ var specialPropKeyWarningShown, specialPropRefWarningShown; function hasValidRef(config) { if (__DEV__) { - if (hasOwnProperty.call(config, 'ref') && - Object.getOwnPropertyDescriptor(config, 'ref').get) { - return false; + if (hasOwnProperty.call(config, 'ref')) { + var getter = Object.getOwnPropertyDescriptor(config, 'ref').get; + if (getter && getter.isReactWarning) { + return false; + } } } return config.ref !== undefined; @@ -44,9 +46,11 @@ function hasValidRef(config) { function hasValidKey(config) { if (__DEV__) { - if (hasOwnProperty.call(config, 'key') && - Object.getOwnPropertyDescriptor(config, 'key').get) { - return false; + if (hasOwnProperty.call(config, 'key')) { + var getter = Object.getOwnPropertyDescriptor(config, 'key').get; + if (getter && getter.isReactWarning) { + return false; + } } } return config.key !== undefined; @@ -201,45 +205,50 @@ ReactElement.createElement = function(type, config, children) { } } if (__DEV__) { - // Create dummy `key` and `ref` property to `props` to warn users - // against its use + // 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)', + typeof type === 'function' && 'displayName' in type ? type.displayName : 'Element' + ); + } + 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)', + typeof type === 'function' && 'displayName' in type ? type.displayName : 'Element' + ); + } + 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, }); } diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 955902d34c71c..877fdb750cc9a 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -199,27 +199,11 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - it('ignores key and ref getters', function() { - var props = { - foo: '56', - }; - Object.defineProperty(props, 'key', { - get: function() { - return '12'; - }, - }); - Object.defineProperty(props, 'ref', { - get: function() { - return '34'; - }, - }); - var element = React.createFactory(ComponentClass)(props); - expect(element.type).toBe(ComponentClass); - expect(element.key).toBe(null); - expect(element.ref).toBe(null); - var expectation = {foo: '56'}; - Object.freeze(expectation); - expect(element.props).toEqual(expectation); + 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() { diff --git a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js index 2c05d7e0f18ae..187ef0bfefdb8 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js @@ -322,32 +322,11 @@ describe('ReactElementClone', function() { ); }); - it('should ignore key and ref getters', function() { - var element = React.createFactory(ComponentClass)({ - key: '12', - ref: '34', - foo: '56', - }); - var props = { - foo: 'ef', - }; - Object.defineProperty(props, 'key', { - get: function() { - return 'ab'; - }, - }); - Object.defineProperty(props, 'ref', { - get: function() { - return 'cd'; - }, - }); - var clone = React.cloneElement(element, props); - expect(clone.type).toBe(ComponentClass); - expect(clone.key).toBe('12'); - expect(clone.ref).toBe('34'); - var expectation = {foo: 'ef'}; - Object.freeze(expectation); - expect(clone.props).toEqual(expectation); + 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() { From 919eba3c99b8fd697d508d36a6e4c13a079fe7e7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 01:08:27 +0100 Subject: [PATCH 08/10] Correctly check that element and props are frozen This fixes an incorrect way of checking introduced in 95373ce769188aacd25c6485bc2c628ff02a856f (it had no effect). --- .../element/__tests__/ReactElement-test.js | 18 ++++++++++++------ .../__tests__/ReactElementClone-test.js | 6 ++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 877fdb750cc9a..ec545c8e580f4 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -53,7 +53,8 @@ describe('ReactElement', function() { expect(element.key).toBe(null); expect(element.ref).toBe(null); var expectation = {}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); @@ -121,7 +122,8 @@ describe('ReactElement', function() { expect(element.key).toBe(null); expect(element.ref).toBe(null); var expectation = {}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); @@ -166,7 +168,8 @@ describe('ReactElement', function() { expect(element.key).toBe('12'); expect(element.ref).toBe('34'); var expectation = {foo: '56'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); @@ -180,7 +183,8 @@ describe('ReactElement', function() { expect(element.key).toBe('null'); expect(element.ref).toBe(null); var expectation = {foo: '12'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); @@ -195,7 +199,8 @@ describe('ReactElement', function() { expect(element.key).toBe(null); expect(element.ref).toBe(null); var expectation = {foo: '56'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); @@ -215,7 +220,8 @@ describe('ReactElement', function() { expect(element.key).toBe('12'); expect(element.ref).toBe(null); var expectation = {foo: '56'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(element.props).toEqual(expectation); }); diff --git a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js index 187ef0bfefdb8..f11798f2092d1 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js @@ -345,7 +345,8 @@ describe('ReactElementClone', function() { expect(clone.key).toBe('12'); expect(clone.ref).toBe('34'); var expectation = {foo: 'ef'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(clone.props).toEqual(expectation); }); @@ -365,7 +366,8 @@ describe('ReactElementClone', function() { expect(clone.key).toBe('null'); expect(clone.ref).toBe(null); var expectation = {foo: 'ef'}; - Object.freeze(expectation); + expect(Object.isFrozen(element)).toBe(true); + expect(Object.isFrozen(element.props)).toBe(true); expect(clone.props).toEqual(expectation); }); From 55a0e4bf801d35cc5ff13404d7f7490f36307317 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 01:20:09 +0100 Subject: [PATCH 09/10] Infer displayName in more cases for props.ref/key warning --- .../classic/element/ReactElement.js | 8 +++- .../element/__tests__/ReactElement-test.js | 46 ++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index d0c46cbbb932a..bfeb389f75aca 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -205,6 +205,10 @@ ReactElement.createElement = function(type, config, children) { } } if (__DEV__) { + 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) { @@ -215,7 +219,7 @@ ReactElement.createElement = function(type, config, children) { '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' + displayName ); } return undefined; @@ -231,7 +235,7 @@ ReactElement.createElement = function(type, config, children) { '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' + displayName ); } return undefined; diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index ec545c8e580f4..399687e8ca95f 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -58,7 +58,7 @@ describe('ReactElement', function() { expect(element.props).toEqual(expectation); }); - 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({ @@ -88,6 +88,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'); From 58c9fda94655327a35e239a2770031f93b8e9948 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 26 May 2016 01:29:54 +0100 Subject: [PATCH 10/10] Inline expectations in tests as they are used once --- .../element/__tests__/ReactElement-test.js | 18 ++++++------------ .../__tests__/ReactElementClone-test.js | 6 ++---- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElement-test.js b/src/isomorphic/classic/element/__tests__/ReactElement-test.js index 399687e8ca95f..353989ad6b089 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElement-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElement-test.js @@ -52,10 +52,9 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); expect(element.ref).toBe(null); - var expectation = {}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({}); }); it('should warn when `key` is being accessed on createClass element', function() { @@ -165,10 +164,9 @@ describe('ReactElement', function() { expect(element.type).toBe('div'); expect(element.key).toBe(null); expect(element.ref).toBe(null); - var expectation = {}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({}); }); it('returns an immutable element', function() { @@ -211,10 +209,9 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); - var expectation = {foo: '56'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({foo: '56'}); }); it('extracts null key and ref', function() { @@ -226,10 +223,9 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('null'); expect(element.ref).toBe(null); - var expectation = {foo: '12'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({foo: '12'}); }); it('ignores undefined key and ref', function() { @@ -242,10 +238,9 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); expect(element.ref).toBe(null); - var expectation = {foo: '56'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({foo: '56'}); }); it('ignores key and ref warning getters', function() { @@ -263,10 +258,9 @@ describe('ReactElement', function() { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); expect(element.ref).toBe(null); - var expectation = {foo: '56'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(element.props).toEqual(expectation); + expect(element.props).toEqual({foo: '56'}); }); it('preserves the owner on the element', function() { diff --git a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js index f11798f2092d1..6c6621e942a41 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementClone-test.js @@ -344,10 +344,9 @@ describe('ReactElementClone', function() { expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); expect(clone.ref).toBe('34'); - var expectation = {foo: 'ef'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(clone.props).toEqual(expectation); + expect(clone.props).toEqual({foo: 'ef'}); }); it('should extract null key and ref', function() { @@ -365,10 +364,9 @@ describe('ReactElementClone', function() { expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('null'); expect(clone.ref).toBe(null); - var expectation = {foo: 'ef'}; expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); - expect(clone.props).toEqual(expectation); + expect(clone.props).toEqual({foo: 'ef'}); }); });