Rule proposal: warn against using findDOMNode()

[gaearon]

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.

[timdorr]

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" />
  }
}

[gaearon]

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.

[vladshcherbin]

@gaearon hey, can you leave a short note or is there a link to read why ref callbacks are preferred to ref strings? Thanks

[notaurologist]

@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.

[timdorr]

@notaurologist There isn't a feature deprecation, just a potentially bad pattern, which is definitely ESLint's domain.

[notaurologist]

@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.

[PEM--]

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.

[soyarsauce]

@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();

[fhelwanger]

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.

[alaindresse]

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}}/>
    }
};

[ljharb]

@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.

[okonet]

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

[koenpunt]

Your third example's "After" doesn't match with "Before".

this.inputNode.scrollIntoView();

Should be

this.inputNode.focus();

[PEM--]

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?

[gaearon]

@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.

[alaindresse]

@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.

[gaearon]

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:

1. Make HOC add a wrapping <div>. This is the easiest way to preserve encapsulation.
2. If absolutely necessary, instead let HOC inject a callback prop like refToMeasure so 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.

[gaearon]

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.

[PEM--]

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.

[Andarist]

@pem @fhelwanger

its possible to clone children with ref callback added to it, so it can be exposed to a wrapper component

https://codepen.io/Andarist/pen/qNaNGY

[PEM--]

@Andarist: Thanks but it only works if your children are immediate DOM elements 😉

[fhelwanger]

@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 😉

[fhelwanger]

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!

https://codepen.io/anon/pen/qNVrwW?editors=0010

[yannickcr]

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()).

[pandaiolo]

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.

[sandy0201]

It's ok now, got it to work.

Instead of using ref={field.inputRef} in NumberFormat component, had to use getInputRef={field.inputRef}.

[andrevenancio]

Hey @sandy0201 I suspect that you're using it on a connected component.

Anyway, the ref should give you access to dom elements that are created inside the class you're using the ref. You can't find a dom element on a component that part of redux-form which I suspect is what the <Field /> is. The redux-form connects the Field to the store and therefore you can't find the dom reference...

I can't give you a precise solution without seeing the whole source, but it seems that this is not a problem related to React or the linter but how you're building your application.

As a rule think of it this way: ref={(e) => { this.something = e; }} creates a variable called this.something in the Class you're creating it. if that reference is applied to a dom element then no problem.

If that reference is applied to a component then the this.something would be a pointer to that component and any other dom element inside that component will have to have its own reference (let's say <input ref={(e) => { this.theinput = e; }}. Now to access it from your parent class you need to call this.something.theinput or this.something.refs['theinput'] (can't remember the API from the top of my head).

If this component is also connected using react-redux you will need to take an extra step when connecting the component connect(null, null, null, { withRef: true }) more info here

If you just console.log(this.input) you will get a javascript Object not a dom element because you're pointing it to a component which is connected to the redux store via redux-form.

[sandy0201]

Hi @andrevenancio , thanks so much for your detailed explanation, will have a look at my code again and try it out. :)

[bradencanderson]

We want to deprecate it eventually (not right now) because it blocks certain improvements in React in the future.

@gaearon -- mind giving an update on this? Are you all still planning on removing findDOMNode but keeping ref callbacks? It seems like based on trueadm's PR here that React might be going in a completely different direction.

[esr360]

For what it's worth, I subscribed to this thread because I believed I had a need for using findDOMNode and was interested in updates. After becoming more experienced and educated in React I was sure enough able to use callback refs to achieve what I wanted.

https://stackoverflow.com/questions/51512130/what-is-the-best-way-to-call-an-external-function-on-the-dom-element-rendered-by

[bradencanderson]

Yup, for sure. We’re having a discussion internally about whether they’re actively harmful. Right now it feels like a “no”, but curious what others think.

…

On Tue, Aug 28, 2018 at 4:59 PM Edmund ***@***.***> wrote: For what it's worth, I subscribed to this thread because I believed I had a need for using findDOMNode and was interested in updates. After becoming more experienced and educated in React I was sure enough able to use callback refs to achieve what I wanted. https://stackoverflow.com/questions/51512130/what-is-the-best-way-to-call-an-external-function-on-the-dom-element-rendered-by — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#678 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE029fXa7sOylrcSSSGKvBiZia4YuSr0ks5uVdlpgaJpZM4JKz4R> .

[maulerjan]

The problem is when you need to access a DOM element nested inside a component exported by 3rd party library. Then you have absolutely no other option than to use findDOMNode.

[rhys-vdw]

@maulerjan That's not a problem. There are many times where linter rules are set to communicate that this is not the preferred style, and then require a disable comment.

For example:

handleClick = () => {
  // NOTE: This library does not expose an API for getting a reference to its internal DOM node.
  // See the issue I've opened at https://github.com/some-person/third-party-component/issues/64
  // eslint:disable-next-line:react/no-find-dom-node
  const element = ReactDOM.findDomNode(this.thirdPartyComponentRef.current)
  alert(element.tagName)
}

[andrewplummer]

Just to add my use case for findDOMNode, it seems that testing semantic-ui-react components with enzyme using refs doesn't work... semantic for some reason feels the need to wrap refs in an HOC that enzyme can't mount.

I realize this is library interop and not React's fault, but I've already torn my hair out over this one too long and findDOMNode gets the job done nicely.

[andrewplummer]

Sorry scratch that it was my mistake... just refactored my code completely to be off findDOMNode!

[xgqfrms]

eslint error && findDOMNode

error Do not use findDOMNode react/no-find-dom-node

https://stackoverflow.com/questions/40499267/react-dnd-avoid-using-finddomnode

React Hooks bug

bug