Skip to content

Commit

Permalink
Unbreak refs
Browse files Browse the repository at this point in the history
If no refs are rendered, `this.refs` is undefined. This is bad since it deopts & is hard to look for. Instead we should make `this.refs` an immutable empty object.
  • Loading branch information
petehunt authored and zpao committed Mar 3, 2014
1 parent 620c1bc commit 6666538
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 6 deletions.
6 changes: 2 additions & 4 deletions src/core/ReactCompositeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,7 @@ var ReactCompositeComponentMixin = {
construct: function(initialProps, children) {
// Children can be either an array or more than one argument
ReactComponent.Mixin.construct.apply(this, arguments);
ReactOwner.Mixin.construct.apply(this, arguments);

this.state = null;
this._pendingState = null;
Expand Down Expand Up @@ -873,10 +874,7 @@ var ReactCompositeComponentMixin = {
this._renderedComponent = null;

ReactComponent.Mixin.unmountComponent.call(this);

if (this.refs) {
this.refs = null;
}
ReactOwner.Mixin.unmountComponent.call(this);

// Some existing components rely on this.props even after they've been
// destroyed (in event handlers).
Expand Down
11 changes: 10 additions & 1 deletion src/core/ReactOwner.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

"use strict";

var emptyObject = require('emptyObject');
var invariant = require('invariant');

/**
Expand Down Expand Up @@ -118,6 +119,10 @@ var ReactOwner = {
*/
Mixin: {

construct: function() {
this.refs = emptyObject;
},

/**
* Lazily allocates the refs object and stores `component` as `ref`.
*
Expand All @@ -132,7 +137,7 @@ var ReactOwner = {
'attachRef(%s, ...): Only a component\'s owner can store a ref to it.',
ref
);
var refs = this.refs || (this.refs = {});
var refs = this.refs === emptyObject ? (this.refs = {}) : this.refs;
refs[ref] = component;
},

Expand All @@ -145,6 +150,10 @@ var ReactOwner = {
*/
detachRef: function(ref) {
delete this.refs[ref];
},

unmountComponent: function() {
this.refs = null;
}

}
Expand Down
12 changes: 12 additions & 0 deletions src/core/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,5 +236,17 @@ describe('ref swapping', function() {
expect(refHopsAround.refs.divTwoRef).toEqual(secondDiv);
expect(refHopsAround.refs.divThreeRef).toEqual(thirdDiv);
});


it('always has a value for this.refs', function() {
var Component = React.createClass({
render: function() {
return <div />;
}
});

var instance = ReactTestUtils.renderIntoDocument(<Component />);
expect(!!instance.refs).toBe(true);
});
});

3 changes: 2 additions & 1 deletion src/utils/__tests__/cloneWithProps-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ require('mock-modules').dontMock('cloneWithProps');
var mocks = require('mocks');

var cloneWithProps = require('cloneWithProps');
var emptyObject = require('emptyObject');

var React;
var ReactTestUtils;
Expand Down Expand Up @@ -107,7 +108,7 @@ describe('cloneWithProps', function() {
console.warn = mocks.getMockFunction();

var component = ReactTestUtils.renderIntoDocument(<Grandparent />);
expect(component.refs).toBe(undefined);
expect(component.refs).toBe(emptyObject);
expect(console.warn.mock.calls.length).toBe(1);
} finally {
console.warn = _warn;
Expand Down

0 comments on commit 6666538

Please sign in to comment.