diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 6e79b9ca97..6d4a6ee0d9 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -28,6 +28,17 @@ var keyMirror = require('keyMirror'); var merge = require('merge'); var mixInto = require('mixInto'); +var invokedBeforeMount = function() { + invariant( + false, + 'You have invoked a method that is automatically bound, before the ' + + 'instance has been mounted. There is nothing conceptually wrong with ' + + 'this, but since this method will be replaced with a new version once ' + + 'the component is mounted - you should be aware of the fact that this ' + + 'method will soon be replaced.' + ); +}; + /** * Policies that describe methods in `ReactCompositeComponentInterface`. */ @@ -283,6 +294,47 @@ var RESERVED_SPEC_KEYS = { } }; +function validateMethodOverride(proto, name) { + var specPolicy = ReactCompositeComponentInterface[name]; + + // Disallow overriding of base class methods unless explicitly allowed. + if (ReactCompositeComponentMixin.hasOwnProperty(name)) { + invariant( + specPolicy === SpecPolicy.OVERRIDE_BASE, + 'ReactCompositeComponentInterface: You are attempting to override ' + + '`%s` from your class specification. Ensure that your method names ' + + 'do not overlap with React methods.', + name + ); + } + + // Disallow defining methods more than once unless explicitly allowed. + if (proto.hasOwnProperty(name)) { + invariant( + specPolicy === SpecPolicy.DEFINE_MANY, + 'ReactCompositeComponentInterface: You are attempting to define ' + + '`%s` on your component more than once. This conflict may be due ' + + 'to a mixin.', + name + ); + } +} + + +function validateLifeCycleOnReplaceState(instance) { + var compositeLifeCycleState = instance._compositeLifeCycleState; + invariant( + instance.isMounted(), + 'replaceState(...): Can only update a mounted component.' + ); + invariant( + compositeLifeCycleState !== CompositeLifeCycle.RECEIVING_STATE && + compositeLifeCycleState !== CompositeLifeCycle.UNMOUNTING, + 'replaceState(...): Cannot update while unmounting component or during ' + + 'an existing state transition (such as within `render`).' + ); +} + /** * Custom version of `mixInto` which handles policy validation and reserved * specification keys when building `ReactCompositeComponent` classses. @@ -290,58 +342,44 @@ var RESERVED_SPEC_KEYS = { function mixSpecIntoComponent(Constructor, spec) { var proto = Constructor.prototype; for (var name in spec) { - if (!spec.hasOwnProperty(name)) { + var property = spec[name]; + if (!spec.hasOwnProperty(name) || !property) { continue; } - var property = spec[name]; - var specPolicy = ReactCompositeComponentInterface[name]; - - // Disallow overriding of base class methods unless explicitly allowed. - if (ReactCompositeComponentMixin.hasOwnProperty(name)) { - invariant( - specPolicy === SpecPolicy.OVERRIDE_BASE, - 'ReactCompositeComponentInterface: You are attempting to override ' + - '`%s` from your class specification. Ensure that your method names ' + - 'do not overlap with React methods.', - name - ); - } - - // Disallow using `React.autoBind` on internal methods. - if (specPolicy != null) { - invariant( - !property || !property.__reactAutoBind, - 'ReactCompositeComponentInterface: You are attempting to use ' + - '`React.autoBind` on `%s`, a method that is internal to React.' + - 'Internal methods are called with the component as the context.', - name - ); - } - - // Disallow defining methods more than once unless explicitly allowed. - if (proto.hasOwnProperty(name)) { - invariant( - specPolicy === SpecPolicy.DEFINE_MANY, - 'ReactCompositeComponentInterface: You are attempting to define ' + - '`%s` on your component more than once. This conflict may be due ' + - 'to a mixin.', - name - ); - } + validateMethodOverride(proto, name); if (RESERVED_SPEC_KEYS.hasOwnProperty(name)) { RESERVED_SPEC_KEYS[name](Constructor, property); - } else if (property && property.__reactAutoBind) { - if (!proto.__reactAutoBindMap) { - proto.__reactAutoBindMap = {}; - } - proto.__reactAutoBindMap[name] = property.__reactAutoBind; - } else if (proto.hasOwnProperty(name)) { - // For methods which are defined more than once, call the existing methods - // before calling the new property. - proto[name] = createChainedFunction(proto[name], property); } else { - proto[name] = property; + // Setup methods on prototype: + // The following member methods should not be automatically bound: + // 1. Expected ReactCompositeComponent methods (in the "interface"). + // 2. Overridden methods (that were mixed in). + var isCompositeComponentMethod = name in ReactCompositeComponentInterface; + var isInherited = name in proto; + var markedDontBind = property.__reactDontBind; + var isFunction = typeof property === 'function'; + var shouldAutoBind = + isFunction && + !isCompositeComponentMethod && + !isInherited && + !markedDontBind; + + if (shouldAutoBind) { + if (!proto.__reactAutoBindMap) { + proto.__reactAutoBindMap = {}; + } + proto.__reactAutoBindMap[name] = property; + proto[name] = invokedBeforeMount; + } else { + if (isInherited) { + // For methods which are defined more than once, call the existing + // methods before calling the new property. + proto[name] = createChainedFunction(proto[name], property); + } else { + proto[name] = property; + } + } } } } @@ -370,7 +408,26 @@ function createChainedFunction(one, two) { * `this._compositeLifeCycleState` (which can be null). * * This is different from the life cycle state maintained by `ReactComponent` in - * `this._lifeCycleState`. + * `this._lifeCycleState`. The following diagram shows how the states overlap in + * time. There are times when the CompositeLifeCycle is null - at those times it + * is only meaningful to look at ComponentLifeCycle alone. + * + * Top Row: ReactComponent.ComponentLifeCycle + * Low Row: ReactComponent.CompositeLifeCycle + * + * +-------+------------------------------------------------------+--------+ + * | UN | MOUNTED | UN | + * |MOUNTED| | MOUNTED| + * +-------+------------------------------------------------------+--------+ + * | ^--------+ +------+ +------+ +------+ +--------^ | + * | | | | | | | | | | | | + * | 0--|MOUNTING|-0-|RECEIV|-0-|RECEIV|-0-|RECEIV|-0-| UN |--->0 | + * | | | |PROPS | | PROPS| | STATE| |MOUNTING| | + * | | | | | | | | | | | | + * | | | | | | | | | | | | + * | +--------+ +------+ +------+ +------+ +--------+ | + * | | | | + * +-------+------------------------------------------------------+--------+ */ var CompositeLifeCycle = keyMirror({ /** @@ -429,11 +486,7 @@ var ReactCompositeComponentMixin = { */ mountComponent: function(rootID, transaction) { ReactComponent.Mixin.mountComponent.call(this, rootID, transaction); - - // Unset `this._lifeCycleState` until after this method is finished. - this._lifeCycleState = ReactComponent.LifeCycle.UNMOUNTED; this._compositeLifeCycleState = CompositeLifeCycle.MOUNTING; - this._processProps(this.props); if (this.__reactAutoBindMap) { @@ -459,15 +512,11 @@ var ReactCompositeComponentMixin = { // Done with mounting, `setState` will now trigger UI changes. this._compositeLifeCycleState = null; - this._lifeCycleState = ReactComponent.LifeCycle.MOUNTED; - - var html = this._renderedComponent.mountComponent(rootID, transaction); - + var markup = this._renderedComponent.mountComponent(rootID, transaction); if (this.componentDidMount) { transaction.getReactOnDOMReady().enqueue(this, this.componentDidMount); } - - return html; + return markup; }, /** @@ -551,18 +600,7 @@ var ReactCompositeComponentMixin = { */ replaceState: function(completeState) { var compositeLifeCycleState = this._compositeLifeCycleState; - invariant( - this.isMounted() || - compositeLifeCycleState === CompositeLifeCycle.MOUNTING, - 'replaceState(...): Can only update a mounted (or mounting) component.' - ); - invariant( - compositeLifeCycleState !== CompositeLifeCycle.RECEIVING_STATE && - compositeLifeCycleState !== CompositeLifeCycle.UNMOUNTING, - 'replaceState(...): Cannot update while unmounting component or during ' + - 'an existing state transition (such as within `render`).' - ); - + validateLifeCycleOnReplaceState.call(null, this); this._pendingState = completeState; // Do not trigger a state transition if we are in the middle of mounting or @@ -583,7 +621,6 @@ var ReactCompositeComponentMixin = { transaction ); ReactComponent.ReactReconcileTransaction.release(transaction); - this._compositeLifeCycleState = null; } }, @@ -777,25 +814,12 @@ var ReactCompositeComponentMixin = { */ _bindAutoBindMethod: function(method) { var component = this; - var hasWarned = false; function autoBound(a, b, c, d, e, tooMany) { invariant( typeof tooMany === 'undefined', 'React.autoBind(...): Methods can only take a maximum of 5 arguments.' ); - if (component._lifeCycleState === ReactComponent.LifeCycle.MOUNTED) { - return method.call(component, a, b, c, d, e); - } else if (!hasWarned) { - hasWarned = true; - if (__DEV__) { - console.warn( - 'React.autoBind(...): Attempted to invoke an auto-bound method ' + - 'on an unmounted instance of `%s`. You either have a memory leak ' + - 'or an event handler that is being run after unmounting.', - component.constructor.displayName || 'ReactCompositeComponent' - ); - } - } + return method.call(component, a, b, c, d, e); } return autoBound; } @@ -850,33 +874,16 @@ var ReactCompositeComponent = { }, /** - * Marks the provided method to be automatically bound to the component. - * This means the method's context will always be the component. - * - * React.createClass({ - * handleClick: React.autoBind(function() { - * this.setState({jumping: true}); - * }), - * render: function() { - * return Jump; - * } - * }); + * TODO: Delete this when all callers have been updated to rely on this + * behavior being the default. * + * Backwards compatible stub for what is now the default behavior. * @param {function} method Method to be bound. * @public */ autoBind: function(method) { - function unbound() { - invariant( - false, - 'React.autoBind(...): Attempted to invoke an auto-bound method that ' + - 'was not correctly defined on the class specification.' - ); - } - unbound.__reactAutoBind = method; - return unbound; + return method; } - }; module.exports = ReactCompositeComponent; diff --git a/src/core/ReactDoNotBindDeprecated.js b/src/core/ReactDoNotBindDeprecated.js new file mode 100644 index 0000000000..11b6c00166 --- /dev/null +++ b/src/core/ReactDoNotBindDeprecated.js @@ -0,0 +1,48 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule ReactDoNotBindDeprecated + */ + +var ReactDoNotBindDeprecated = { + /** + * Marks the method for not being automatically bound on component mounting. A + * couple of reasons you might want to use this: + * + * - Automatically supporting the previous behavior in components that were + * built with previous versions of React. + * - Tuning performance, by avoiding binding on initial render for methods + * that are always invoked while being preceded by `this.`. Such binds are + * unnecessary. + * + * React.createClass({ + * handleClick: ReactDoNotBindDeprecated.doNotBind(function() { + * alert(this.setState); // undefined! + * }), + * render: function() { + * return Jump; + * } + * }); + * + * @param {function} method Method to avoid automatically binding. + * @public + */ + doNotBind: function(method) { + method.__reactDontBind = true; // Mutating + return method; + } +}; + +module.exports = ReactDoNotBindDeprecated; diff --git a/src/core/ReactID.js b/src/core/ReactID.js index 313ccebefe..edc5a10e0e 100644 --- a/src/core/ReactID.js +++ b/src/core/ReactID.js @@ -30,7 +30,7 @@ var nodeCache = {}; * other objects so just return '' if we're given something other than a * DOM node (such as window). * - * @param {DOMElement|DOMWindow|DOMDocument} node DOM node. + * @param {?DOMElement|DOMWindow|DOMDocument|DOMTextNode} node DOM node. * @returns {string} ID of the supplied `domNode`. */ function getID(node) { diff --git a/src/core/__tests__/ReactBind-test.js b/src/core/__tests__/ReactBind-test.js index 2fa5f77c59..4d31f38ab0 100644 --- a/src/core/__tests__/ReactBind-test.js +++ b/src/core/__tests__/ReactBind-test.js @@ -21,9 +21,11 @@ var mocks = require('mocks'); var React = require('React'); +var ReactDoNotBindDeprecated = require('ReactDoNotBindDeprecated'); var ReactTestUtils = require('ReactTestUtils'); var reactComponentExpect = require('reactComponentExpect'); +// TODO: Test render and all stock methods. describe('React.autoBind', function() { it('Holds reference to instance', function() { @@ -31,16 +33,20 @@ describe('React.autoBind', function() { var mouseDidEnter = mocks.getMockFunction(); var mouseDidLeave = mocks.getMockFunction(); var mouseDidClick = mocks.getMockFunction(); - var didBadIdea = mocks.getMockFunction(); var TestBindComponent = React.createClass({ - onMouseEnter: mouseDidEnter, - onMouseLeave: mouseDidLeave, + getInitialState: function() { + return {something: 'hi'}; + }, + onMouseEnter: ReactDoNotBindDeprecated.doNotBind(mouseDidEnter), + onMouseLeave: ReactDoNotBindDeprecated.doNotBind(mouseDidLeave), onClick: React.autoBind(mouseDidClick), - // autoBind needs to be on the top-level spec. + // auto binding only occurs on top level functions in class defs. badIdeas: { - badBind: React.autoBind(didBadIdea) + badBind: function() { + this.state.something; + } }, render: function() { @@ -48,7 +54,8 @@ describe('React.autoBind', function() {
+ onClick={this.onClick} + /> ); } }); diff --git a/src/core/__tests__/ReactComponentLifeCycle-test.js b/src/core/__tests__/ReactComponentLifeCycle-test.js index 4f66eb10e2..a3c18be304 100644 --- a/src/core/__tests__/ReactComponentLifeCycle-test.js +++ b/src/core/__tests__/ReactComponentLifeCycle-test.js @@ -286,6 +286,8 @@ describe('ReactComponentLifeCycle', function() { if (isInitialRender) { this._testJournal.stateInInitialRender = clone(this.state); this._testJournal.lifeCycleInInitialRender = this._lifeCycleState; + this._testJournal.compositeLifeCycleInInitialRender = + this._compositeLifeCycleState; } else { this._testJournal.stateInLaterRender = clone(this.state); this._testJournal.lifeCycleInLaterRender = this._lifeCycleState; @@ -319,7 +321,7 @@ describe('ReactComponentLifeCycle', function() { GET_INIT_STATE_RETURN_VAL ); expect(instance._testJournal.lifeCycleAtStartOfGetInitialState) - .toBe(ComponentLifeCycle.UNMOUNTED); + .toBe(ComponentLifeCycle.MOUNTED); expect(instance._testJournal.compositeLifeCycleAtStartOfGetInitialState) .toBe(CompositeComponentLifeCycle.MOUNTING); @@ -328,7 +330,7 @@ describe('ReactComponentLifeCycle', function() { instance._testJournal.returnedFromGetInitialState ); expect(instance._testJournal.lifeCycleAtStartOfWillMount) - .toBe(ComponentLifeCycle.UNMOUNTED); + .toBe(ComponentLifeCycle.MOUNTED); expect(instance._testJournal.compositeLifeCycleAtStartOfWillMount) .toBe(CompositeComponentLifeCycle.MOUNTING); @@ -343,7 +345,10 @@ describe('ReactComponentLifeCycle', function() { expect(instance._testJournal.stateInInitialRender) .toEqual(INIT_RENDER_STATE); expect(instance._testJournal.lifeCycleInInitialRender).toBe( - ComponentLifeCycle.UNMOUNTED + ComponentLifeCycle.MOUNTED + ); + expect(instance._testJournal.compositeLifeCycleInInitialRender).toBe( + CompositeComponentLifeCycle.MOUNTING ); expect(instance._lifeCycleState).toBe(ComponentLifeCycle.MOUNTED); @@ -429,70 +434,5 @@ describe('ReactComponentLifeCycle', function() { expect(instance.state.stateField).toBe('goodbye'); }); - it('should call nested lifecycle methods in the right order', function() { - var log; - var logger = function(msg) { - return function() { - // return true for shouldComponentUpdate - log.push(msg); - return true; - }; - }; - var Outer = React.createClass({ - render: function() { - return
; - }, - componentWillMount: logger('outer componentWillMount'), - componentDidMount: logger('outer componentDidMount'), - componentWillReceiveProps: logger('outer componentWillReceiveProps'), - shouldComponentUpdate: logger('outer shouldComponentUpdate'), - componentWillUpdate: logger('outer componentWillUpdate'), - componentDidUpdate: logger('outer componentDidUpdate'), - componentWillUnmount: logger('outer componentWillUnmount') - }); - var Inner = React.createClass({ - render: function() { - return {this.props.x}; - }, - componentWillMount: logger('inner componentWillMount'), - componentDidMount: logger('inner componentDidMount'), - componentWillReceiveProps: logger('inner componentWillReceiveProps'), - shouldComponentUpdate: logger('inner shouldComponentUpdate'), - componentWillUpdate: logger('inner componentWillUpdate'), - componentDidUpdate: logger('inner componentDidUpdate'), - componentWillUnmount: logger('inner componentWillUnmount') - }); - var instance; - - log = []; - instance = ReactTestUtils.renderIntoDocument(); - expect(log).toEqual([ - 'outer componentWillMount', - 'inner componentWillMount', - 'inner componentDidMount', - 'outer componentDidMount' - ]); - - log = []; - instance.setProps({x: 42}); - expect(log).toEqual([ - 'outer componentWillReceiveProps', - 'outer shouldComponentUpdate', - 'outer componentWillUpdate', - 'inner componentWillReceiveProps', - 'inner shouldComponentUpdate', - 'inner componentWillUpdate', - 'inner componentDidUpdate', - 'outer componentDidUpdate' - ]); - - log = []; - instance.unmountComponent(); - expect(log).toEqual([ - 'outer componentWillUnmount', - 'inner componentWillUnmount' - ]); - }); - }); diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index 5a6e9602bf..d6623e9daa 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -27,6 +27,7 @@ var ReactCurrentOwner; var ReactProps; var ReactTestUtils; var ReactID; +var ReactDoNotBindDeprecated; var cx; var reactComponentExpect; @@ -38,6 +39,7 @@ describe('ReactCompositeComponent', function() { reactComponentExpect = require('reactComponentExpect'); React = require('React'); ReactCurrentOwner = require('ReactCurrentOwner'); + ReactDoNotBindDeprecated = require('ReactDoNotBindDeprecated'); ReactProps = require('ReactProps'); ReactTestUtils = require('ReactTestUtils'); ReactID = require('ReactID'); @@ -52,7 +54,7 @@ describe('ReactCompositeComponent', function() { }, render: function() { - var toggleActivatedState = this._toggleActivatedState.bind(this); + var toggleActivatedState = this._toggleActivatedState; return !this.state.activated ? : ; @@ -64,9 +66,9 @@ describe('ReactCompositeComponent', function() { return {activated: false}; }, - _toggleActivatedState: React.autoBind(function() { + _toggleActivatedState:function() { this.setState({activated: !this.state.activated}); - }), + }, render: function() { return !this.state.activated ? @@ -167,15 +169,18 @@ describe('ReactCompositeComponent', function() { }); it('should auto bind methods and values correctly', function() { - var RETURN_VALUE_AFTER_MOUNT = 'returnValue'; var ComponentClass = React.createClass({ getInitialState: function() { - return { - valueToReturn: RETURN_VALUE_AFTER_MOUNT - }; + return {valueToReturn: 'hi'}; }, - methodBoundOnMount: React.autoBind(function() { - return this.state.valueToReturn; + methodToBeExplicitlyBound: function() { + return this; + }, + methodAutoBound: function() { + return this; + }, + methodExplicitlyNotBound: ReactDoNotBindDeprecated.doNotBind(function() { + return this; }), render: function() { return
; @@ -184,18 +189,33 @@ describe('ReactCompositeComponent', function() { var instance = ; // Autobound methods will throw before mounting. + // TODO: We should actually allow component instance methods to be invoked + // before mounting for read-only operations. We would then update this test. expect(function() { - instance.methodBoundOnMount(); + instance.methodToBeExplicitlyBound.bind(instance)(); }).toThrow(); + expect(function() { + instance.methodAutoBound(); + }).toThrow(); + expect(function() { + instance.methodExplicitlyNotBound(); + }).not.toThrow(); // Next, prove that once mounted, the scope is bound correctly to the actual // component. ReactTestUtils.renderIntoDocument(instance); - var retValAfterMount = instance.methodBoundOnMount(); - expect(retValAfterMount).toBe(RETURN_VALUE_AFTER_MOUNT); - var retValAfterMountWithCrazyScope = - instance.methodBoundOnMount.call({thisIsACrazyScope:null}); - expect(retValAfterMountWithCrazyScope).toBe(RETURN_VALUE_AFTER_MOUNT); + var explicitlyBound = instance.methodToBeExplicitlyBound.bind(instance); + var autoBound = instance.methodAutoBound; + var explicitlyNotBound = instance.methodExplicitlyNotBound; + + expect(explicitlyBound.call(null)).toBe(instance); + expect(autoBound.call(null)).toBe(instance); + expect(explicitlyNotBound.call(null)).toBe(null); + + expect(explicitlyBound.call(instance)).toBe(instance); + expect(autoBound.call(instance)).toBe(instance); + expect(explicitlyNotBound.call(instance)).toBe(instance); + }); it('should normalize props with default values', function() {