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

;\n+ return

this.innerRef = c} \/>;\n },\n render: function() {\n return (\n this.wrapperRef = c}\n getContent={this.getInner}\n \/>\n );\n },\n componentDidMount: function() {\n- \/\/ TODO: Currently new refs aren't available on initial render\n- },\n- componentDidUpdate: function() {\n \/\/ Check .props.title to make sure we got the right elements back\n- this.wrapperRef.then(function(wrapper) {\n- expect(wrapper.props.title).toBe(\"wrapper\");\n- refsResolved++;\n- });\n- this.innerRef.then(function(inner) {\n- expect(inner.props.title).toEqual(\"inner\");\n- refsResolved++;\n- });\n- expect(refsResolved).toBe(0);\n+ expect(this.wrapperRef.props.title).toBe('wrapper');\n+ expect(this.innerRef.props.title).toBe('inner');\n+ mounted = true;\n }\n });\n \n var instance = ;\n instance = ReactTestUtils.renderIntoDocument(instance);\n- instance.forceUpdate();\n- expect(refsResolved).toBe(2);\n+ expect(mounted).toBe(true);\n+ });\n+\n+ it('should call refs at the correct time', function() {\n+ var log = [];\n+\n+ var Inner = React.createClass({\n+ render: function() {\n+ log.push(`inner ${this.props.id} render`);\n+ return

;\n+ },\n+ componentDidMount: function() {\n+ log.push(`inner ${this.props.id} componentDidMount`);\n+ },\n+ componentDidUpdate: function() {\n+ log.push(`inner ${this.props.id} componentDidUpdate`);\n+ },\n+ componentWillUnmount: function() {\n+ log.push(`inner ${this.props.id} componentWillUnmount`);\n+ }\n+ });\n+\n+ var Outer = React.createClass({\n+ render: function() {\n+ return (\n+

\n+ {\n+ log.push(`ref 1 got ${c ? `instance ${c.props.id}` : 'null'}`);\n+ }}\/>\n+ {\n+ log.push(`ref 2 got ${c ? `instance ${c.props.id}` : 'null'}`);\n+ }}\/>\n+ <\/div>\n+ );\n+ },\n+ componentDidMount: function() {\n+ log.push('outer componentDidMount');\n+ },\n+ componentDidUpdate: function() {\n+ log.push('outer componentDidUpdate');\n+ },\n+ componentWillUnmount: function() {\n+ log.push('outer componentWillUnmount');\n+ }\n+ });\n+\n+ \/\/ mount, update, unmount\n+ var el = document.createElement('div');\n+ log.push('start mount');\n+ React.render(, el);\n+ log.push('start update');\n+ React.render(, el);\n+ log.push('start unmount');\n+ React.unmountComponentAtNode(el);\n+\n+ expect(log).toEqual([\n+ 'start mount',\n+ 'inner 1 render',\n+ 'inner 2 render',\n+ 'inner 1 componentDidMount',\n+ 'ref 1 got instance 1',\n+ 'inner 2 componentDidMount',\n+ 'ref 2 got instance 2',\n+ 'outer componentDidMount',\n+ 'start update',\n+ \/\/ Previous (equivalent) refs get cleared\n+ 'ref 1 got null',\n+ 'inner 1 render',\n+ 'ref 2 got null',\n+ 'inner 2 render',\n+ 'inner 1 componentDidUpdate',\n+ 'ref 1 got instance 1',\n+ 'inner 2 componentDidUpdate',\n+ 'ref 2 got instance 2',\n+ 'outer componentDidUpdate',\n+ 'start unmount',\n+ 'outer componentWillUnmount',\n+ 'ref 1 got null',\n+ 'inner 1 componentWillUnmount',\n+ 'ref 2 got null',\n+ 'inner 2 componentWillUnmount'\n+ ]);\n });\n \n it('should correctly determine if a component is mounted', function() {\n"}

dillon1000/react

Files changed