Skip to content

Commit

Permalink
Warn if getDOMNode or isMounted is accessed in render
Browse files Browse the repository at this point in the history
This is an anti-pattern that can be easily avoided by putting the logic
in componentDidMount and componentDidUpdate instead.

It creates a dependency on stale data inside render without enforcing
a two-pass render.
  • Loading branch information
sebmarkbage committed Feb 17, 2015
1 parent 9119412 commit 2490289
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 27 deletions.
1 change: 1 addition & 0 deletions src/browser/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,5 @@ describe('ReactDOM', function() {
expect(element.type).toBe('div');
expect(console.warn.argsForCall.length).toBe(0);
});

});
18 changes: 18 additions & 0 deletions src/browser/findDOMNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
*/

'use strict';

var ReactCurrentOwner = require('ReactCurrentOwner');
var ReactInstanceMap = require('ReactInstanceMap');
var ReactMount = require('ReactMount');

var invariant = require('invariant');
var isNode = require('isNode');
var warning = require('warning');

/**
* Returns the DOM node rendered by this element.
Expand All @@ -24,6 +27,21 @@ var isNode = require('isNode');
* @return {DOMElement} The root node of this element.
*/
function findDOMNode(componentOrElement) {
if (__DEV__) {
var owner = ReactCurrentOwner.current;
if (owner !== null) {
warning(
owner._warnedAboutRefsInRender,
'%s is accessing getDOMNode or findDOMNode inside its render(). ' +
'render() should be a pure function of props and state. It should ' +
'never access something that requires stale data from the previous ' +
'render, such as refs. Move this logic to componentDidMount and ' +
'componentDidUpdate instead.',
owner.getName() || 'A component'
);
owner._warnedAboutRefsInRender = true;
}
}
if (componentOrElement == null) {
return null;
}
Expand Down
16 changes: 16 additions & 0 deletions src/classic/class/ReactClass.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

var ReactComponent = require('ReactComponent');
var ReactCurrentOwner = require('ReactCurrentOwner');
var ReactElement = require('ReactElement');
var ReactErrorUtils = require('ReactErrorUtils');
var ReactInstanceMap = require('ReactInstanceMap');
Expand Down Expand Up @@ -746,6 +747,21 @@ var ReactClassMixin = {
* @final
*/
isMounted: function() {
if (__DEV__) {
var owner = ReactCurrentOwner.current;
if (owner !== null) {
warning(
owner._warnedAboutRefsInRender,
'%s is accessing isMounted inside its render() function. ' +
'render() should be a pure function of props and state. It should ' +
'never access something that requires stale data from the previous ' +
'render, such as refs. Move this logic to componentDidMount and ' +
'componentDidUpdate instead.',
owner.getName() || 'A component'
);
owner._warnedAboutRefsInRender = true;
}
}
var internalInstance = ReactInstanceMap.get(this);
return (
internalInstance &&
Expand Down
20 changes: 0 additions & 20 deletions src/core/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,26 +254,6 @@ describe('ReactComponent', function() {
]);
});

it('should correctly determine if a component is mounted', function() {
var Component = React.createClass({
componentWillMount: function() {
expect(this.isMounted()).toBeFalsy();
},
componentDidMount: function() {
expect(this.isMounted()).toBeTruthy();
},
render: function() {
expect(this.isMounted()).toBeFalsy()
return <div/>;
}
});

var element = <Component />;

var instance = ReactTestUtils.renderIntoDocument(element);
expect(instance.isMounted()).toBeTruthy();
});

it('fires the callback after a component is rendered', function() {
var callback = mocks.getMockFunction();
var container = document.createElement('div');
Expand Down
52 changes: 45 additions & 7 deletions src/core/__tests__/ReactComponentLifeCycle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,16 +261,54 @@ describe('ReactComponentLifeCycle', function() {
);
});

it('is not mounted inside initial render', function() {
var InitialRender = React.createClass({
it('should correctly determine if a component is mounted', function() {
spyOn(console, 'warn');
var Component = React.createClass({
componentWillMount: function() {
expect(this.isMounted()).toBeFalsy();
},
componentDidMount: function() {
expect(this.isMounted()).toBeTruthy();
},
render: function() {
expect(this.isMounted()).toBe(false);
return (
<div></div>
);
expect(this.isMounted()).toBeFalsy()
return <div/>;
}
});
ReactTestUtils.renderIntoDocument(<InitialRender />);

var element = <Component />;

var instance = ReactTestUtils.renderIntoDocument(element);
expect(instance.isMounted()).toBeTruthy();

expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain(
'Component is accessing isMounted inside its render()'
);
});

it('warns if getDOMNode is used inside render', function() {
spyOn(console, 'warn');
var Component = React.createClass({
getInitialState: function() {
return { isMounted: false };
},
componentDidMount: function() {
this.setState({ isMounted: true });
},
render: function() {
if (this.state.isMounted) {
expect(this.getDOMNode().tagName).toBe('DIV');
}
return <div/>;
}
});

ReactTestUtils.renderIntoDocument(<Component />);
expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain(
'Component is accessing getDOMNode or findDOMNode inside its render()'
);
});

it('should carry through each of the phases of setup', function() {
Expand Down
1 change: 1 addition & 0 deletions src/core/instantiateReactComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ function instantiateReactComponent(node, parentCompositeType) {

if (__DEV__) {
instance._isOwnerNecessary = false;
instance._warnedAboutRefsInRender = false;
}

// Internal instances should fully constructed at this point, so they should
Expand Down

0 comments on commit 2490289

Please sign in to comment.