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

Deprecate findDOMNode in StrictMode #13841

Merged
merged 4 commits into from Oct 12, 2018

Conversation

Projects
None yet
@sebmarkbage
Member

sebmarkbage commented Oct 12, 2018

Motivation

The main motivation is that findDOMNode is breaking abstraction levels but allowing a parent to reason about what kind of children a component might render. It creates a refactoring hazard where you can't change the implementation details of a component because a parent might be reaching into its DOM node.

Attaching explicit refs is the alternative. The main thing that used to be difficult is creating a higher order component that seamlessly work with its children. This can now be achieved using forwardRefs.

Up until this point it was a loose recommendation that you don't use this functions but there are certain technical details that are twisting our hands to want to deprecate it.

  • In the Fiber architecture it is not possible to cache the "current" node at a higher level since at any given point there are several possible future trees. This causes the findDOMNode algorithm to turn into a very complicated and possibly slow search algorithm in bad cases.
  • In the compiler project, it becomes difficult to know if any given DOM node might need to be found. That forces us to replicate the tree structure just in case someone calls it. We'd rather not support this. Instead only explicit refs will be the ones that gets materialized.

We'll only deprecate this in StrictMode for the foreseeable future. It won't get deleted for a long time because lots of existing code depends on it. This is more about preventing new code being written using this outdated technique.

PR

There are two scenarios. One is that we pass a component instance that is
already in strict mode or the node that we find is in strict mode if
an outer component renders into strict mode.

I use a separate method findHostInstanceWithWarning for this so that
a) I can pass the method name (findDOMNode/findNodeHandle).
b) Can ignore this warning in React Native mixins/NativeComponent that use this helper.

I don't want to expose the fiber to the renderers themselves.

@TrySound

This comment has been minimized.

Contributor

TrySound commented Oct 12, 2018

Does it mean all possible usages of findDOMNode are deprecated or just some cases?

@sizebot

This comment has been minimized.

sizebot commented Oct 12, 2018

Details of bundled changes.

Comparing: 0af8199...665596c

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.3% +0.2% 656.95 KB 658.77 KB 153.51 KB 153.82 KB UMD_DEV
react-dom.development.js +0.3% +0.2% 652.29 KB 654.12 KB 152.13 KB 152.44 KB NODE_DEV
ReactDOM-dev.js +0.3% +0.2% 669.21 KB 671.37 KB 152.73 KB 153.07 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.5% +0.4% 374.35 KB 376.14 KB 80.83 KB 81.16 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 52.68 KB 52.74 KB 15.8 KB 15.82 KB NODE_PROD
react-reconciler-persistent.development.js +0.5% +0.4% 372.96 KB 374.75 KB 80.26 KB 80.6 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 52.7 KB 52.75 KB 15.8 KB 15.82 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.4% +0.3% 506.42 KB 508.65 KB 111.76 KB 112.11 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.4% +0.3% 506.13 KB 508.36 KB 111.68 KB 112.03 KB RN_OSS_DEV
ReactFabric-dev.js +0.4% +0.3% 496.59 KB 498.81 KB 109.33 KB 109.69 KB RN_FB_DEV
ReactFabric-dev.js +0.4% +0.3% 496.62 KB 498.85 KB 109.35 KB 109.71 KB RN_OSS_DEV

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 12, 2018

Only in strict mode — yes, all usage.

Forwarding refs should be sufficient.

Deprecate findDOMNode in StrictMode
There are two scenarios. One is that we pass a component instance that is
already in strict mode or the node that we find is in strict mode if
an outer component renders into strict mode.

I use a separate method findHostInstanceWithWarning for this so that
a) I can pass the method name (findDOMNode/findNodeHandle).
b) Can ignore this warning in React Native mixins/NativeComponent that use this helper.

I don't want to expose the fiber to the renderers themselves.
false,
'%s is deprecated in StrictMode. ' +
'%s was passed an instance of %s which is in a StrictMode subtree. ' +
'Use an explicit ref directly on the element you want to get a handle on.' +

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

How about this:

%s was passed an instance of %s which is inside StrictMode. Instead, add a ref directly to the element you want to reference.

Also, do we even need "which is inside StrictMode"? It or ConcurrentMode should always appear in the stack unless they're using createRoot.

false,
'%s is deprecated in StrictMode. ' +
'%s was passed an instance of %s which renders a StrictMode subtree. ' +
'The nearest child is in StrictMode. ' +

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

"The nearest child is in StrictMode." feels redundant

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

Yea, it is but I'm not confident people understand what that means so clarifying the mechanism might help.

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

I don't know that this is any clearer than the previous line though.

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

I changed to "StrictMode children". I think that's clearer and fits in one line.

methodName,
methodName,
componentName,
getStackByFiberInDevAndProd(fiber),

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

Should we pass hostFiber here too? Since you'll need to find it in order to change your code.

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

It should always be one the same stack since fiber is a child of hostFiber.

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

Other way around, right? They have the same ancestors, but the hostFiber stack is longer/deeper.

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

Ah. You mean for the instance case. I figured that you'd want to look at refactoring the top component and then work downwards. But I guess we could use the same stack for both.

const fiber = ReactInstanceMap.get(component);
if (fiber === undefined) {
if (typeof component.render === 'function') {
invariant(false, 'Unable to find node on an unmounted component.');

This comment has been minimized.

@sophiebits

sophiebits Oct 12, 2018

Member

I know these are the same, but dev-only invariants feel risky.

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 12, 2018

Member

yea see what you're saying but refactoring this function is a pita and makes it worse.

Once we fix RN to not use the special NativeMixin/NativeComponent, we can remove this special case and always warn with the same function.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Oct 12, 2018

Notably this doesn't warn if no node is found. E.g. called on a strict mode component that renders null. I think this is fine because I think the next path would be that we make this always return null for strict mode.

warningWithoutStack(
false,
'%s is deprecated in StrictMode. ' +
'%s was passed an instance of %s which renders a StrictMode children. ' +

This comment has been minimized.

@gaearon

gaearon Oct 12, 2018

Member

"a StrictMode children" sounds odd, maybe drop "a"?

@sebmarkbage sebmarkbage merged commit 4773fdf into facebook:master Oct 12, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@sag1v

This comment has been minimized.

sag1v commented Oct 13, 2018

Hi, Is there any discussion or RFC about deprecating findDOMNode? I'm not sure ref forwarding can solve all cases.

@oriSomething

This comment has been minimized.

oriSomething commented Oct 14, 2018

@sag1v I event wrote about it:
reactjs/rfcs#56

It'll just make things even more complicated

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Oct 16, 2018

I might open an RFC for <Fragment ref={arrayOfChildNodes => ...}>{children}</Fragment> which would solve some outstanding issues:

  1. It would have a callback that fires when any of them changes so you would have a way to detect when the child components changes.
  2. It only needs to know what the "current" children are during the commit phase. Not at arbitrary random times which makes the implementation much faster and more predictable.
  3. It's opt-in so React doesn't need to store or emit trees to support these for everything in the whole tree - just in case.
  4. It works with function components.
  5. It supports more than one child node (such as when fragments are returned from render).

What we do know is that the current API is broken and needs to be deprecated but we could potentially add a different API to support the one remaining use case.

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Oct 16, 2018

I haven't thought much about the Fragment-ref proposal, but initial reaction is that I like it, and can see myself using it.

That being said– it doesn't help libraries, since they'll be stuck between the old way of doing things (e.g. findDOMNode) that now warns and the newer way of doing things (e.g. forwardRef or Fragment refs) that doesn't work for older React versions.

This is like the gDSFP problem again. :)

@sag1v

This comment has been minimized.

sag1v commented Oct 16, 2018

@sebmarkbage This looks solid and totally address the use case i mentioned here.

@migueloller

This comment has been minimized.

migueloller commented Oct 19, 2018

@sebmarkbage, <Fragment ref={...}>{children}</Fragment> sounds like a powerful enough replacement for findDOMNode. We use findDOMNode to portal components to an <iframe> and want to keep the frame document clean but need to measure DOM nodes. Instead of wrapping components in a <div ref={...}>{children}</div> we use findDOMNode. <Fragment ref={...}>{children}</Fragment> would be perfect!

linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018

Deprecate findDOMNode in StrictMode (facebook#13841)
* Deprecate findDOMNode in StrictMode

There are two scenarios. One is that we pass a component instance that is
already in strict mode or the node that we find is in strict mode if
an outer component renders into strict mode.

I use a separate method findHostInstanceWithWarning for this so that
a) I can pass the method name (findDOMNode/findNodeHandle).
b) Can ignore this warning in React Native mixins/NativeComponent that use this helper.

I don't want to expose the fiber to the renderers themselves.

@gaearon gaearon referenced this pull request Oct 23, 2018

Merged

Add 16.6.0 changelog #13927

@eps1lon

This comment has been minimized.

eps1lon commented Oct 26, 2018

Only in strict mode — yes, all usage.

Seems like this is not currently the case (will explain later why I think this is good): https://codesandbox.io/s/qko6n6vzjj

It only warns if findDOMNode is called with a component. If one calls it with an element node (or nully) then it doesn't warn (because it simply returns the argument) because of an early return in

if ((componentOrElement: any).nodeType === ELEMENT_NODE) {
return (componentOrElement: any);
}

This is however very valuable for library maintainers that allow users to pass custom components e.g.

// library.js
// imagine it's a class component
function ({Component}) {
  return <Component ref={ref => doSomething(React.findDOMNode(ref))} />
}
// userland.js
<LibraryComponent Component={UserLandComponent} />

This way we can still test the library codebase for bad components that don't properly forward their refs and keep supporting users both in and out of React.StrictMode.

I know this is most likely a case of "Shouldn't have done that in the first place" but it would be really nice if we could support this use case.

If this was your intention I'll gladly add a test to avoid regression. Might be also good idea to amend the blog post.

@oliviertassinari

This comment has been minimized.

Contributor

oliviertassinari commented Nov 1, 2018

@sebmarkbage We are looking into migrating away from the findDOMNode() method on Material-UI. @eps1lon has started experimenting. Now, I'm wondering. forwardedRef is a great, it's definitely solving the higher-order component issue.

But how are we supposed to solve the composition problem?

Let's say we have the following React tree A > B > C:

  • A a class component.
  • B a class component.
  • C a simple div primitive.

How does A can get a ref on C without agreeing on some sort of "standard" property for cascading the reference? Like in the documentation using forwardedRef. So far, our answer on Material-UI is the RootRef component, but we can no longer use it with the depreciation of findDOMNode().

import React from 'react';
import RootRef from '@material-ui/core/RootRef';

class MyComponent extends React.Component {
  constructor() {
    super();
    this.domRef = React.createRef();
  }

  componentDidMount() {
    console.log(this.domRef.current); // DOM node
  }

  render() {
    return (
      <RootRef rootRef={this.domRef}>
        <SomeChildComponent />
      </RootRef>
    );
  }
}

It's a component we use internally for the Tootlip or that we can use for the ClickAwayListener.

⭐️ <Fragment ref={arrayOfChildNodes => ...}>{children}</Fragment> would be awesome!

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 1, 2018

Can you provide a sandbox with a tiny demo for your question? Not sure I get it.

@oliviertassinari

This comment has been minimized.

Contributor

oliviertassinari commented Nov 1, 2018

@sag1v

This comment has been minimized.

sag1v commented Nov 1, 2018

@oliviertassinari I have the exact same use case and now the API is forced to expose a ref. I've posted an example in this comment.
@gaearon I wish we could have another solution which won't suffer from the downsides findDOMNode has but will be able to provide the same capabilities. 🤞
findDOMNode allowed us to provide a much simpler API for consumers and without exposing implementation details like refs.

Edit
I think it's better to post the example here instead of a link to a comment (if you don't mind):

Sometimes we want to get the DOM node without caring what type of children we get (function, text, class or regular DOM element).

findDOMNode was the perfect tool for this task.

Given this component:

class Trap extends React.Component {
  state = {
    trapped: false
  };

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

  componentWillUnmount() {
    const { event } = this.props;
    document.removeEventListener(event, this.handleEvent);
  }

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

  render() {
    const { children } = this.props;
    const { trapped } = this.state;

    if (typeof children === "function") {
      return children(trapped);
    } else {
      return null;
    }
  }
}

We could use it with a simple API:
<Trap event="click">{trapped => <Box isFocused={trapped} />}</Trap>

But now with out findDOMNode we invert the responsibility of handling the ref to the consumer.
So our API must change and expose the ref function for the consumer to use it:

class TrapWithRef extends React.Component {
  state = {
    trapped: false
  };

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

  componentWillUnmount() {
    const { event } = this.props;
    document.removeEventListener(event, this.handleEvent);
  }

  handleEvent = ({ target }) => {
    if (this.ref && this.ref.contains) {
      const trapped = this.ref.contains(target);
      this.setState({ trapped });
    } else {
      console.log("no ref -> ", this.ref);
    }
  };

  render() {
    const { children } = this.props;
    const { trapped } = this.state;

    if (typeof children === "function") {
      return children(trapped, ref => (this.ref = ref));
    } else {
      return null;
    }
  }
}

The consumer will use it this way:

<TrapWithRef event="click">
  {(trapped, ref) => <Box innerRef={ref} isFocused={trapped} />}
</TrapWithRef>
@oliviertassinari

This comment has been minimized.

Contributor

oliviertassinari commented Nov 1, 2018

@sag1v It's the best solution I can think of too. We will have to release a major on Material-UI side if it's the go-to approach and have people change their implication. But I think that it's laborious and error-prone (hard to debug when dealing with multiple level of components), they might be a better way 🤔.

<TrapWithRef event="click">
  {(trapped, ref) => <Box innerRef={ref} isFocused={trapped} />}
</TrapWithRef>
@gaearon

This comment has been minimized.

Member

gaearon commented Nov 1, 2018

I'm still confused why you have <Box innerRef> in examples rather than <Box ref>.

@sag1v

This comment has been minimized.

sag1v commented Nov 1, 2018

@gaearon <Box innerRef> is the consumer, it's just an example of the usage.
If it's clearer then it can be something like this:

<TrapWithRef event="click">
  {
    (trapped, ref) => (
      <div>
        <div ref={ref}>{trapped ? 'open' : 'close' }</div>
      </div>
    )
  }
</TrapWithRef>

The only advantage for this approach is that the consumer can decide where to attach the ref.

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 1, 2018

I mean that you can pass <Box ref>, and Box can forward it anywhere. Just making sure we're on the same page that you don't need to introduce ref-like props to your API. Specifically, I still don't understand why

agreeing on some sort of "standard" property for cascading the reference

is necessary, and how that's relevant to the example above.

@Macil

This comment has been minimized.

Macil commented Nov 1, 2018

I recently updated ReactFloatAnchor and ReactMenuList to no longer use findDOMNode, and I had to make changes similarly to @sag1v. ReactFloatAnchor takes ref => ReactNode instead of just ReactNode now. In ReactMenuList's SubMenuItem, I needed to use ReactFloatAnchor and pass the ref onto MenuItem's dom element, so I added a "domRef" prop to MenuItem which MenuItem passes as a ref to its div. (This was also pretty awkward, because MenuItem itself needs a ref to its own div, so I passed a function as the ref to the div which set a property on the class and then called my own setRef function with props.domRef if present.)

@gaearon In my case, I couldn't use ref forwarding because MenuItem has many imperative methods and users need to be able to get a ref to the MenuItem itself too.

@oliviertassinari

This comment has been minimized.

Contributor

oliviertassinari commented Nov 1, 2018

I'm still confused why you have in examples rather than .

@gaearon The fact that we don't understand each other is a good sign, something is off. Here is my understanding of the situation. As a library author, we don't control what the user's Box component will provide. It can be a functional component, that doesn't accept reference or it can be a class component, that will return a reference to the class component instance. Either way, we don't have access to the underlying DOM node. There is one case where it can work, it's if the Box component uses forwardRef and apply the ref on a native element. It's unlikely.

@jquense If I remember correctly, you were commenting about this problem 1-2 years ago. Maybe you could provide some light on the issue 💡, thanks!

@sag1v

This comment has been minimized.

sag1v commented Nov 1, 2018

@gaearon I made a small and concise codesandbox example (a very naive implementation ) with just the parts that are relevant for this issue (from my angle at least, maybe @oliviertassinari has some more use-cases ).

As you can see, both Trap and TrapWithRef are almost identical,
The only difference is that Trap uses findDOMNode and doesn't care what and how the children are rendered. its just finds the first DOM node and use it for its internal logic.

As for TrapWithRef, its exposing a ref callback and will only work if and when the child (consumer of the library) will use it and pass it to some DOM element.

It may seem like a small change to the API but this is quite significant IMHO. it feels awkward and confusing (for the consumers).

I'll be honest, there is an upside for this pattern though, The consumer gets to decide where or what exactly to "Trap". but this is a very small upside, IMO.

@sag1v

This comment has been minimized.

sag1v commented Nov 7, 2018

Another concern i have for exposing an explicit ref instead of using findDOMNode is that it may conflict with other components.
For example, if someone is using 2 parent components that needs a ref to the element (for a DOM logic) they can't assign it to the same DOM element (as far as i know).

Given this example, how can we use both ref and ref2?

<ElementResize>
  {(rect, ref) => (
    <Trap event="click">
      {(trapped, ref2) => (
        <div ref={???}>
          <div> {trapped ? 'focused' : 'blur'}</div>
          <div>{rect.width}</div>
        </div>
      )}
    </Trap>
  )}
</ElementResize>
@oliviertassinari

This comment has been minimized.

Contributor

oliviertassinari commented Nov 7, 2018

Given this example, how can we use both ref and ref2?

@sag1v We have an helper for that on Material-UI. But yes, it's a bit verbose otherwise.

@jquense

This comment has been minimized.

Collaborator

jquense commented Nov 7, 2018

I would add we've tried generally to move to a findDOMNode-less, ref only only APi in react-bootstrap 1 and it's been kinda painful. For all the reasons me and @taion have noted in the past as well as what @oliviertassinari is saying now.

Honestly tho, the most frustrating problem I have is that we can't always tell if a user swallowed or dropped the ref we passed their component. Take the case of a dropdown/tooltip. We need two nodes, the toggle node, and the menu node. Once both both nodes exist we can pass them to Popper for positioning and calculation. Consider if they conditionally render the menu or toggle. We can't always warning that a ref wasn't used because they haven't used it yet, there are of course some workarounds and it's not intractable, but it has definitely increased bugs.

Its ultimately the same sort of problem as props passing and middle components not passing through some prop. However the problem is worse because:

  • it's hard to debug where refs are lost in a component stack via the dev tools
  • it's not clear whether adding some HoC or recompose util will swallow the ref (this bites us all the time with emotion/SC style components)
  • RHL eats refs via the proxy component
@sag1v

This comment has been minimized.

sag1v commented Nov 7, 2018

@sag1v We have an helper for that on Material-UI. But yes, it's a bit verbose otherwise.

@oliviertassinari do you mean using both ref's on a single DOM element?
Care to share a link? :)

@TrySound

This comment has been minimized.

Contributor

TrySound commented Nov 7, 2018

@sag1v With hooks ref composition may not be required

const Component = () => {
  const ref = React.useRef()
  const rect = useElementResize(ref)
  const trapped = useTrap(ref)

  return (
    <div ref={ref}>
      <div> {trapped ? 'focused' : 'blur'}</div>
      <div>{rect.width}</div>
    </div>
  )
}
@sag1v

This comment has been minimized.

sag1v commented Nov 7, 2018

@TrySound Yeah i know, i already played with it 😄

image

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