Rule proposal: warn against using findDOMNode()
gaearon opened this issue · 134 comments
There are almost no situations where you’d want to use findDOMNode() over callback refs. We want to deprecate it eventually (not right now) because it blocks certain improvements in React in the future.
For now, we think establishing a lint rule against it would be a good start. Here’s a few examples of refactoring findDOMNode() to better patterns.
findDOMNode(this)
Before:
class MyComponent extends Component {
componentDidMount() {
findDOMNode(this).scrollIntoView();
}
render() {
return <div />
}
}After:
class MyComponent extends Component {
componentDidMount() {
this.node.scrollIntoView();
}
render() {
return <div ref={node => this.node = node} />
}
}findDOMNode(stringDOMRef)
Before:
class MyComponent extends Component {
componentDidMount() {
findDOMNode(this.refs.something).scrollIntoView();
}
render() {
return (
<div>
<div ref='something' />
</div>
)
}
}After:
class MyComponent extends Component {
componentDidMount() {
this.something.scrollIntoView();
}
render() {
return (
<div>
<div ref={node => this.something = node} />
</div>
)
}
}findDOMNode(childComponentStringRef)
Before:
class Field extends Component {
render() {
return <input type='text' />
}
}
class MyComponent extends Component {
componentDidMount() {
findDOMNode(this.refs.myInput).focus();
}
render() {
return (
<div>
Hello, <Field ref='myInput' />
</div>
)
}
}After:
class Field extends Component {
render() {
return (
<input type='text' ref={this.props.inputRef} />
)
}
}
class MyComponent extends Component {
componentDidMount() {
this.inputNode.focus();
}
render() {
return (
<div>
Hello, <Field inputRef={node => this.inputNode = node} />
</div>
)
}
}Other cases?
There might be situations where it’s hard to get rid of findDOMNode(). This might indicate a problem in the abstraction you chose, but we’d like to hear about them and try to suggest alternative patterns.
Some ideas to ease the transition:
Automatically reference the top node rendered on the component
class MyComponent extends Component {
componentDidMount() {
this._topDOMNode.scrollIntoView();
}
render() {
return <div />
}
}Somehow do shorthand aliasing for the ref prop:
class MyComponent extends Component {
componentDidMount() {
this.node.scrollIntoView();
}
render() {
return <div ref={this.node} />
}
}Provide a reference to the DOM node separately from the element reference:
class MyComponent extends Component {
componentDidMount() {
this.nodeRefs.myNode.scrollIntoView();
}
render() {
return <div ref="myNode" />
}
}In my opinion it seems like one of those cases where the only reason people want shortcuts is because they’ve been exposed to shorter magic syntax. Shorthands might seem “nice” but they actually make less sense coming from a beginner’s perspective. It’s easier to learn how the system works once than remember that topDOMNode is automatic but for everything else you need to use refs, or that this.node is somehow going to turn into a magic ref but there is just one such ref. As for the string suggestion, we won’t go with it because string refs are problematic by themselves, and so we want to get away from them as well.
@gaearon hey, can you leave a short note or is there a link to read why ref callbacks are preferred to ref strings? Thanks
@gaearon Great idea! However, is this in addition to a warning within React itself? If the React team definitely wants to deprecate this, seems like it should definitely warn there, as well. Not everyone may use ESLint, and IMO, it's not ESLint's responsibility to notify users about feature deprecation.
@notaurologist There isn't a feature deprecation, just a potentially bad pattern, which is definitely ESLint's domain.
Suppose that I want to create a behavior component that acts on the DOM of its provided child (like a fake mutation observer, for instance):
class Measure extends Component {
componentDidMount() {
const childNode = findDOMNode(this).children[0];
// Here I call whatever I want when the child is loaded
// @NOTE: There's no refs implied.
}
render() {
const { children } = this.props;
// Here, I'm agnostic to whatever the child might be, a pure function or a class
return children;
}
}Now, as I'm agnostic to the type children passed like this:
<Measure>
<AnyChildWithoutRefsLikePureComponent/>
</Measure>I could clone the AnyChildWithoutRefsLikePureComponent, check it its a Component or a pure function, and if it's a pure function, turns it into a Component just to get a dynamic ref on it. But that would defeat the whole purpose of being agnostic to Component vs pure function.
@gaearon small side note, in the after block - I believe the findDOMNode(childComponentStringRef) example is supposed to read this.inputNode.focus(); rather than this.inputNode.scrollIntoView();
There's an example here of what @PEM-- described.
Basically, it's an element that pins its children scroll position to bottom as it grows.
const { render, findDOMNode } = ReactDOM
class PinToBottom extends React.Component {
/// ...
componentWillUpdate() {
const node = findDOMNode(this)
// ...
}
componentDidUpdate() {
if (this.pinToBottom) {
const node = findDOMNode(this)
node.scrollTop = node.scrollHeight
}
}
render() {
return React.Children.only(this.props.children)
}
}And then it can be used by any content container, like that:
<PinToBottom>
<ul>
{lines.map((line, index) => (
<li key={index}>{line}</li>
))}
</ul>
</PinToBottom>I don't know how it could be made simpler by using callback refs or something else.
How do you suggest one deals with higher order functions over non-dom components, such as
var Wrapper = ComposedElement => class extends React.Component {
componentDidMount() {
// if ComposedElement is not a DOM component
// this.domNode <> ReactDOM.findDOMNode(this)
}
render() {
return <ComposedElement ref={r=>{this.domNode = r}}/>
}
};@alaindresse why would that work any differently using this.domNode? the wrapped component would still work the same with the ref as it would with findDOMNode.
I'm also wondering how is HOCs like this should be re-written then?
https://github.com/okonet/react-container-dimensions/blob/master/src/index.js
Your third example's "After" doesn't match with "Before".
this.inputNode.scrollIntoView();Should be
this.inputNode.focus();Actually, it seems to me that all examples drill down to the same behavior. If we could setup a callback ref on a children passed as prop, the findDOMNode could then be removed.
A function like React.Children.addCallbackRef could do it.
Sounds legitimate. A parent can call its children 😉
@gaearon, what do you think of this?
@timdorr I agree, but as @gaearon said: "We want to deprecate it eventually (not right now) because it blocks certain improvements in React in the future." I think this definitely warrants an additional warning in React.
Deprecating is adding a warning. So we plan to add a warning but in the future. I thought that maybe ESLint rule might be a better first idea before we deprecate it.
@ljharb in the HOC, you don't know if ComposedElement is a DOM or a react class. The reference is then either a DOM node, or an instance of a react class. In the latter case, you need findDOMNode to get the actual dom node...
One idea would be to have two arguments in the ref callback
ref={(r,n)=>{this.component = r; this.node = n}
if the component is a dom node, r===n. If the component is a react class, n is the topDOMNode @timdorr referred to earlier.
Suppose that I want to create a behavior component that acts on the DOM of its provided child
Yes, HOCs like this would have to wrap their content in an extra <div>. This sounds bad until you see that the whole concept is flawed.
People often request that React adds support for returning multiple nodes from render (“fragments”). Imagine we implement this. Now any component can either return zero, one, or many nodes.
Somebody changes one of the “measured” components to return two nodes in some state. What should findDOMNode return? First node? An array?
Whether a component returns many nodes is its implementation detail. It should be possible to change it without breaking any code in other components, but this would not work with our HOC. So the pattern itself is problematic.
There are two solutions:
- Make HOC add a wrapping
<div>. This is the easiest way to preserve encapsulation. - If absolutely necessary, instead let HOC inject a callback prop like
refToMeasureso wrapped component can use<div ref={this.props.refToMessure}>. This is identical to how my last example works. Components explicitly exposed nodes they want to.
Reading nodes of child components is like wanting to access their state. This is not a pattern we should support or allow (even if it is technically possible). If it was unsupported from day one, I don’t think it would be much of a controversy. However it is less obvious that the pattern is problematic because it’s been possible for a while.
If we could setup a callback ref on a children passed as prop, the findDOMNode could then be removed.
You can as long as children are DOM elements. You can check for that with this.props.children && typeof this.props.children.type === 'string'. In this case, to attach a callback ref, you can use cloneElement with a ref callback that calls the original ref function if it exists, and then does whatever you needed.
For reasons above you cannot do this on custom components.
Indeed, I agree. It's like having a form that parses its input fields instead of giving the fields the capabilities to inform the form itself. It's parsing against eventing. And that's against the purpose of React 👍
DOM based operation should ensure that a parsable DOM is present.
its possible to clone children with ref callback added to it, so it can be exposed to a wrapper component
@Andarist @PEM-- Yes! The nice (or bad 😄) thing about findDOMNode is that it can be another React.Component, something like:
<PinToBottom>
<List>
{lines.map((line, index) => (
<ListItem key={index}>{line}</ListItem>
))}
</List>
</PinToBottom>And it'll find its DOM node anyway.
- Here is the working example, using
findDOMNode. - Here is the non working example, using callback refs.
One can argue that by using findDOMNodeyou're breaking the component's encapsulation, but besides that, I think that for this particular use case findDOMNode is more straightforward than cloneElement + callback refs. But maybe it's just me 😉
Just read @gaearon comments, and I agree 1000% with:
Whether a component returns many nodes is its implementation detail. It should be possible to change it without breaking any code in other components, but this would not work with our HOC. So the pattern itself is problematic.
Now that I understand the problem better and how this make returning multiple nodes from render very difficult, I rewrote the example to wrap every children of PinToBottom inside a div.
It's much cleaner and doesn't break encapsulation!
To come back to the ESLint rule
Deprecating is adding a warning. So we plan to add a warning but in the future. I thought that maybe ESLint rule might be a better first idea before we deprecate it.
I'm agree, also adding a rule like this is pretty easy (we've already done something similar for isMounted()).
As an additional example for those who need access to a component DOM node from its parent but also from the component itself, here is the modified code from @gaearon example:
class Field extends Component {
render() {
return (
<input type='text' ref={node => this.props.inputRef(node) && (this.node = node)} />
)
}
}It seems somewhat trivial but it took me a bit of time to figure it out so it may help anybody with the same use case.
Another problem I can come up with, is when it comes to animating properties like scrollTop. Animating scrollTop with any animation library requires the actual DOM node. For example, the only way to add animation to @fhelwanger's pin to bottom codepen example would be to manually hardcode the animation logic, instead of being able to reuse Velocity or jQuery, etc
@perrin4869 I described this use case in the very first post in this thread. You can get the DOM node without findDOMNode(), just use a ref.
@perrin4869 Check this: #678 (comment)
You can use the ref available inside PinToBottom to get the DOM node you need. Then you can pass it to jQuery, Velocity, etc.
Hm... I'm confused then. In the snippet below, is there any difference between this.node and this.domNode, and could it affect how animation libraries work?
class Foo extends Component {
componentDidMount() {
this.domNode = findDOMNode(this.node);
}
render() {
return <div ref={c => { this.node = c; }} />;
}
}@perrin4869 You don't need this.domNode = findDOMNode for it to work. The ref prop on your div already gives you the DOM element. Thus, this.node is already your DOM element.
You can do something like the following:
class Foo extends Component {
componentDidMount() {
$(this.node).slideDown();
}
render() {
return <div ref={c => this.node = c } />;
}
}@perrin4869 the use case where there is a difference, is illustrated by @gaearon in the last example of his first post, is when you reference a react component - and not already a DOM element like the div in your example - and need to access the actual DOM node of this component (eg to use the focus() method of an input tag).
In that case, the children component needs to expose a prop to forward the DOM node to its parent, as shown in the example, where previously the parent would just use a ref on the children component and call findDOMNode()
@pandaiolo but you probably shouldn't be accessing the DOM node from another component.
Isn't the TextField a good example? I also have a video component enriching the video DOM element, which I need to access props and methods (play(), pause(), paused, currentTime, etc). Is there a reason I would not let the parent access the video DOM node?
I think the best solution would be to let the video component have a isPlaying prop, which changes trigger the required methods on the video component.
Something like this:
class Video extends React.Component {
getDefaultProps() {
return { isPlaying: false }
}
componentDidUpdate(prevProps) {
if(this.props.isPlaying !== prevProps.isPlaying) {
this.props.isPlaying ? this.video.play() : this.video.pause()
}
}
render() {
return <video ref={(el) => this.video = el} src="..."></video>
}
}The component is simple and just exposes the video events as a prop. I could explicitly rewire all the native methods and props as well, but i'm not sure the added complexity is worth the linter code in that case. In the end that would just mean putting back the video element in the parent component.
Thanks for pointing this anyway, I fully adhere to the "React way" so I understand that based on the simple example, it makes sense to ditch the external ref!
Are you using React without Redux or flux or anything? Because then I can understand. But when you do use one of those, you really want to control the video using the state of the application.
I do in the parent component which concern is about video control. Putting the video element back there would add rather than remove complexity / readability - but yeah it's my call :)
Isn't the TextField a good example? I also have a video component enriching the video DOM element, which I need to access props and methods (play(), pause(), paused, currentTime, etc). Is there a reason I would not let the parent access the video DOM node?
In this case you can put them on the TextField instance. Then the parent will have a ref to that instance, and call methods on it. And TextField will use its own ref to DOM element.
play() {
this.node.play()
}etc
It’s more typing but it’s also more explicit and doesn’t leak implementation details.
This is the Law of Demeter
deprecating findDOMNode is a an alarmingly upseting idea... Unless you are adding back getDOMNode(), it's going to make writing a large amount of react code just so so terrible. "Add and extra DOM node to use callback refs" is really really untenable for a ton of use cases; along with, effectively making interop with css frameworks not written with react almost impossible.
not to mention it effectively just ignores the fact that multiple layers of HOCs are the norm now thanks to mixin "deprecation" :P
This sounds like an idea from folks who nicely own every single component they use. for those using libraries... oof
If you have specific examples of problematic patterns please post them in the thread. This is exactly what I did in the post, and it would really help us weigh on this decision (and maybe think of better alternatives). It would help us a lot if you could do the same thing. You might think your concerns are obvious and we are intentionally ignoring them. In reality it's not at all obvious what you are concerned about without code samples. Thanks!
I also believed I explained above in this thread why findDOMNode API is fundamentally incompatible with some features long requested by the community (such as fragments). We are not deprecating it just because we enjoy deprecating things. 😉
The case where this comes up most acutely is having a positioner component for e.g. tooltips and overlays.
Consider:
<Button ref={c => { this.button = c; }} />
<Position target={this.button}>
<Tooltip>Some tooltip</Tooltip>
</Position>For <Position> to work as a generic component that can e.g. calculate the size of its child, then position it appropriately relative to its target, it needs to be able to get the child's DOM node.
To be compatible with e.g. Bootstrap styling, it's not generally possible for a component like <Position> to render an extra <div>.
<Position> just adding a ref to its child is insufficient, since its child may well be some composite component (e.g. <Tooltip> above) rather than a DOM component.
The category in general is utility components that need to interact with the DOM nodes belonging to their children, for functionality such as positioning, animation, &c.
If you control all of your own styling, then perhaps the extra <div> is acceptable, though not ideal. If you don't control your CSS, then adding an extra <div> can be a non-option.
Sorry if I sound annoyed, these sort of drastic changes can feel like they are coming out of the blue. A proper RFC process for this stuff would really be helpful here.
to add to @taions point, so also for focus management, which is required for accessible components, right now react just offers nothing useful for that problem which requires dropping down to DOM nodes, and not wrapping ones in the HOC case. imperative methods sort of work but are often and frustratingly obscured by HOCs, and just not possible for function components.
The extra DOM nodes are often presented as a "just do that" sort of simple solution but I feel like that ignores that layout and styling in the DOM is extremely intolerant to adding in bonus nodes. not to mention the lack of semantic value. To be honest I think most often the reason want folks want fragments (maybe one of the most requested API changes?) in the first place is exactly to avoid these bonus nodes.
I appreciate the desire to really abstract away the DOM as a render target, but these escape hatches are important not the least of which because the DOM is not particularly a good target for this sort of thing and wildly quirky (I encourage folks to look at the react event code to see just what is needed for consistency on that front).
Saying you can only drop to the host objects via function refs feels like a line drawn against an arbitrary and unhelpful line. Escape hatches should be used prudently and judiciously but they should be easily and constitently accessible, otherwise they aren't useful in any case. This is the same problem with stateless function components not being ref-able. there is no real reason to think someone would not need the escape hatch for those components over classy ones.
it seems like if findDOMNode is blocking other features then perhaps change they way it works, instead of removing it? Personally I'd be fine if it sometimes returned an array of nodes vs a single one, jQuery has done this with a ton of success forever
If you have specific examples of problematic patterns please post them in the thread.
@gaearon I ran into a problematic pattern today where I think I need findDOMNode. This is a bit of an unusual situation and if you can see an alternate solution, I would love to know.
I am currently working on setting up Happo to do visual regression testing in CI. The basic idea is we have some code that:
- renders a React component to the DOM
- finds the coordinates the rendered component occupies in the DOM
- takes a screenshot of the page
- crops the screenshot to match the coordinates
In order to get any component's coordinates, we need a reference to the DOM node. Because you can't use ref on SFCs, we have a basic wrapper component that we wrap when rendering these examples:
class Wrapper extends React.Component {
render() {
return React.Children.only(this.props.children);
}
}Then, inside our example, we have some code like:
const container = document.createElement('div');
document.body.appendChild(container);
let elemRef;
const elem = (
<Wrapper ref={(ref) => { elemRef = ref; }}>
{example}
</Wrapper>
);
ReactDOM.render(elem, container);
setTimeout(() => done(ReactDOM.findDOMNode(elemRef)), 0);From this DOM node reference, we find the coordinates via getBoundingClientRect().
Perhaps I've overlooked something, but is there currently a way to achieve this without ReactDOM.findDOMNode()? We thought that perhaps we could wrap it in an inline-block <div> with a ref (which then gives us a DOM node), but that changes the styling characteristics of some components (e.g. display: block elements don't stretch out).
Is there an issue tracking this on facebook/react?
It's a bit weird to discuss it here. The lint rule is fine, and I think we all agree that many or even most uses of findDOMNode are better handled with a ref on a DOM component.
The eslint-plugin-react repo is not the right place to discuss whether findDOMNode should exist as a React DOM feature.
Please feel free to file a discussion on React repo, sure!
This is the same problem with stateless function components not being ref-able.
What would you expect to get as a reference to a functional component? Since it doesn’t have an instance, it’s not clear what you are suggesting.
Wouldn’t this work for your use case?
const container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(example, container);
setTimeout(() => done(container.firstChild), 0);The case where this comes up most acutely is having a positioner component for e.g. tooltips and overlays.
<Button ref={c => { this.button = c; }} /> <Position target={this.button}> <Tooltip>Some tooltip</Tooltip> </Position>For to work as a generic component that can e.g. calculate the size of its child, then position it appropriately relative to its target, it needs to be able to get the child's DOM node.
Please correct me if I’m wrong, but in this use case it seems like Position content would not be a part of the downward rendering tree anyway. I’m assuming that <Position> is going to render <Tooltip> to a different tree root (i.e. as a portal).
If my assumption is correct, you render this.props.children into another <div> anyway (like this this._mountNode). So you should have access to this._mountNode.firstChild as well. Sure, it’s not “Reacty”, but that’s the point: you are intentionally breaking enapsulation and accessing the platform-specific tree, so it makes sense that you use platform APIs for this.
Or maybe I misunderstand what <Position> does (which is why I asked for more complete examples).
(Note: it’s an official team’s position, just something I think makes sense. I may be wrong.)
to add to @taions point, so also for focus management, which is required for accessible components, right now react just offers nothing useful for that problem which requires dropping down to DOM nodes, and not wrapping ones in the HOC case. imperative methods sort of work but are often and frustratingly obscured by HOCs, and just not possible for function components.
I’ve used the “ref prop” pattern extensively and found it to work really well for this.
I believe I’ve specifically shown this example in the first post but I will repeat it with some comments:
class Field extends Component {
render() {
return (
// Note: ref={this.props.inputRef}. I'm delegating ref to a prop.
// This will work through many layers of HOCs because they pass props through.
<input type='text' ref={this.props.inputRef} />
)
}
}
class MyComponent extends Component {
componentDidMount() {
// It doesn't matter how many levels of HOCs above I am:
// I will still get this DOM node right here.
this.inputNode.focus();
}
render() {
return (
<div>
{/* Please note: I’m not using ref={}, I’m using my own inputRef={} prop.*/}
Hello, <Field inputRef={node => this.inputNode = node} />
</div>
)
}
}And this pattern works just as fine with functional components as it does with classes.
If you think it’s problematic can you please point out why?
And this pattern works just as fine with functional components as it does with classes.
If you think it’s problematic can you please point out why?
The main problem is that it requires all potential children to expose this sort of API. For an environment that owns all their components fine, that is a pattern that works*. Most of us however are cobbling together applications from lots of smaller libraries, getting every one of them to expose a ref prop is unlikely and fraught.
*More to the pattern itself though, it requires the consumed child to make the choice about whether it will expose it's DOM node. At first glance this sounds like encapsulation but its not; I'd argue its actually leaking implementation details. A child component cannot reasonably make the choice that no consuming parent will ever need to access its rendered node. The Api's that depend on/need findDOMNode are exactly the sort that don't work with this pattern. Positioning for instance is a concern that belongs to the parent not the child. It's unfortunate that the DOM doesn't allow us to express this in terms of the parent always (flexbox does which is one reason its great) but providing the escape hatch, for this DOM limitation, doesn't belong to the child.
What would you expect to get as a reference to a functional component?
The the DOM node of the component, like host components. Sorry for being a bit unclear my problem with the lack of ref-able function components is not that you don't get an instance (agree that makes no sense). The real problem is the same as i'm describing above, its that the component has to make an arbitrary choice about whether it allows an escape hatch to be used, i.e. accessing the underlying host node. Again, this is just not a choice a component can make, it has zero information necessary to make that choice. (I know you can wrap them in a stateful component, but come on, the DX of that is terrible). So we are in positions where we have to not use Function components on the off chance one of them needs to be measured in order to render a tooltip e.g. @taion's above example.
I think both me and @taion are on board that most times you should be using a ref for host node access. What is concerning though is that the escape hatches that make React super flexible and un-frustrating when working with the DOM are slowly being neutered by placing them behind gatekeepers that lack the context necessarily to allow their use.
There seems to be this side effect of api design choices that makes escape hatches harder and harder to use. This does de-incentivize use, but that hurts those of us that need them and makes it harder to provide helpful components and encapsulate the use of those escape hatches away from the consumer. Instead what's happening is we need to now involve our users more and more in the ugliness of FFI use by requiring they expose props for refs, or not use Function components. This seems counter intuitive, I'd be nicer to see more work going into solving the reasons why ppl need these escape hatches before actually or effectively removing them.
Sorry if my tone is harsh, It stems from a these changes that make our lives harder as maintainers. I realize also that folks may not have the same issues as us at React-Bootstrap, but most UI libs are not actively dealing with interop with non-react stuff. I feel like our use case is important, and also a bit more indicative of what folks working on brownfield applications face, especially those who don't post on the eslint-react-plugin repo :P
also a quick side point, CSS flexbox is extremely intolerant to extra wrapping nodes. On one hand this is one reason why fragments would be awesome, but that would be negated by HoC's needing to wrap in extra divs :P
@gaearon yes! I actually came to the same realization late last night. Thanks for being my rubber duck.
I did make one modification in case the component renders null:
const elem = container.firstChild instanceof Element
? container.firstChild
: container;Positioning for instance is a concern that belongs to the parent not the child.
In case of positioning, don’t you already pull the child out into a new tree where you have access to the DOM? See #678 (comment).
A child component cannot reasonably make the choice that no consuming parent will ever need to access its rendered node.
I’m not sure I’m following without a specific example. (I answered to all specific examples above so far, I think.)
You mentioned the focus use case, but isn’t it reasonable for third party React input components to always provide imperative focus() method? I don’t really see why this would be unexpected for those component libraries. Again, a specific example we could look at would be helpful here.
@gaearon not all of positioning involves subtrees. @taion's example is a good one still: the Button can have a ref, but you need to use findDOMNode to access the position info in order to display the overlay.
<Button ref={c => { this.button = findDOMNode(c); }} />
<Position target={this.button}>
<Tooltip>Some tooltip</Tooltip>
</Position>Or:
class ButtonWithTooltip extends React.Component {
componentDidMount() {
renderToolTipInSubtree({
target: ()=> findDOMNode(this)
})
},
render() {
// cannot wrap in a div, as it will not longer work in `ButtonToolbar` or ModalFooter
return <Button {...this.props}/>
}
}The above is a good example of who should own the use of the escape hatch. Button should not have to think about whether someone might want to display the tooltip. I don't think it makes sense for the button author to ask themselves: "May this need to be measured? Maybe! so no Function component and provide a ref prop"
The other use case is needing to measure, oneself.
class ElementQuery extends React.Component {
componentDidUpdate() {
let node = findDOMNode(this);
let visible = node.offsetWidth > this.props.width;
if (visible !== this.state.visible)
this.setState({ visible })
}
render() {
if (this.state.visible)
return React.Children.only(this.props.children)
return null;
}
}You mentioned the focus use case, but isn’t it reasonable for third party React input components to always provide imperative focus() method?
I agree though, again, the use HoC's for everything and Function components makes imperative methods mostly not an option anymore. Being able to drill down to the component node to focus it is often the only viable option, and well behaved input components should handle a native focus() correctly.
The above are simplistic examples of course that could have alternatives, I am providing them in the context of earlier conversation about how wrapping nodes aren't always feasible, and buttonRef props being leaky and semantically wrong.
In the position example, a lot of cases can just be handled with position: absolute. No need for a subtree there.
Button should not have to think about whether someone might want to display the tooltip. I don't think it makes sense for the button author to ask themselves: "May this need to be measured? Maybe! so no Function component and provide a ref prop"
I’m not quite convinced components that require this don’t already know this.
Do I understand correctly that the only reason why you have to use Button’s DOM node (and not a wrapping node) is because it’s a part of a CSS framework like Bootstrap which forces direct parent-child relationships and doesn’t allow extra DOM nodes?
It seems that Button then knows it’s in an intimate relationship with another component (such as ButtonGroup), and that it actually knows that it should expose its node to work around such issues.
If it was a truly independent component, it would work with a wrapping <div> just fine, and thus wouldn’t need to expose anything. But since it’s not, it looks like the Button has all the necessary knowledge to opt into sharing its node.
I can ask the same question about flex children: don’t they already know they are flex children and rely on having a specific parent, and thus need to expose their nodes just in case something else wants to access them?
The ergonomics of exposing the node is a different question (it could be made as simple as forwarding a ref without need for custom props). Ref forwarding would similarly solve the HOC problem in the same way.
But I wonder what are the cases where you can’t wrap a component in a <div> and that component isn’t aware of this.
We're trying to write primitives that can be used in multiple contexts. React-Bootstrap is one of those contexts, but it's not the only one.
Every external CSS library that needs to interop with React for things like popovers should not be required to bundle its own specific overlay wrapper.
I’m not quite sure I understand. I don’t suggest changes to CSS libraries.
I’m only saying that React components that wrap those CSS libraries generally seem to know when they’re relying on something like "component A must be direct child of component B".
And so in this case, with proposal like facebook/react#4213, they would need to add something like
getPublicInstance() {
return this.node;
}for their refs to get resolved to their node.
Would this not work for you?
It is not clear to me what you mean in the last comment so perhaps code again would help.
I’m not quite convinced components that require this don’t already know this.
I don't think every component should have to think about where someone might want to display a tooltip over it, which is what I mean here.
Do I understand correctly that the only reason why you have to use Button’s DOM node (and not a wrapping node) is because it’s a part of a CSS framework like Bootstrap which forces direct parent-child relationships and doesn’t allow extra DOM nodes?
Its not just bootstrap, somethings just can't be reasonably styled to handle either <button> or <div><button/></div>. In the case of a button group if you need to do something
.btn-group > *:not(:first-child, :last-child) {
border-radius: 0;
}
// and so on...to get buttons that are neatly styled next to each otherYou could right that as .btn-group .btn {} but that makes nesting stuff hard and just is flat out impossible with css-modules see for more context there.
It's easy to say "just write your styles in a way that is agnostic to DOM structure" but that often just not possible.
Its not just bootstrap, somethings just can't be reasonably styled to handle either
<button>or<div><button/></div>
My point was that presumably components using such classNames know they’re one of “those things”, and thus may opt into exposing their nodes like I described here: #678 (comment).
Is there some reason why a component might not be aware that it has to be used in a specific parent DOM structure?
Is there some reason why a component might not be aware that it has to be used in a specific parent DOM structure?
sure, most library components. We provide <Button>, its not possible for us to know that that user is going to write a <ButtonTabList> that uses Button. Or in the case where you are using Button from library A and Tooltip from library B. on a philosophical note.
On a more philosophical point it seems like an odd to me that HoC's should add DOM nodes, when the pattern is often used as a way to compose in behavior, not structure.
To be clear, we don't need findDOMNode specifically, we need a way to get at a component's rendered output without that component needing to explicitly opt in to that, or make it super simple to forward to the right node in the child, in the way that style and className work. The problem is that unless that pattern is explicitly defined by React the contract will differ from library to library making the idea of Overlay, or Position primitive that can be used in a wide range of contexts impossible. e.g. RB will have elementRef, while MUI has nodeRef, etc etc. tho I still consider that solution suboptimal to the current findDOMNode api.
The other issue with the "ref prop" approach is that it requires you to have control of everything that you render, if you need to avoid extra DOM nodes.
For example, if <MyComponent> renders a <div>, then it could use a ref prop. However, if <MyComponent> renders something like <MyCustomCard>, then <MyCustomCard> needs to know about that same ref prop and attach that to its DOM node. With additional composition, this adds a lot more clutter.
More problematically, if the top-level view component exists in a separate library, then this becomes close to intractable.
FWIW, I don't think we're far apart on this.
It's fine for it to be an opt-in for a component to expose a way to get its own DOM node, but this needs to be an ecosystem-wide convention.
Something like a pass-through nodeRef is unnecessarily clunky, though. Really it sounds like what you're describing is something like an opt-in version of the old getDOMNode.
sure, most library components. We provide , its not possible for us to know that that user is going to write a that uses Button. Or in the case where you are using Button from library A and Tooltip from library B. on a philosophical note.
Do I understand correctly that relying on Button’s className and writing stylesheets using it directly in expressions like .stuff > .button in other components is considered acceptable usage?
Do I understand correctly that relying on Button’s className and writing stylesheets using it directly in expressions like .stuff > .button in other components is considered acceptable usage?
when building on top of a UI framework like bootstrap or similar? sure, the class names are generally considered the api to the css framework. Admittedly React components allow an ergonomic way to avoid that, however, even limiting that wouldn't solve the problem:
.my-button-group {
display: flex;
align-items: space-between;
}
.my-button-group > * {
flex: 1;
}Uses no reliance on library classNames (and is the recommended approach for css-modules), but would break hard if the button child was wrapped in a div. It is also not easily written to handle an either a button or a button wrapped in a div.
Note that these examples assume one HOC, but having more isn't unreasonable either when you are trying to build lower level composable utilities, think: withTooltip(measurable(Component)) is each supposed to add another div?
Yeah, I see what you’re saying.
Thanks for explaining these use cases, we’ll keep them in mind for the future.
As I said before findDOMNode() is not deprecated, it’s just discouraged. I still think it’s reasonable to have this as a lint rule, as the use case is relatively rare and can easily be confined to a very specific sort of component (like Measurer etc).
I still think it’s reasonable to have this as a lint rule, as the use case is relatively rare and can easily be confined to a very specific sort of component (like Measurer etc).
definately, on the same page there :)
I have the following usecase and replacing findDOMNode with the ref returned in the callback makes the code throw when I try to call methods of the DOM node?
import React, { Component } from 'react';
class Align extends Component {
constructor(props) {
super(props);
this.alignRef = null;
}
componentDidMount() {
this.alignRef.getBoundingClientRect();
}
render() {
const childPropsNew = {
ref: node => { this.alignRef = node; },
};
return React.cloneElement(this.children, childPropsNew);
}
}Uncaught TypeError: this.alignRef.getBoundingClientRect is not a function
Is it because of the clone or am I doing something wrong?
With a ref callback that's an arrow function, you will sometimes get null in the ref argument. I'm not sure why this is, though.
It is not about the ref being null, the ref is set but it is a ReactElement and not a DOM node. So it works fine when using findDOMNode in the componentDidMount. So I don't understand how it should work without? Is the element instance supposed to proxy DOM methods/properties of the DOM node?
Ok I think I got it, I can replace findDOMNode when having refs on browser components( <div /> etc), but if i do not know if my child is ReactComponent/DOMNode I can't get around doing findDOMNode unless I would introduce extra markup around my child with a ref.
i have
<div ref={node => this.domNode = node} style={this.getStyle()}>{ this.props.children }</div>
when i do this
this.domNode.addEventListener('mousedown', this.onDrag);
there is an error
this.domNode.addEventListener is not a function
@ljharb
It is expected, you get null for clean up. Every time you pass an arrow, it changes, so we call the old arrow with null, and the new one with the node right away. This is not a perf problem.
@danez @Radwa0
If you believe this is a bug in React, please file a bug in React repo with a reproducing case.
This is discussion about a lint rule, so it is not a right place to file bugs.
@Gaeron sure, makes sense that it's not a perf problem - but if I haven't unmounted, I wouldn't expect to get anything but a node. Essentially, that's a gotcha to code around in arrow ref callbacks - I'll have to do node => { if (node) { this.foo = node; } }
On the opposite, it is deliberate that we pass null on unmounting so that your field gets nulled too. This is to prevent potential memory leaks.
Even before unmounting, you are passing a new function. So it gives null to the old function and node to the new function. There is normal (even if a bit non-obvious) and there is no reason to guard against this. Your code should already handle null because it has to handle unmounting. Please trust React to call it with the node after calling it with null during the update. React has tests for this. 😉
What am I missing?
Are there any lifecycle methods or event handlers that could get called between the old callback and the new one? We've seen bugs caused by other code expecting a node, but receiving null.
I don’t know the ref code that well but if you had issues with them, please try to isolate them and file bugs against React.
As far as I can see from the source, the refs get detached (called with null) before the internal state related to <div> (“the host instance”) is updated. Later, all refs get attached (called with DOM nodes) during the flush, which is currently synchronous in React unless you use unsupported extensions (like rAF rendering). There is indeed a period where you might not have refs, but as far as I see we don’t call lifecycle methods on the component itself during this period.
I can imagine that if <A> contains <B>, and <B> calls a callback passed down from <A> in its componentWillUpdate, and that callback accesses a ref to <B> on <A>, it might be null at this point. I’m not sure why we don’t just keep the old ref attached in this case but this is really a discussion we should have in a React issue.
All the examples above talk about callback ref on a <div>.
But what about callback ref on Component? It will give us the Component object for class, or null for pure functions. To get the DOM node, we have to transform the stateless function into a class, and use findDOMNode. How can you get the dom node without it?
@webdif Take a look at findDOMNode(childComponentStringRef) here #678 (comment).
I think it's what you're looking for.
@fhelwanger Thanks a lot 🎉
I did not update the child component, that's why it did not work. Make a lot of sense, even if it is not very convenient to do so!
Should we enhance the doc's for this rule to show a few more of these examples? Seems like this is going to be a common issue while the community learns how to use refs without findDOMNode.
There is already a link to this issue in the no-find-dom-node.md, but a more detailed doc could be useful, to avoid other developers to make the mistake I made between findDOMNode(this) and findDOMNode(childComponentStringRef).
Just updated to latest and started getting this error. Turns out I have a use case for findDOMNode that doesn't fall super neatly into the above categories. Actually I worked out a solution while I was writing this comment, but I'll post it anyway in case it's useful to anyone (or someone knows a cleaner solution).
I have a decorator called Dimensions that will set probs width and height on the decorated component.
(not the complete code):
export default function DimensionsDecorator(DecoratedComponent) {
class DimensionsContainer extends Component {
constructor(props) {
super(props)
this.state = { width: 0, height: 0 }
this.handleWindowResize = debounce(
this.handleWindowResize.bind(this),
RESIZE_DEBOUNCE_DELAY_MS
)
}
componentDidMount() {
window.addEventListener('resize', this.handleWindowResize)
this.updateDimensions()
}
componentWillUnmount() {
window.removeEventListener('resize', this.handleWindowResize)
}
handleWindowResize(event) {
this.updateDimensions()
}
updateDimensions() {
// eslint-disable-next-line react/no-find-dom-node
const element = findDOMNode(this.innerComponent)
if (!isHidden(element)) {
const width = element.offsetWidth
const height = element.offsetHeight
this.setState({ width, height })
}
}
render() {
const { width, height } = this.state
return (
<DecoratedComponent
ref={innerComponent => this.innerComponent = innerComponent}
width={width}
height={height}
{...this.props}
/>
)
}
}
return DimensionsContainer
}So, looking at the suggestions above, the closest fit I can think of is requiring components to also implement a containerRef prop perhaps? Something like this?
componentDidMount() {
if (this.containerElement == null) {
if (process.env.NODE_ENV === 'development') throw new Error(
`DimensionsDecorator expected component ${DecoratedComponent.name} ` +
`to handle \`containerRef\` prop`
)
}
window.addEventListener('resize', this.handleWindowResize)
this.updateDimensions()
}
render() {
const { width, height } = this.state
return (
<DecoratedComponent
containerRef={element => this.containerElement = element}
width={width}
height={height}
{...this.props}
/>
)
}Consider the following:
- A have a component from a third-party library (say, it's "material ui")
- I want to get a dom node from it
- It doesn't have a ref, though
What do?
How would you rewrite this ? Not sure I understand how the new ref method works :(
describe('Render', () => {
it('should render clock', () => {
const clock = TestUtils.renderIntoDocument();
const
const actualText = $el.find('.clock-text').text();
expect(actualText).toBe('01:02');
});
});
Should I render into document while passing the ref ?
@cAstraea you can use https://github.com/airbnb/enzyme
Again, to be clear, we don't have a problem per se with encouraging refs – but we really need the React team to promulgate some sort of standard for "this is the name of the prop to use for the ref to get to the DOM node". Otherwise interop across component libraries is a hopeless cause.
Just settle on nodeRef or elementRef or whatever. But pick something and give it the official imprimatur. I'm not interested in inventing something myself only to find that other libraries use different conventions, and we're stick with no ability to interoperate for months without extra shims. That's no good.
(and ideally also make it work appropriately with DOM elements)
I was using
handleClick = () => {
const collapseEl = ReactDOM.findDOMNode(this).querySelector('.collapse');
collapseEl.classList.toggle('active');
}I don't see how this notation can help
handleClick = () => {
const collapseEl = this.node.querySelector('.collapse');
collapseEl.classList.toggle('active');
}querySelector doesn't exist. Is there a way to target an element to add a css3 class, I need to trigger a css3 effect and state wasn't very helpful for that job !
@kopax If you're doing it right, querySelector should exists, as the ref callback is returning a HTMLElement object. You must have a render method like this:
render() {
return (
<div ref={(node) => { this.node = node; }}>
<div className="collapse" />
<button onClick={this.handleClick} />
</div>
);
}But, maybe you don't need ref alltogether. The best way to enjoy React is to embrace the declarative way, and just change the state when something happen. For example :
state = {
active: false,
}
handleClick = () => {
this.setState({ active: true });
}
render() {
return (
<div>
<div className={`collapse ${this.state.active ? 'active' : ''}`} />
<button onClick={this.handleClick} />
</div>
);
}With or without state, it just adds a class, thus it triggers the css effect.
(I edited the above comment to remove the ref callback entirely in the latter example)
@gaearon There is one more case that a component needs to know about click outside.
componentWillMount() {
document.addEventListener('click', this.onOutsideClick, false);
}
componentWillUnmount() {
document.removeEventListener('click', this.onOutsideClick, false);
}
onOutsideClick(event) {
const domNode = ReactDOM.findDOMNode(this);
if (!domNode.contains(event.target) && this.state.isOpen) {
this.setState({ isOpen: false });
}
}
Using ref:
<div ref={(node) => { this.mainNode = node; }}>
----
</div>
onOutsideClick(event) {
if (!this.mainNode.contains(event.target) && this.state.isOpen) {
this.setState({ isOpen: false });
}
}
I might be missing something, but trying to get the element dom height or any other property coming from (let's say) getBoundingClientRect() simply calling this.node.getBoundingClientRect() will return undefined. this on the latest version of react.
import React from 'react';
class Bla extends React.Component {
componentDidMount() {
console.log(this.node.getBoundingClientRect());
}
render() {
return (
<div ref={(node) => this.node = node}>blaaaaa</div>
);
}
}
export default Bla;
@andrevenancio If you store your DOM element in this.node, why do you use this.foo? Maybe, it's a typo?
import React from 'react';
class Bla extends React.Component {
componentDidMount() {
console.log(this.node.getBoundingClientRect());
}
render() {
return (
<div ref={(node) => this.node = node}>blaaaaa</div>
);
}
}
export default Bla;yeah that was a typo. corrected my comment.
Just copied and pasted your code.... it works. http://codepen.io/anon/pen/peRPrz?editors=0010
It must be something else in your code. Try to create a minimal example that reproduces your problem.
The one case where I was not able to use a callback ref instead of findDOMNode is with ReactCSSTransitionGroup. There's no other way to get the DOM node used as the wrapper element, because a ref on the ReactCSSTransitionGroup just gives you a ref to that component, not the DOM node.