From 35874606750b398a7ca6c42b92b35a6cc27cc2b3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 17 Feb 2015 11:06:54 -0800 Subject: [PATCH 1/3] Warn if accessing .type on a factory This was an important convenience as an upgrade path but shouldn't be necessary if you're using best-practice of calling createFactory in the consuming component. --- src/classic/element/ReactElementValidator.js | 27 +++++++++++++++++++ .../__tests__/ReactElementValidator-test.js | 19 +++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/classic/element/ReactElementValidator.js b/src/classic/element/ReactElementValidator.js index c4d2e314ad..23572ddb69 100644 --- a/src/classic/element/ReactElementValidator.js +++ b/src/classic/element/ReactElementValidator.js @@ -387,6 +387,33 @@ var ReactElementValidator = { ); // Legacy hook TODO: Warn if this is accessed validatedFactory.type = type; + + if (__DEV__) { + try { + Object.defineProperty( + validatedFactory, + 'type', + { + enumerable: false, + get: function() { + warning( + false, + 'Factory.type is deprecated. Access the class directly ' + + 'before passing it to createFactory.' + ); + Object.defineProperty(this, 'type', { + value: type + }); + return type; + } + } + ); + } catch (x) { + // IE will fail on defineProperty (es5-shim/sham too) + } + } + + return validatedFactory; } diff --git a/src/classic/element/__tests__/ReactElementValidator-test.js b/src/classic/element/__tests__/ReactElementValidator-test.js index a922ffbbbb..2ca5055cb9 100644 --- a/src/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/classic/element/__tests__/ReactElementValidator-test.js @@ -330,4 +330,23 @@ describe('ReactElementValidator', function() { expect(console.warn.calls[0].args[0]).toContain('use of a keyed object'); }); + it('should warn when accessing .type on an element factory', function() { + spyOn(console, 'warn'); + var TestComponent = React.createClass({ + render: function() { + return
; + } + }); + var TestFactory = React.createFactory(TestComponent); + expect(TestFactory.type).toBe(TestComponent); + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toBe( + 'Warning: Factory.type is deprecated. Access the class directly before ' + + 'passing it to createFactory.' + ); + // Warn once, not again + expect(TestFactory.type).toBe(TestComponent); + expect(console.warn.argsForCall.length).toBe(1); + }); + }); From 91194126d87c6165edf7d33ac67dd2fae86cf08f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 17 Feb 2015 11:23:12 -0800 Subject: [PATCH 2/3] Warn if using Maps as children We're not sure if this is the way we want to support this API. It creates two ways of doing things. It is convenient to avoid needing to explicitly redefine the key of Maps. However, this use case isn't as common as having an iterable where the key is on the value, not the key. --- src/utils/__tests__/traverseAllChildren-test.js | 9 +++++++++ src/utils/traverseAllChildren.js | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/utils/__tests__/traverseAllChildren-test.js b/src/utils/__tests__/traverseAllChildren-test.js index 91b51f0292..d998443b72 100644 --- a/src/utils/__tests__/traverseAllChildren-test.js +++ b/src/utils/__tests__/traverseAllChildren-test.js @@ -396,6 +396,8 @@ describe('traverseAllChildren', function() { }); it('should use keys from entry iterables', function() { + spyOn(console, 'warn'); + var threeDivEntryIterable = { '@@iterator': function() { var i = 0; @@ -445,6 +447,13 @@ describe('traverseAllChildren', function() { '.$#3:0', 2 ); + + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toContain( + 'Warning: Using Maps as children is not yet fully supported. It is an ' + + 'experimental feature that might be removed. Convert it to a sequence ' + + '/ iterable of keyed ReactElements instead.' + ); }); }); diff --git a/src/utils/traverseAllChildren.js b/src/utils/traverseAllChildren.js index da8fd53bb9..e547237a5a 100644 --- a/src/utils/traverseAllChildren.js +++ b/src/utils/traverseAllChildren.js @@ -17,6 +17,7 @@ var ReactInstanceHandles = require('ReactInstanceHandles'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); +var warning = require('warning'); var SEPARATOR = ReactInstanceHandles.SEPARATOR; var SUBSEPARATOR = ':'; @@ -34,6 +35,8 @@ var userProvidedKeyEscaperLookup = { var userProvidedKeyEscapeRegex = /[=.:]/g; +var didWarnAboutMaps = false; + function userProvidedKeyEscaper(match) { return userProvidedKeyEscaperLookup[match]; } @@ -158,6 +161,15 @@ function traverseAllChildrenImpl( ); } } else { + if (__DEV__) { + warning( + didWarnAboutMaps, + 'Using Maps as children is not yet fully supported. It is an ' + + 'experimental feature that might be removed. Convert it to a ' + + 'sequence / iterable of keyed ReactElements instead.' + ); + didWarnAboutMaps = true; + } // Iterator will provide entry [k,v] tuples rather than values. while (!(step = iterator.next()).done) { var entry = step.value; From 2490289c4adda494cc774d6a30c90de0db41db6f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 17 Feb 2015 11:49:20 -0800 Subject: [PATCH 3/3] Warn if getDOMNode or isMounted is accessed in render This is an anti-pattern that can be easily avoided by putting the logic in componentDidMount and componentDidUpdate instead. It creates a dependency on stale data inside render without enforcing a two-pass render. --- src/browser/__tests__/ReactDOM-test.js | 1 + src/browser/findDOMNode.js | 18 +++++++ src/classic/class/ReactClass.js | 16 ++++++ src/core/__tests__/ReactComponent-test.js | 20 ------- .../__tests__/ReactComponentLifeCycle-test.js | 52 ++++++++++++++++--- src/core/instantiateReactComponent.js | 1 + 6 files changed, 81 insertions(+), 27 deletions(-) diff --git a/src/browser/__tests__/ReactDOM-test.js b/src/browser/__tests__/ReactDOM-test.js index 2feace23ad..0e0e5d4e85 100644 --- a/src/browser/__tests__/ReactDOM-test.js +++ b/src/browser/__tests__/ReactDOM-test.js @@ -118,4 +118,5 @@ describe('ReactDOM', function() { expect(element.type).toBe('div'); expect(console.warn.argsForCall.length).toBe(0); }); + }); diff --git a/src/browser/findDOMNode.js b/src/browser/findDOMNode.js index 730580bf65..f7f75ef775 100644 --- a/src/browser/findDOMNode.js +++ b/src/browser/findDOMNode.js @@ -11,11 +11,14 @@ */ 'use strict'; + +var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactInstanceMap = require('ReactInstanceMap'); var ReactMount = require('ReactMount'); var invariant = require('invariant'); var isNode = require('isNode'); +var warning = require('warning'); /** * Returns the DOM node rendered by this element. @@ -24,6 +27,21 @@ var isNode = require('isNode'); * @return {DOMElement} The root node of this element. */ function findDOMNode(componentOrElement) { + if (__DEV__) { + var owner = ReactCurrentOwner.current; + if (owner !== null) { + warning( + owner._warnedAboutRefsInRender, + '%s is accessing getDOMNode or findDOMNode inside its render(). ' + + 'render() should be a pure function of props and state. It should ' + + 'never access something that requires stale data from the previous ' + + 'render, such as refs. Move this logic to componentDidMount and ' + + 'componentDidUpdate instead.', + owner.getName() || 'A component' + ); + owner._warnedAboutRefsInRender = true; + } + } if (componentOrElement == null) { return null; } diff --git a/src/classic/class/ReactClass.js b/src/classic/class/ReactClass.js index e8136d909a..db58d92806 100644 --- a/src/classic/class/ReactClass.js +++ b/src/classic/class/ReactClass.js @@ -12,6 +12,7 @@ 'use strict'; var ReactComponent = require('ReactComponent'); +var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactElement = require('ReactElement'); var ReactErrorUtils = require('ReactErrorUtils'); var ReactInstanceMap = require('ReactInstanceMap'); @@ -746,6 +747,21 @@ var ReactClassMixin = { * @final */ isMounted: function() { + if (__DEV__) { + var owner = ReactCurrentOwner.current; + if (owner !== null) { + warning( + owner._warnedAboutRefsInRender, + '%s is accessing isMounted inside its render() function. ' + + 'render() should be a pure function of props and state. It should ' + + 'never access something that requires stale data from the previous ' + + 'render, such as refs. Move this logic to componentDidMount and ' + + 'componentDidUpdate instead.', + owner.getName() || 'A component' + ); + owner._warnedAboutRefsInRender = true; + } + } var internalInstance = ReactInstanceMap.get(this); return ( internalInstance && diff --git a/src/core/__tests__/ReactComponent-test.js b/src/core/__tests__/ReactComponent-test.js index 2a2be2a974..7dd26652b6 100644 --- a/src/core/__tests__/ReactComponent-test.js +++ b/src/core/__tests__/ReactComponent-test.js @@ -254,26 +254,6 @@ describe('ReactComponent', function() { ]); }); - it('should correctly determine if a component is mounted', function() { - var Component = React.createClass({ - componentWillMount: function() { - expect(this.isMounted()).toBeFalsy(); - }, - componentDidMount: function() { - expect(this.isMounted()).toBeTruthy(); - }, - render: function() { - expect(this.isMounted()).toBeFalsy() - return
; - } - }); - - var element = ; - - var instance = ReactTestUtils.renderIntoDocument(element); - expect(instance.isMounted()).toBeTruthy(); - }); - it('fires the callback after a component is rendered', function() { var callback = mocks.getMockFunction(); var container = document.createElement('div'); diff --git a/src/core/__tests__/ReactComponentLifeCycle-test.js b/src/core/__tests__/ReactComponentLifeCycle-test.js index 929d1438d4..c665a2f24b 100644 --- a/src/core/__tests__/ReactComponentLifeCycle-test.js +++ b/src/core/__tests__/ReactComponentLifeCycle-test.js @@ -261,16 +261,54 @@ describe('ReactComponentLifeCycle', function() { ); }); - it('is not mounted inside initial render', function() { - var InitialRender = React.createClass({ + it('should correctly determine if a component is mounted', function() { + spyOn(console, 'warn'); + var Component = React.createClass({ + componentWillMount: function() { + expect(this.isMounted()).toBeFalsy(); + }, + componentDidMount: function() { + expect(this.isMounted()).toBeTruthy(); + }, render: function() { - expect(this.isMounted()).toBe(false); - return ( -
- ); + expect(this.isMounted()).toBeFalsy() + return
; } }); - ReactTestUtils.renderIntoDocument(); + + var element = ; + + var instance = ReactTestUtils.renderIntoDocument(element); + expect(instance.isMounted()).toBeTruthy(); + + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toContain( + 'Component is accessing isMounted inside its render()' + ); + }); + + it('warns if getDOMNode is used inside render', function() { + spyOn(console, 'warn'); + var Component = React.createClass({ + getInitialState: function() { + return { isMounted: false }; + }, + componentDidMount: function() { + this.setState({ isMounted: true }); + }, + render: function() { + if (this.state.isMounted) { + expect(this.getDOMNode().tagName).toBe('DIV'); + } + return
; + } + }); + + ReactTestUtils.renderIntoDocument(); + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toContain( + 'Component is accessing getDOMNode or findDOMNode inside its render()' + ); }); it('should carry through each of the phases of setup', function() { diff --git a/src/core/instantiateReactComponent.js b/src/core/instantiateReactComponent.js index ffdfbc92fd..69732ce2bb 100644 --- a/src/core/instantiateReactComponent.js +++ b/src/core/instantiateReactComponent.js @@ -116,6 +116,7 @@ function instantiateReactComponent(node, parentCompositeType) { if (__DEV__) { instance._isOwnerNecessary = false; + instance._warnedAboutRefsInRender = false; } // Internal instances should fully constructed at this point, so they should