Skip to content

Commit 6666538

Browse files
petehuntzpao
authored andcommitted
Unbreak refs
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.
1 parent 620c1bc commit 6666538

File tree

4 files changed

+26
-6
lines changed

4 files changed

+26
-6
lines changed

src/core/ReactCompositeComponent.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -767,6 +767,7 @@ var ReactCompositeComponentMixin = {
767767
construct: function(initialProps, children) {
768768
// Children can be either an array or more than one argument
769769
ReactComponent.Mixin.construct.apply(this, arguments);
770+
ReactOwner.Mixin.construct.apply(this, arguments);
770771

771772
this.state = null;
772773
this._pendingState = null;
@@ -873,10 +874,7 @@ var ReactCompositeComponentMixin = {
873874
this._renderedComponent = null;
874875

875876
ReactComponent.Mixin.unmountComponent.call(this);
876-
877-
if (this.refs) {
878-
this.refs = null;
879-
}
877+
ReactOwner.Mixin.unmountComponent.call(this);
880878

881879
// Some existing components rely on this.props even after they've been
882880
// destroyed (in event handlers).

src/core/ReactOwner.js

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
"use strict";
2020

21+
var emptyObject = require('emptyObject');
2122
var invariant = require('invariant');
2223

2324
/**
@@ -118,6 +119,10 @@ var ReactOwner = {
118119
*/
119120
Mixin: {
120121

122+
construct: function() {
123+
this.refs = emptyObject;
124+
},
125+
121126
/**
122127
* Lazily allocates the refs object and stores `component` as `ref`.
123128
*
@@ -132,7 +137,7 @@ var ReactOwner = {
132137
'attachRef(%s, ...): Only a component\'s owner can store a ref to it.',
133138
ref
134139
);
135-
var refs = this.refs || (this.refs = {});
140+
var refs = this.refs === emptyObject ? (this.refs = {}) : this.refs;
136141
refs[ref] = component;
137142
},
138143

@@ -145,6 +150,10 @@ var ReactOwner = {
145150
*/
146151
detachRef: function(ref) {
147152
delete this.refs[ref];
153+
},
154+
155+
unmountComponent: function() {
156+
this.refs = null;
148157
}
149158

150159
}

src/core/__tests__/refs-test.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,5 +236,17 @@ describe('ref swapping', function() {
236236
expect(refHopsAround.refs.divTwoRef).toEqual(secondDiv);
237237
expect(refHopsAround.refs.divThreeRef).toEqual(thirdDiv);
238238
});
239+
240+
241+
it('always has a value for this.refs', function() {
242+
var Component = React.createClass({
243+
render: function() {
244+
return <div />;
245+
}
246+
});
247+
248+
var instance = ReactTestUtils.renderIntoDocument(<Component />);
249+
expect(!!instance.refs).toBe(true);
250+
});
239251
});
240252

src/utils/__tests__/cloneWithProps-test.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ require('mock-modules').dontMock('cloneWithProps');
2424
var mocks = require('mocks');
2525

2626
var cloneWithProps = require('cloneWithProps');
27+
var emptyObject = require('emptyObject');
2728

2829
var React;
2930
var ReactTestUtils;
@@ -107,7 +108,7 @@ describe('cloneWithProps', function() {
107108
console.warn = mocks.getMockFunction();
108109

109110
var component = ReactTestUtils.renderIntoDocument(<Grandparent />);
110-
expect(component.refs).toBe(undefined);
111+
expect(component.refs).toBe(emptyObject);
111112
expect(console.warn.mock.calls.length).toBe(1);
112113
} finally {
113114
console.warn = _warn;

0 commit comments

Comments
 (0)