Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

findDOMNode deprecation #14357

Open
eps1lon opened this issue Nov 29, 2018 · 20 comments
Open

findDOMNode deprecation #14357

eps1lon opened this issue Nov 29, 2018 · 20 comments

Comments

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 29, 2018

Timeline

  1. <= 16.3: findDOMNode is discouraged but accepted for certain use cases
  2. 16.3 (2018-03-28): forwardRef is introduced:
    It can be used in HOCs to avoid using findDOMNode on the enhanced component
  3. 16.6 (2018-10-23): findDOMNode is deprecated in React.StrictMode
  4. 16.7.alpha (2018-10-24): React.Concurrent mode is released:
    This mode extends React.StrictMode in a way that findDOMNode is deprecated in that mode too.
  5. 16.8 (Q2 2019): stable React.Concurrent mode

findDOMNode use cases

If you have more use cases please let me know. I only started with some examples from mui-org/material-ui.

with a planned alternative

State of forwardRef

react has 3.4M downloads/week.

hoist-non-react-statics (3.9M downloads/week; not clear what percentage is 2.x)

A utility mainly used in HOCs and encouraged to use in the official react docs. However everyone stuck at 2.x will likely encounter issues with forwardRef since that version
does not handle any react@^16.3 features. ^3.2.0 should have no issues apart from some minor
issues with propTypes hoisting from forwardRef to forwardRef. The latest stable from zeit/next still uses that outdated version. However the latest canary for 7.0.3 does not.

react-docgen (400k downloads/week)

Not recognized as a valid component definition. PR open at reactjs/react-docgen#311.

react-redux (1.4M downloads/week)

connect does properly forward their refs in the beta release of 6.x. No timeline for stable release given
however 3 betas have already been released so it's probably soon.

react-router (1.4M downloads/week)

withRouter is planned to forward refs (ReactTraining/react-router#6056#issuecomment-435524678).
However no comment about the other components and no major release candidate is published.

display name

React.forwardRef components are recognized by react-devtools. However when wrapped
in a HOC it's very likely that the display name is lost. See facebook/react#14319

The issue

Assumptions:

  • you are not in control of your whole component tree i.e. you use components from 3rd party libraries
  • you want to use React.ConcurrentMode
  • Usable includes production and development. It specifically means for development that deprecation warnings in a component
    make that component not usable in development mode because of all the noise it adds in those cases.
    Noise because it's not actionable if that component is from a 3rd party library.

If none of those applies to you then you probably don't have an issue with findDOMNode deprecation.

The mode of a partial tree can only be made more restrictive but not loosened up. If
you wrap your tree in React.StrictMode and use a component from a 3rd party library
that 3rd party library has to be React.StrictMode compliant too.

This means that you can't use React.StrictMode effectiveley. This might be ok since it's for development only anyway and has no implications for production. However Concurrent mode can have actual implications for production. Since it is new and the community wants to use new things libraries have to make sure that they are strict mode compliant too.

In addition between the relase of an alternative in the form of React.forwardRef and the deprecation only 7 months have passed. One could argue that this is plenty of time but (at least from my perspective) the work on migrating from findDOMNode to refs and forwardRef was postponed because findDOMNode was not deprecated yet. However the actual deprecation happened one day before the release of unstable_ConcurrentMode virtually giving no time to migrate. We'll have to see when a stable 16.7 release will happen but assuming this happens today only a month has passed between deprecation and virtual removal. React 16.x Roadmap was release pointing towards Q2 2019 as a release date of stable React.Concurrent mode. This relaxes pressure for library maintainers quite a bit IMO.

Conclusion

Refs are not a viable upgrade path to replace findDOMNode yet.
Until refs are usable without headaches from forwarding refs findDOMNode should be undeprecated.

Releated

@Jessidhia
Copy link
Contributor

styled-components used findDOMNode as a debug assist to verify if your wrapper component does eventually forward the className to a DOM node.

However, as is the case with all of these uses of findDOMNode, it'll only work correctly if only one DOM Element is eventually rendered, or if the appropriate element is the first one rendered. Any fragment siblings are completely invisible to it. This is why it's deprecated.

@gaearon
Copy link
Collaborator

gaearon commented Dec 4, 2018

As I commented in styled-components/styled-components#2154 (comment), let's track this issue to enumerate use cases that are addressed by findDOMNode but not by refs. Please comment with a small demo and a description of what and why you're trying to do. Note that it takes time and effort to understand your particular problem so please try to explain what kind of API you're trying to implement. Don't assume we're familiar with your library. Thanks!

@Fer0x
Copy link

Fer0x commented Dec 4, 2018

I have a project - visual editor for composing layouts with React components ui library by dragging components with mouse.

This React components are developed by different team so we don't have a direct access to modify their codebase and for now passing refs to that components is not supported. And we can use different React UI libraries by different developer because with findDOMNode we can work with them universally independent of supporting props with forwarding refs .

In our project codebase, we wrap each of that components with several HOCs. Each HOC contains some code with findDOMNode - for adding event listeners like click/mousemove or for using getBoundingClientRect() method.

So findDOMNode is the only option for us.

@Fer0x
Copy link

Fer0x commented Dec 4, 2018

@gaearon

About styled-components:

findDOMNode used only in dev environment for warn users of incorrect using of styled-components. It's very helpful feature, because a lot of beginners stumble over this issue.

Details of usage:
That's issue can be reproduced when custom React component is passed to styled() factory. In this case styled-components create className and pass it as prop to that custom React component. If component is not supposed of supporting className prop, warn of incorrect usage should be thrown.
So, in styled-components code, we need to find DOM node with generated className.
If it exist then everything is OK. But we can't get our users to use ref forwarding on each their component. We need universal approach to finding child DOM node, that's why findDOMNode is used.

@sag1v
Copy link

sag1v commented Dec 4, 2018

A use case that i think of, is when you want to perform some logic on the underline DOM element of the child.
For example, a component that wraps any other component and traps events to check if its "outside" of this child. Like click or mouseover etc.

This is the relevant logic that depends on the ref:

handleEvent = ({ target }) => {
  const trapped = this.ref.contains(target);
  this.setState({ trapped });
};

With findDOMNode its easy and flexible to get a reference to the child no matter what it is. let it be a function, a class or a text.

componentDidMount() {
  const { event } = this.props;
  this.ref = ReactDOM.findDOMNode(this);
  document.addEventListener(event, this.handleEvent);
}

The alternative (hooks aside) is to expose the ref callback to the user (via render prop for example):

render() {
  const { children } = this.props;
  const { trapped } = this.state;
  return children(trapped, ref => (this.ref = ref));
}

And the user will attach it to what ever DOM element he/she wants

{(trapped, refCallback) => (
  <div ref={refCallback} >
    <div>{trapped ? "open" : "close"}</div>
  </div>
)}

This will work but the API is kind of awkward, plus we shift the responsibility for dealing with refs to the end user (sort of implementation details).

A more critical problem with this API is that it may conflict with other components that are using this approach.

Consider a second library like ElementResize that needs a ref to calculate some resize logic on it, and this library is also exposing a ref callback.

<ElementResize>
  {(rect, refCallback) => (
    <div ref={refCallback}>
      <div>{rect.width}</div>
    </div>
  )}
</ElementResize>

If a user wants to use both libraries, he/she can't use both callbacks on the same element (again, hooks aside):

<ElementResize>
  {(rect, refCallback1) => (
    <Trap event="click">
      {(trapped, refCallback2) => (
        <div ref={???}> <--- can't use 2 ref callbacks on the same element
          <div> {trapped ? 'focused' : 'blur'}</div>
          <div>{rect.width}</div>
        </div>
      )}
    </Trap>
  )}
</ElementResize>

I made a running example of the 2 approaches

A ref on a fragment can solve these issues as the library can render a "transparent" element yet keep a ref without the end user has to know or worry about it.

@Jessidhia
Copy link
Contributor

Jessidhia commented Dec 5, 2018

This use case is similar to the use case of react-transition-group. I don't remember the architecture, but either TransitionGroup or CSSTransition use findDOMNode to get a reference to the underlying DOM node, so they can apply CSS classes based on the transition state and even delay the React unmount when the exit state is present.

The delaying is done, I believe, with cloneElement; but the className manipulation still requires findDOMNode.

@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2018

@Fer0x Thanks for explaining the styled-component use case. I replied with some alternative ideas in styled-components/styled-components#2154 (comment), happy to continue the discussion about that particular use case there.

@TrySound
Copy link
Contributor

TrySound commented Dec 7, 2018

I'd say this api is quite nice. I actually would pass ref to tooltip via props which is the easiest way to reuse this ref in another component or hook.

@liuyangc3
Copy link

liuyangc3 commented Dec 14, 2018

in some case, if needs to get postion of a component which is from a third lib.
first I need to use findDOMNode to get the DOM element from this component,
so I can call getBoundingClientRect method somewhere later.

I think that forwardRef can't doing this.

@TrySound
Copy link
Contributor

@liuyangc3 reactjs/rfcs#97

@liuyangc3
Copy link

@Kovensky thanks, I will look into this

@noah79
Copy link

noah79 commented Dec 2, 2019

I have a use case where I've wrapped the excellent @bvaughn 's react-window components with my own in order to force repainting on mousewheel scroll. To do this I do the following:

render() {
    const {props: {syncScroll, ...props}} = this
    return <FixedSizeList {...props} ref={this.setListRef} />
  }

  scrollNode: HTMLDivElement

  unregisterScrollEvent = () => {
    this.scrollNode && this.scrollNode.removeEventListener("wheel", this.onWheel)
    this.scrollNode = null
  }

  list: FixedSizeList

  setListRef = (list: FixedSizeList) => {
    this.list = list
    if (this.props.syncScroll) {
      if (!list) {
        this.unregisterScrollEvent()
      }
      else {
        this.scrollNode = ReactDOM.findDOMNode(list) as HTMLDivElement

        this.scrollNode.addEventListener("wheel", this.onWheel)
      }
    }
  }

How would you suggest I approach this instead?

@azuruce
Copy link

azuruce commented Feb 16, 2020

I have a use case that the tooltip didn't properly be tethered to menu item inside dropdown that is tethered to a dropdown button. We used Tether and plan to move to Popper but it is not clear Tether is the cause. We found that we can use MutationObserver DOM API to detect DOM mutation and force Tether's global position handler to run and solve this problem. However, we want the MutationObserver to observe a smaller subtree of DOM below a "node", instead of the whole DOM. To do this, we will need to call observe(node) and we need to use findDOMNode to get the node.

@esr360
Copy link

esr360 commented Apr 11, 2020

I have a problem that I have so far not been able to solve using refs, but can solve very nicely with findDOMNode.

I have a fairly complex custom library, but for brevity it can be reduced to the following:

import React from "react";

export default function App() {
  return (
    <Module styles={{ background: 'lightgrey' }}>
      <Module styles={{ color: 'red' }}>Some div</Module>
      <Module styles={{ color: 'blue' }}>Another div</Module>
    </Module>
  );
}

class Module extends React.Component {
  constructor(props) {
    super(props);
    this.REF = React.createRef();
    this.TAG = props.as || 'div';
  }

  componentDidMount() {
    this.paint(this.REF.current, this.props.styles);
  }

  // in reality this.paint() does much more than re-construct the 
  // input styles object as shown here
  paint(node, styles = {}) {
    for (const [prop, val] of Object.entries(styles)) {
      node.style[prop] = val;
    }
  }

  render() {
    return (
      <this.TAG ref={this.REF}>{this.props.children}</this.TAG>
    );
  }
}

CodeSandbox link

Essentially, within componentDidMount of my custom <Module> Component, I need the underlying DOM node so I can pass it to this.paint() (which requires the underlying DOM node).

So far, this works fine using refs. The issue occurs when attempting to pass other React Components to the as prop, so that this.TAG is some other React Component instead of a div.

In my case, I am trying to pass the Components from Pure React Carousel to my custom <Module> Component. The relevant part from the above code would now be:

...
import { CarouselProvider, Slider, Slide } from 'pure-react-carousel';
import 'pure-react-carousel/dist/react-carousel.es.css';

export default function App() {
  return (
    <CarouselProvider naturalSlideWidth={4} naturalSlideHeight={4} totalSlides={2}>
      <Module as={Slider} styles={{ background: 'lightgrey' }}>
        <Module as={Slide} styles={{ color: 'red' }}>Slide 1</Module>
        <Module as={Slide} styles={{ color: 'blue' }}>Slide 2</Module>
      </Module>
    </CarouselProvider>
  );
}

...

CodeSandbox link

The code inside the componentDidMount() method of <Module> now breaks because this.REF.current now points to the host Component instead of the DOM node. This can be fixed by changing the componentDidMount() method to:

componentDidMount() {
  this.paint(findDOMNode(this.REF.current), this.props.styles);
}

CodeSandbox link

I would love to know if what I am attempting can be achieved with only refs. Thanks.

@simonguo
Copy link

There is an application scenario, there is a component Overlay, which needs to be attached to a third-party component and obtain the position of the DOM element of the third component, such as:
https://github.com/react-bootstrap/react-bootstrap/blob/master/src/OverlayTrigger.js#L122

@stale
Copy link

stale bot commented Jul 12, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 12, 2020
@esr360
Copy link

esr360 commented Jul 13, 2020

Big fat bump, stale bot sucks

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 13, 2020
@stale
Copy link

stale bot commented Oct 12, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added Resolution: Stale Automatically closed due to inactivity and removed Resolution: Stale Automatically closed due to inactivity labels Oct 12, 2020
@IMalyugin
Copy link

We are using an internal-developed WYSIWYG as a developer framework and cms. It's entire logic is written around findDomNode. Basically all the reusable (random-sourced) components, are wrapped around with an abstract 'wrapper', that attaches all kids of handlers from mouseEnter to drop to the domNode created by that component. As a result, when hovering any component in the tree, you can bubble up to closest library ui-component, then you build a node tree and can modify it separately under any resolution, ab-testing condition or any other context.

This case has been stated quite a few times and it's entirely impossible to do via refs. IMO wysiwyg cms as the development tool is actually the next step in applying redux to build web-applications, as the CMS itself may take away all the extra logic, adding editor-layers, such as: Adaptivity, Language, ContentMapping, Extra Styling, AB-Testing, WebAnalytics, WebOptimization etc, and even move e2e testing to a whole new level, by allowing to record testing cases with attachment to concrete nodes.

So we're sincerely hoping findDOMNode will always be a viable option. Moving it into a separate package but still supporting in react, just like prop-types seems like the best options here.

@robatwilliams
Copy link

Fragment event handlers (#12051) would be good to have. It would allow replacing findDOMNode() usages that are only done to add event listeners without needing to add an extra DOM element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests