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/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 5781db76ad..2ee825da0c 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); + }); + }); 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 diff --git a/src/utils/__tests__/traverseAllChildren-test.js b/src/utils/__tests__/traverseAllChildren-test.js index 84feb171f7..0d940c062f 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;