diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 3202e4c17ed0b..7747b800e7db8 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -161,8 +161,6 @@ var ReactCompositeComponentMixin = assign({}, context ); - ReactRef.attachRefs(this, this._currentElement); - this._context = context; this._mountOrder = nextMountID++; this._rootNodeID = rootID; @@ -254,9 +252,18 @@ var ReactCompositeComponentMixin = assign({}, if (inst.componentDidMount) { transaction.getReactMountReady().enqueue(inst.componentDidMount, inst); } + transaction.getReactMountReady().enqueue(this.attachRefs, this); return markup; }, + /** + * Helper to call ReactRef.attachRefs with this composite component, split out + * to avoid allocations in the transaction mount-ready queue. + */ + attachRefs: function() { + ReactRef.attachRefs(this, this._currentElement); + }, + /** * Releases any resources allocated by `mountComponent`. * @@ -266,6 +273,8 @@ var ReactCompositeComponentMixin = assign({}, unmountComponent: function() { var inst = this._instance; + ReactRef.detachRefs(this, this._currentElement); + this._compositeLifeCycleState = CompositeLifeCycle.UNMOUNTING; if (inst.componentWillUnmount) { inst.componentWillUnmount(); @@ -281,8 +290,6 @@ var ReactCompositeComponentMixin = assign({}, this._pendingCallbacks = null; this._pendingElement = null; - ReactRef.detachRefs(this, this._currentElement); - ReactComponent.Mixin.unmountComponent.call(this); ReactComponentEnvironment.unmountIDFromEnvironment(this._rootNodeID); @@ -728,15 +735,23 @@ var ReactCompositeComponentMixin = assign({}, nextUnmaskedContext ); - // Update refs regardless of what shouldComponentUpdate returns - ReactRef.updateRefs(this, prevParentElement, nextParentElement); - var inst = this._instance; var prevContext = inst.context; var prevProps = inst.props; var nextContext = prevContext; var nextProps = prevProps; + + var refsChanged = ReactRef.shouldUpdateRefs( + this, + prevParentElement, + nextParentElement + ); + + if (refsChanged) { + ReactRef.detachRefs(this, prevParentElement); + } + // Distinguish between a props update versus a simple state update if (prevParentElement !== nextParentElement) { nextContext = this._processContext(nextParentElement._context); @@ -774,7 +789,18 @@ var ReactCompositeComponentMixin = assign({}, } } - if (!shouldUpdate) { + if (shouldUpdate) { + this._pendingForceUpdate = false; + // Will set `this.props`, `this.state` and `this.context`. + this._performComponentUpdate( + nextParentElement, + nextProps, + nextState, + nextContext, + transaction, + nextUnmaskedContext + ); + } else { // If it's determined that a component should not update, we still want // to set props and state but we shortcut the rest of the update. this._currentElement = nextParentElement; @@ -782,19 +808,12 @@ var ReactCompositeComponentMixin = assign({}, inst.props = nextProps; inst.state = nextState; inst.context = nextContext; - return; } - this._pendingForceUpdate = false; - // Will set `this.props`, `this.state` and `this.context`. - this._performComponentUpdate( - nextParentElement, - nextProps, - nextState, - nextContext, - transaction, - nextUnmaskedContext - ); + // Update refs regardless of what shouldComponentUpdate returns + if (refsChanged) { + transaction.getReactMountReady().enqueue(this.attachRefs, this); + } }, /** @@ -819,6 +838,7 @@ var ReactCompositeComponentMixin = assign({}, ) { var inst = this._instance; + var prevElement = this._currentElement; var prevProps = inst.props; var prevState = inst.state; var prevContext = inst.context; diff --git a/src/core/ReactRef.js b/src/core/ReactRef.js index 42fb1c2f07223..7d3665670b355 100644 --- a/src/core/ReactRef.js +++ b/src/core/ReactRef.js @@ -14,98 +14,22 @@ var ReactOwner = require('ReactOwner'); var ReactUpdates = require('ReactUpdates'); -var accumulate = require('accumulate'); -var assign = require('Object.assign'); -var forEachAccumulated = require('forEachAccumulated'); -var invariant = require('invariant'); - -function ReactRef() { - this._value = null; - this._successCallbacks = null; - this._failureCallbacks = null; -} - -/** - * Call the enqueued success or failure callbacks for a ref, as appropriate. - */ -function dispatchCallbacks() { - /*jshint validthis:true */ - var successCallbacks = this._successCallbacks; - var failureCallbacks = this._failureCallbacks; - this._successCallbacks = null; - this._failureCallbacks = null; - - if (this._value) { - forEachAccumulated(successCallbacks, callSuccess, this); - } else { - forEachAccumulated(failureCallbacks, callFailure); - } -} - -/** - * Call a single success callback, passing the ref's value. - */ -function callSuccess(cb) { - /*jshint validthis:true */ - cb(this._value); -} - -/** - * Call a single failure callback, passing no arguments. - */ -function callFailure(cb) { - cb(); -} - -assign(ReactRef.prototype, { - /** - * Get the value of a ref asynchronously. Accepts a success callback and an - * optional failure callback. If the ref has been rendered, the success - * callback will be called with the component instance; otherwise, the failure - * callback will be executed. - * - * @param {function} success Callback in case of success - * @param {?function} failure Callback in case of failure - */ - then: function(success, failure) { - invariant( - typeof success === 'function', - 'ReactRef.then(...): Must provide a success callback.' - ); - if (this._successCallbacks == null) { - ReactUpdates.asap(dispatchCallbacks, this); - } - this._successCallbacks = accumulate(this._successCallbacks, success); - if (failure) { - this._failureCallbacks = accumulate(this._failureCallbacks, failure); - } - } -}); - -function attachFirstClassRef(ref, value) { - ref._value = value.getPublicInstance(); -} - -function detachFirstClassRef(ref, value) { - // Check that `component` is still the current ref because we do not want to - // detach the ref if another component stole it. - if (ref._value === value) { - ref._value = null; - } -} +var ReactRef = {}; function attachRef(ref, component, owner) { - if (ref instanceof ReactRef) { - attachFirstClassRef(ref, component); + if (typeof ref === 'function') { + ref(component.getPublicInstance()); } else { + // Legacy ref ReactOwner.addComponentAsRefTo(component, ref, owner); } } function detachRef(ref, component, owner) { - if (ref instanceof ReactRef) { - detachFirstClassRef(ref, component); + if (typeof ref === 'function') { + ref(null); } else { + // Legacy ref ReactOwner.removeComponentAsRefFrom(component, ref, owner); } } @@ -117,7 +41,7 @@ ReactRef.attachRefs = function(instance, element) { } }; -ReactRef.updateRefs = function(instance, prevElement, nextElement) { +ReactRef.shouldUpdateRefs = function(instance, prevElement, nextElement) { // If either the owner or a `ref` has changed, make sure the newest owner // has stored a reference to `this`, and the previous owner (if different) // has forgotten the reference to `this`. We use the element instead @@ -130,16 +54,10 @@ ReactRef.updateRefs = function(instance, prevElement, nextElement) { // is made. It probably belongs where the key checking and // instantiateReactComponent is done. - if (nextElement._owner !== prevElement._owner || - nextElement.ref !== prevElement.ref) { - if (prevElement.ref != null) { - detachRef(prevElement.ref, instance, prevElement._owner); - } - // Correct, even if the owner is the same, and only the ref has changed. - if (nextElement.ref != null) { - attachRef(nextElement.ref, instance, nextElement._owner); - } - } + return ( + nextElement._owner !== prevElement._owner || + nextElement.ref !== prevElement.ref + ); }; ReactRef.detachRefs = function(instance, element) { diff --git a/src/core/__tests__/ReactComponent-test.js b/src/core/__tests__/ReactComponent-test.js index 67b9c344ae000..5f8c0f6c66e02 100644 --- a/src/core/__tests__/ReactComponent-test.js +++ b/src/core/__tests__/ReactComponent-test.js @@ -112,50 +112,27 @@ describe('ReactComponent', function() { } }); - var refsResolved = 0; - var refsErrored = 0; + var mounted = false; var Component = React.createClass({ - componentWillMount: function() { - this.innerRef = React.createRef(); - this.outerRef = React.createRef(); - this.unusedRef = React.createRef(); - }, render: function() { - var inner = ; + var inner = this.innerRef = c} />; var outer = ( - + this.outerRef = c}> {inner} ); return outer; }, componentDidMount: function() { - // TODO: Currently new refs aren't available on initial render - }, - componentDidUpdate: function() { - this.innerRef.then(function(inner) { - expect(inner.getObject()).toEqual(innerObj); - refsResolved++; - }); - this.outerRef.then(function(outer) { - expect(outer.getObject()).toEqual(outerObj); - refsResolved++; - }); - this.unusedRef.then(function() { - throw new Error("Unused ref should not be resolved"); - }, function() { - refsErrored++; - }); - expect(refsResolved).toBe(0); - expect(refsErrored).toBe(0); + expect(this.innerRef.getObject()).toEqual(innerObj); + expect(this.outerRef.getObject()).toEqual(outerObj); + mounted = true; } }); var instance = ; instance = ReactTestUtils.renderIntoDocument(instance); - instance.forceUpdate(); - expect(refsResolved).toBe(2); - expect(refsErrored).toBe(1); + expect(mounted).toBe(true); }); it('should support new-style refs with mixed-up owners', function() { @@ -165,47 +142,114 @@ describe('ReactComponent', function() { } }); - var refsResolved = 0; + var mounted = false; var Component = React.createClass({ - componentWillMount: function() { - this.wrapperRef = React.createRef(); - this.innerRef = React.createRef(); - }, getInner: function() { // (With old-style refs, it's impossible to get a ref to this div // because Wrapper is the current owner when this function is called.) - return
; + return
this.innerRef = c} />; }, render: function() { return ( this.wrapperRef = c} getContent={this.getInner} /> ); }, componentDidMount: function() { - // TODO: Currently new refs aren't available on initial render - }, - componentDidUpdate: function() { // Check .props.title to make sure we got the right elements back - this.wrapperRef.then(function(wrapper) { - expect(wrapper.props.title).toBe("wrapper"); - refsResolved++; - }); - this.innerRef.then(function(inner) { - expect(inner.props.title).toEqual("inner"); - refsResolved++; - }); - expect(refsResolved).toBe(0); + expect(this.wrapperRef.props.title).toBe('wrapper'); + expect(this.innerRef.props.title).toBe('inner'); + mounted = true; } }); var instance = ; instance = ReactTestUtils.renderIntoDocument(instance); - instance.forceUpdate(); - expect(refsResolved).toBe(2); + expect(mounted).toBe(true); + }); + + it('should call refs at the correct time', function() { + var log = []; + + var Inner = React.createClass({ + render: function() { + log.push(`inner ${this.props.id} render`); + return
; + }, + componentDidMount: function() { + log.push(`inner ${this.props.id} componentDidMount`); + }, + componentDidUpdate: function() { + log.push(`inner ${this.props.id} componentDidUpdate`); + }, + componentWillUnmount: function() { + log.push(`inner ${this.props.id} componentWillUnmount`); + } + }); + + var Outer = React.createClass({ + render: function() { + return ( +
+ { + log.push(`ref 1 got ${c ? `instance ${c.props.id}` : 'null'}`); + }}/> + { + log.push(`ref 2 got ${c ? `instance ${c.props.id}` : 'null'}`); + }}/> +
+ ); + }, + componentDidMount: function() { + log.push('outer componentDidMount'); + }, + componentDidUpdate: function() { + log.push('outer componentDidUpdate'); + }, + componentWillUnmount: function() { + log.push('outer componentWillUnmount'); + } + }); + + // mount, update, unmount + var el = document.createElement('div'); + log.push('start mount'); + React.render(, el); + log.push('start update'); + React.render(, el); + log.push('start unmount'); + React.unmountComponentAtNode(el); + + expect(log).toEqual([ + 'start mount', + 'inner 1 render', + 'inner 2 render', + 'inner 1 componentDidMount', + 'ref 1 got instance 1', + 'inner 2 componentDidMount', + 'ref 2 got instance 2', + 'outer componentDidMount', + 'start update', + // Previous (equivalent) refs get cleared + 'ref 1 got null', + 'inner 1 render', + 'ref 2 got null', + 'inner 2 render', + 'inner 1 componentDidUpdate', + 'ref 1 got instance 1', + 'inner 2 componentDidUpdate', + 'ref 2 got instance 2', + 'outer componentDidUpdate', + 'start unmount', + 'outer componentWillUnmount', + 'ref 1 got null', + 'inner 1 componentWillUnmount', + 'ref 2 got null', + 'inner 2 componentWillUnmount' + ]); }); it('should correctly determine if a component is mounted', function() {