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

Rule proposal: warn against using findDOMNode() #678

Closed
gaearon opened this Issue Jul 12, 2016 · 127 comments

Comments

Projects
None yet
@gaearon
Copy link
Collaborator

gaearon commented Jul 12, 2016

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

This comment has been minimized.

Copy link

timdorr commented Jul 12, 2016

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

This comment has been minimized.

Copy link
Collaborator Author

gaearon commented Jul 12, 2016

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

This comment has been minimized.

Copy link

vladshcherbin commented Jul 12, 2016

@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

This comment has been minimized.

Copy link

notaurologist commented Jul 12, 2016

@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

This comment has been minimized.

Copy link

timdorr commented Jul 12, 2016

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

@notaurologist

This comment has been minimized.

Copy link

notaurologist commented Jul 12, 2016

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

This comment has been minimized.

Copy link

PEM-- commented Jul 12, 2016

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

This comment has been minimized.

Copy link

soyarsauce commented Jul 12, 2016

@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

This comment has been minimized.

Copy link

fhelwanger commented Jul 13, 2016

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

This comment has been minimized.

Copy link

alaindresse commented Jul 13, 2016

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jul 13, 2016

@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

This comment has been minimized.

Copy link

okonet commented Jul 13, 2016

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

This comment has been minimized.

Copy link

koenpunt commented Jul 13, 2016

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

this.inputNode.scrollIntoView();

Should be

this.inputNode.focus();
@PEM--

This comment has been minimized.

Copy link

PEM-- commented Jul 13, 2016

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

This comment has been minimized.

Copy link
Collaborator Author

gaearon commented Jul 13, 2016

@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

This comment has been minimized.

Copy link

alaindresse commented Jul 13, 2016

@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

This comment has been minimized.

Copy link
Collaborator Author

gaearon commented Jul 13, 2016

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

This comment has been minimized.

Copy link
Collaborator Author

gaearon commented Jul 13, 2016

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

This comment has been minimized.

Copy link

PEM-- commented Jul 13, 2016

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

This comment has been minimized.

Copy link
Contributor

Andarist commented Jul 13, 2016

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

This comment has been minimized.

Copy link

PEM-- commented Jul 13, 2016

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

@fhelwanger

This comment has been minimized.

Copy link

fhelwanger commented Jul 13, 2016

@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

This comment has been minimized.

Copy link

fhelwanger commented Jul 13, 2016

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

This comment has been minimized.

Copy link
Owner

yannickcr commented Jul 14, 2016

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

@yannickcr yannickcr added the new rule label Jul 14, 2016

@yannickcr yannickcr referenced this issue Jul 17, 2016

Closed

Roadmap v6.0.0 #686

9 of 9 tasks complete

@yannickcr yannickcr closed this in 55b9cbe Jul 19, 2016

@JohnWeisz

This comment has been minimized.

Copy link

JohnWeisz commented May 15, 2018

I think findDOMNode has valid use-cases, such as when direct DOM-manipulation or readings are necessary but a ref is not necessarily feasible or even available (such as in the case of a child component that you don't control).

In this case, you could easily create a HOC to facilitate the acquisition of the DOM-node itself, but it seems overly complicated just to acquire the underlying DOM-node -- it's almost like generating unique ids and using document.getElementById.

Instead, how about warning against the use of findDOMNode only if it's used to acquire a DOM-node rendered by same component it is used from?


i.e. this is bad:

class MyComponent extends React.Component
{
    render()
    {
        return (
            <div>
                ...
            </div>
        );
    }

    componentDidMount()
    {
        ReactDOM.findDOMNode(this);
    }
}

but this is good:

class MyComponent extends React.Component
{
    render()
    {
        return (
            <div>
                <MyChildComponent ref={inst => this._childInst = inst} />
            </div>
        );
    }

    componentDidMount()
    {
        ReactDOM.findDOMNode(this._childInst);
    }
}

Note: this is primarily in the case when MyChildComponent is not controlled by me, e.g. it's from a library, and I cannot just add a divRef prop to it.

rrdelaney pushed a commit to rrdelaney/reason-scripts that referenced this issue May 23, 2018

Update eslint-plugin-react and enable new rules (#696)
* Update eslint-plugin-react and enable new rules

New rules:
* `react/no-danger-with-children` (yannickcr/eslint-plugin-react#710)
* `react/no-find-dom-node` (yannickcr/eslint-plugin-react#678)
* `react/style-prop-object` (yannickcr/eslint-plugin-react#715)

* Remove react/no-find-dom-node for now

@interactivellama interactivellama referenced this issue May 23, 2018

Merged

React-Modal upgrade to 3.4.4 #1391

4 of 10 tasks complete
@amethyst82

This comment has been minimized.

Copy link

amethyst82 commented Jun 14, 2018

How can I do this after componentDidMount without findDomNode?
scenario is like this:

  1. after mount and render
  2. user input something into the UI form
  3. the page should be verified and focus on the first error field.
    I use findDomNode can implement this scenario but not the current ref solution
@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented Jun 14, 2018

on componentDidMount whatever references you added in your render method they'll be accessible to you.

Its then up to you to give focus to the element you want this.holder.focus().

import React,  { PureComponent } from 'react';

class Example extends PureComponent {
    componentDidMount() {
        console.log(this.holder); // will return the DOM element.
    }

    render() {
        return (
            <input ref={(e) => { this.holder = e; }} type="text" />
        );
    }
}

export default Example;
@sandy0201

This comment has been minimized.

Copy link

sandy0201 commented Jul 26, 2018

Hi @gaearon , I am currently using React v16.2.0, and Redux-Form (v7.2.0) in combination with React-Number-Format (v3.1.3). I am trying to focus a field on button click.

When I use ref={(input) => { this.input = input; }} on the element and this.input.focus() it throws an error saying that this.input.focus() is not a function. But when I use ref={(input) => { this.input = ReactDOM.findDOMNode(input); }} then the focus works.

ref={(input) => { this.input = input; }} does work in some places and some it doesn't.

Do you perhaps know why this is happening or any suggestions?

Thank you.

Here's my code snippets from various files linked together:
enter-value.js

...
   focusField = () => {
        console.log(this.input);
        console.log(this.input.children);
        this.input.focus();
    }

    render() {
        return (
                <div className="col-md-12">
                    <form onSubmit={handleSubmit(this.callApi)}>
                        <Textbox
                            inputRef={(input) => { this.input = input; }}
                            name="customField"
                            className="form-control"
                            placeholder={copy.placeholder}
                            maxlength="16"
                            onChange={(e) => {
                                errCode = '';
                                this.setState({
                                    fieldValue: e.target.value.replace(/[\s]/g, ''),
                                    error: '',
                                });
                            }}
                            onKeyPress={(e) => { if ((e.key === 'Enter' && !e.target.value) || e.key === ' ') e.preventDefault(); }}
                            error={this.state.error}
                            autoComplete="off"
                            autoFocus="autofocus"
                            numberType="true"
                            fieldFormat="###### #### ## #"
                        />
                        <Button
                            className="btn btn-solid"
                            type="submit"
                            btnRef={(btn) => { this.btn = btn; }}
                            value={buttonTranslation.next}
                            name="next"
                        />
                    </form>
                    <Modals screen={this.state.screen} errorCode={this.state.errorCode} error={errorObj} focusField={() => this.focusField()} reset={() => this.reset()} />
                </div>
        );
    }
...

textbox.js

import NumberFormat from 'react-number-format';
...
renderNumberField = (field) => {
        const { meta: { error } } = field;
        const className = `form-group ${error || field.error ? 'has-error' : ''}`;

        return (
            <fieldset className={className}>
                <NumberFormat
                    {...field.input}
                    {..._.omit(field, [
                        'input',
                        'meta',
                        'maxlength',
                        'inputRef',
                        'inputValue',
                        'empty',
                        'rule',
                        'error',
                        'numberType',
                        'fieldFormat',
                    ])}
                    ref={field.inputRef}
                    format={field.fieldFormat}
                />
                {(error || field.error) && <p className="help-block">{!error ? field.error : error}</p>}
            </fieldset>
        );
    }

    render() {
        return (
            <Field
                {..._.omit(this.props, [])}
                component={this.renderNumberField}
            />
        );
    }
...

modals.js

...
    render() {
        return (
            <Modal
                id={this.id()}
                header={{
                    icon,
                    heading: this.heading(enterIdTranslation, idvResultsTranslation),
                }}
                body={(<div className="btn-container clearfix">
                    <Button
                        type="button"
                        className="btn btn-contour pull-right"
                        name="changeId"
                        onClick={() => { this.reset(); this.props.focusField(); }}
                        data-dismiss="modal"
                        value={buttonTranslation.changeId}
                    />
                </div>)}
                footer={this.footer(buttonTranslation)}
            />
        );
    }
...
@sandy0201

This comment has been minimized.

Copy link

sandy0201 commented Jul 26, 2018

It's ok now, got it to work.

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

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented Jul 26, 2018

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

This comment has been minimized.

Copy link

sandy0201 commented Jul 27, 2018

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

arkhaix added a commit to arkhaix/lit-reader that referenced this issue Aug 27, 2018

@bradencanderson

This comment has been minimized.

Copy link

bradencanderson commented Aug 28, 2018

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

This comment has been minimized.

Copy link

esr360 commented Aug 28, 2018

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

This comment has been minimized.

Copy link

bradencanderson commented Aug 29, 2018

@maulerjan

This comment has been minimized.

Copy link

maulerjan commented Sep 7, 2018

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

This comment has been minimized.

Copy link

rhys-vdw commented Sep 7, 2018

@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 http://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)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment