jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.99k stars 2.77k forks source link

Rule proposal: warn against using findDOMNode() #678

Closed gaearon closed 8 years ago

gaearon commented 8 years ago

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.

josoriouz commented 7 years ago

I'm trying to stop using findDOMNode, but I need to get the getComputedStyle but I can't figure it out how. Does anybody have a suggestion???

leosco commented 7 years ago

I'm fully behind this change, but I want to point out that doing something like this:

<input type="text" ref={node => this.node = node} placeholder="Add a new task" />

triggers a [eslint] no-return-assign warning.

webdif commented 7 years ago

@leosco Indeed. We can do this to avoid eslint errors:

<input type="text" ref={(node) => { this.node = node; }} placeholder="Add a new task" />

No warning, but maybe less readable here. Better to keep the example simple, I think 🙂

ljharb commented 7 years ago

@leosco The latter is correct; in the former, you're using the implicit return form and conflating "returning" with "assignment". The latter example uses the explicit return form and thus is more readable.

oleksandr-shvets commented 7 years ago

Node, Node, Node... I think, we should avoid duplication, like this:

<input type="text" ref={_ => this.node = _} placeholder="Add a new task" />
ljharb commented 7 years ago

Now you're just duplicating the underscore, and still misusing the implicit return form of the arrow.

Andarist commented 7 years ago

as to HOC accessing a ref of WrappedComponent:

is there any convention how layers of HOCs should be able to access this ref? im thinking about passing down wrapperRef function with each HOC just hooking into it

basically each would check for wrapperRef in props, if not there then create one and use, if already available, just create a new one wrapping previous one, the chain would be automatically stopped on the WrappedComponent itself, as its not HOC itself

the one downside I can think of is that the WrappedComponent gets wrapperRef in its props, so its a little bit leaky

wondering if there is any established convention for this kind of thing, basically I need to A and B to have access to C’s ref in compose(A, B)(C) pattern

Would be easier if we could access ref prop, but React doesnt allow for this, could be achieved with each interoperable HOC simply extending the props:

const props = {
    ...this.props,
    ref: !this.props.ref
        ? ref => this.ref = ref
        : ref => {
          this.ref = ref
          this.props.ref(ref)
        },
}

Which would also allow for a rendered component to do the same and pass there a ref of some other Component or DOM node, useful for accessing inputs etc without need of creating extra special prop like innerRef or something

cc @gaearon

yonatanmn commented 7 years ago

I'm trying to do some style manipulation since flex-box, columns and other css properties are not good enough. Basically I have <Layout> component, and many <Widgets> as children, or grand children. I want to give those widgets styles from one point - Layout, since I need to take them all under consideration when computing each style. Since they are not direct children of Layout, and I want to have free use of them, what alternatives do I have ?

=== EDIT: === actually came up with this solution, not fully working yet -

class Layout extends Component {
  getChildContext() {
    return {
      passRefToLayout: this.collectWidgetRefs
    };
  }
  collectWidgetRefs(r){ this.widgetsRefs.push(r); }
  componentDidMount(){ 
    this.widgetsRefs.map(w => w.getBoundingClientRect) 
   // ...
  }
}

function Widget(props: Props, context): Element<any> {
  return (
    <div ref={context.passRefToLayout}>
      {props.children}
    </div>
  );
}

the problem is that when some widgets removed, it's not removing the from the array obviously. I guess using context here is "more react way", but searching the DOM instead every time I want to style is more functional.

If you have any thoughts would love to hear.

minmingsheng commented 7 years ago

@gaearon Assuming I have a third-party(or cannot be changed/refactored) component which contain

:

return(
<div className="component001">
   ......
</div>
)

But it does not have either ref="string" or ref={this.prop.cbRef}. Then I am having following structure in my <widget001> component:

//widget 001
...
if(condition){
return(
     <component 003>
         <component 002 /> {/*it contains <component001>*/}
     </component 003>
}else{
    <component 003>
           <component 001 {...api}/>
    </component 003>
}
)
...

Neither nor have real DOM. My goal is to find <div className="component001"></div> node in <component001> In this situation what should I do to avoid using findDOMNode(component001Node)? Any advice or example link would be appreciated.

ljharb commented 7 years ago

Find a ref that your wrapper Component sets in its own div, and traverse down from there.

budarin commented 6 years ago

@gaearon refs is not apply-able for stateless components in HOCs and extra div would break design among inline elements.

ljharb commented 6 years ago

A ref is state; stateless components wouldn't need refs. Do you have an example?

budarin commented 6 years ago

@ljharb

render() {
  if (someConditions) {
    return this.props.children; // or TargetComponent in HOC
  }
  if (this.props.PlaceHolder) {
    return <PlaceHolder />;  // PlaceHolder is or maybe stateless component
  }
  return null;  // <span /> - here we can get ref
}

How to get ref with callback? someConditions - conditions are calculated based on getBoundingClientRect

ljharb commented 6 years ago

I think the general idea, is that if you want the ref of a component, it should expose it explicitly via a prop callback, rather than letting you reach in from outside.

If you can't wrap it safely in a div, and it's not exposing a ref, then I'd say you may want to consider finding an alternative solution.

taion commented 6 years ago

BTW see https://github.com/facebook/react/issues/11401

So unless that's changed, that likely will be the future – DOM components implement a hostRef plain prop that calls back with the DOM node.

JohnWeisz commented 6 years ago

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.

amethyst82 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

It's ok now, got it to work.

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

andrevenancio commented 6 years ago

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 commented 6 years ago

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

bradencanderson commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 notifications@github.com 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 https://github.com/yannickcr/eslint-plugin-react/issues/678#issuecomment-416778536, or mute the thread https://github.com/notifications/unsubscribe-auth/AE029fXa7sOylrcSSSGKvBiZia4YuSr0ks5uVdlpgaJpZM4JKz4R .

maulerjan commented 6 years ago

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 commented 6 years ago

@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)
}
OZZlE commented 5 years ago

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

This causes eslint error: error Arrow function should not return assignment no-return-assign @gaearon

Is it eslint infinit loop? :D

giankotarola commented 5 years ago

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

This causes eslint error: error Arrow function should not return assignment no-return-assign @gaearon

Is it eslint infinit loop? :D

@OZZlE add braces to the function in inputRef prop:

<Field inputRef={node => { this.inputNode = node }} />
OZZlE commented 5 years ago

@giankotarola aha it was that simple! _D thank you! I think the error messages from eslint can be kind of cryptic often..

I find it strange that in a proposal you make an example that doesn't pass eslint :P perhaps it was before that rule came to life? also not sure how this is off-topic when it's about this proposed (now accepted) rule and how to work with it??

ljharb commented 5 years ago

@OZZlE it's off-topic because it's got nothing to do with warning against using findDOMNode. It's an entirely different rule that was triggered. I'm going to hide these, too - please file a new issue to discuss it further instead of pinging everyone on this thread.

andrewplummer commented 5 years ago

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 commented 5 years ago

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

xgqfrms commented 4 years ago

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

image

bug

image