Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added findDOMNode, as we move toward deprecating g #2646

Merged
merged 1 commit into from
Dec 22, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions src/browser/__tests__/findDOMNode.-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* Copyright 2013-2014, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @emails react-core
*/

"use strict";

var React = require('React');
var ReactTestUtils = require('ReactTestUtils');

describe('findDOMNode', function() {
it('findDOMNode should return null if passed null', function() {
expect(React.findDOMNode(null)).toBe(null);
});

it('findDOMNode should find dom element', function() {
var MyNode = React.createClass({
render: function() {
return <div><span>Noise</span></div>;
}
});

var myNode = ReactTestUtils.renderIntoDocument(<MyNode />);
var myDiv = React.findDOMNode(myNode);
var mySameDiv = React.findDOMNode(myDiv);
expect(myDiv.tagName).toBe('DIV');
expect(mySameDiv).toBe(myDiv);
});

it('findDOMNode should reject random objects', function() {
expect(function() {React.findDOMNode({foo: 'bar'});})
.toThrow('Invariant Violation: Element appears to be neither ' +
'ReactComponent nor DOMNode (keys: foo)'
);
});

it('findDOMNode should reject unmounted objects with render func', function() {
expect(function() {React.findDOMNode({render: function(){}});})
.toThrow('Invariant Violation: Component contains `render` ' +
'method but is not mounted in the DOM'
);
});

});

50 changes: 50 additions & 0 deletions src/browser/findDOMNode.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright 2013-2014, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule findDOMNode
* @typechecks static-only
*/

"use strict";

var ReactComponent = require('ReactComponent');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused.

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

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

/**
* Returns the DOM node rendered by this element.
*
* @param {ReactComponent|DOMElement} element
* @return {DOMElement} The root node of this element.
*/
function findDOMNode(componentOrElement) {
if (componentOrElement == null) {
return null;
}
if (isNode(componentOrElement)) {
return componentOrElement;
}
if (ReactInstanceMap.has(componentOrElement)) {
return ReactMount.getNodeFromInstance(componentOrElement);
}
invariant(
!(componentOrElement.render != null && typeof(componentOrElement.render) === 'function'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might read a lot better if we demorganize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done. You guys seem to really like the inverted form of if statements. Personally, I tend to think it's easier to understand "if the component has a render function, throw an error" rather than "assert that render is null OR that the render property is not a function". The former is closer to the semantic reasoning, while the latter feels like demorganized compiled output. The exclamation point out front just converts the "invariant" into an "if", and the inside of the "if" is the failure condition. I know both are technically relying on short-circuit evaluation, but it just feels easier to reason about the former. Anyway, that's my two cents. Updated diff uses demorganized form.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree that I used to think of it as "if...then...throw". However, the invariant model has a long standing tradition at Facebook since the early PHP days. It's also similar to assert statements in several other languages.

It's nice an declarative in a TDD kind of way. It has grown on me. At the end of the day, it doesn't really matter which inversion you chose. Different people will have different mental models here.

That said, the worst kind is the double negative. To think this one through, you might have to do two conversions! Demorganizing is hard enough already. People (me) screw this up all the time. I'd go as far as to say that it's objectively worse to have double negatives like this.

'Component contains `render` method but is not mounted in the DOM',
Object.keys(componentOrElement)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no %s so these won't be inserted anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch, thanks!

);
invariant(
false,
'Element appears to be neither ReactComponent nor DOMNode (keys: %s)',
Object.keys(componentOrElement)
);
}

module.exports = findDOMNode;
2 changes: 2 additions & 0 deletions src/browser/ui/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ var ReactRef = require('ReactRef');
var ReactServerRendering = require('ReactServerRendering');

var assign = require('Object.assign');
var findDOMNode = require('findDOMNode');
var onlyChild = require('onlyChild');

ReactDefaultInjection.inject();
Expand Down Expand Up @@ -69,6 +70,7 @@ var React = {
},
constructAndRenderComponent: ReactMount.constructAndRenderComponent,
constructAndRenderComponentByID: ReactMount.constructAndRenderComponentByID,
findDOMNode: findDOMNode,
render: render,
renderToString: ReactServerRendering.renderToString,
renderToStaticMarkup: ReactServerRendering.renderToStaticMarkup,
Expand Down
10 changes: 2 additions & 8 deletions src/browser/ui/ReactBrowserComponentMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@

"use strict";

var ReactMount = require('ReactMount');

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

var ReactBrowserComponentMixin = {
/**
Expand All @@ -24,11 +22,7 @@ var ReactBrowserComponentMixin = {
* @protected
*/
getDOMNode: function() {
invariant(
this.isMounted(),
'getDOMNode(): A component must be mounted to have a DOM node.'
);
return ReactMount.getNodeFromInstance(this);
return findDOMNode(this);
}
};

Expand Down