RFC: An alternative to ReactDOM.findDOMNode

[silvenon]

Hi everyone 👋

The time has come to implement an alternative to ReactDOM.findDOMNode, as explained in React docs:

findDOMNode is an escape hatch used to access the underlying DOM node. In most cases, use of this escape hatch is discouraged because it pierces the component abstraction. It has been deprecated in StrictMode.

The goal is to come up with an API that is optional and falls back to ReactDOM.findDOMNode. I think that we should eventually change the entire API into something more sustainable, probably hooks, but that's a separate discussion.

So far we have three proposed solutions:

1. findDOMNode, a function that returns a node (#457)

@eps1lon's solution would look like this:

import React from 'react'
import { Transition } from 'react-transition-group'

const StrictModeTransition = () => {
  const childRef = React.useRef(null)

  return (
    <Transition appear in findDOMNode={() => childRef.current}>
      <div ref={childRef}>
        transition
      </div>
    </Transition>
  )
}

2. domRef, a ref object (#559)

@iamandrewluca's solution would look like this:

import React from 'react'
import { Transition } from 'react-transition-group'

const StrictModeTransition = () => {
  const childRef = React.useRef(null)

  return (
    <Transition appear in domRef={childRef}>
      <div ref={childRef}>
        transition
      </div>
    </Transition>
  )
}

3. ref object as a 2nd argument in render prop

My solution would look like this:

import React from 'react'
import { Transition } from 'react-transition-group'

const StrictModeTransition = () => {
  return (
    <Transition appear in>
      {(status, childRef) => (
        <div ref={childRef}>
          transition
        </div>
      )}
    </Transition>
  )
}

So let's begin!

Updates

For those who are just catching up with the discussion, one notable proposition is to extend the 3rd solution with the ability to pass your own ref, which would then be forwarded to the render prop:

import React from 'react'
import { Transition } from 'react-transition-group'

const StrictModeTransition = () => {
  const childRef = React.useRef(null)
  return (
    <Transition appear in ref={childRef}>
      {(status, innerRef) => (
        // innerRef === childRef
        <div ref={innerRef}>
          {status}
        </div>
      )}
    </Transition>
  )
}

This way we can avoid having to merge refs, which is the key benefit of the 2nd solution. (If you don't pass a ref to Transition, it would create one internally.)

[silvenon]

The 3rd solution just came to my mind while I was writing the RFC, I might have overlooked something disqualifying.

[eps1lon]

I'm in favor of proposal two:

• more popular than number one
• basic use case doesn't require additional functions

Number 3 is problematic since it's easier to spot wrong usage. By requiring domRef we could potentially warn if it isn't provided whereas in proposal 3 forwarding the ref could be forgotten without react-transition-group noticing (because it's not clear whether a DOM instance isn't created yet or the ref isn't actually attached). It could still be forgotten in number 2 but at least you have declared it without usage.

[inomdzhon]

@eps1lon no problem with warn. Here is demo.

I give my vote to the third solution. It's look tasty and simple.

[unrevised6419]

With third solution we can't get a ref direct to element, we have to use one provided by transition

import React from 'react'
import { Transition } from 'react-transition-group'

const StrictModeTransition = () => {
  const divRef = React.useRef(null)
  return (
    <Transition appear in>
      {(status, childRef) => (
        // how we pass `divRef` here? we have to use `childRef` instead
        <div ref={childRef}>
          {status}
        </div>
      )}
    </Transition>
  )
}

[unrevised6419]

Let's think this problem also from perspective of adding hooks version of library

e.g. const status = useTransition(/* ... */)

[inomdzhon]

@iamandrewluca we can do this

import React from 'react'
import { Transition } from 'react-transition-group';

function setRef(ref, value) {
  if (typeof ref === 'function') {
    ref(value);
  } else if (ref) {
    ref.current = value;
  }
}

function createChainedRef(refA, refB) {
  return (value) => {
    setRef(refA, value);
    setRef(refB, value);
  };
}

const StrictModeTransition = () => {
  const divRef = React.useRef(null);

  return (
    <Transition appear in>
      {(status, childRef) => (
        <div ref={createChainedRef(childRef, divRef)}>
          {status}
        </div>
      )}
    </Transition>
  )
}

[unrevised6419]

Here is an implementation of merged refs
https://github.com/react-restart/hooks/blob/master/src/useMergedRefs.ts#L13

[eps1lon]

The solution in #606 (comment) is quite excessive since it re-creates ref callbacks on every render which means the we need to cleanup and re-attach on every render. Solution number two does not have these problems since the ref callback responsible for forking can be memoized.

[silvenon]

By "solution number two" you're talking about useMergedRefs?

[silvenon]

Anyway, regardless of which strategy for merging refs is best to use, this is possible and should be done by the user if they need to.

[jquense]

I think a combo of both is probably the most common and ergonomic?

  const divRef = React.useRef(null);

  return (
    <Transition appear in ref={ref}>
      {(status, innerRef) => (
        <div ref={innerRef}>
          {status}
        </div>
      )}
    </Transition>
  )

Alternatively, i've been dragging my feet on this mostly because i think that Transition likely shouldn't be a component at all and should look more like

const status = useTransition({
  in,
  appear: true,
})

// use an effect instead of callbacks
useEffect(() => {
  if (status === 'exited') onExited()
}, [status])

[silvenon]

Definitely, I also think it shouldn't be a component, IMO same goes for CSSTransition, but maybe a unified shape of the result:

const transition = useCssTransition({
  in: inProp,
  appear: true,
  classNames: 'fade'
})

return transition.status !== 'exited'
  ? (
    <div
      ref={transition.ref}
      className={transition.className}
    />
  : null

EDIT: Or possibly not having useCssTransition at all? 😜 Instead useTransition could add this behavior based on whether or not it receives classNames. It's fun to think about. 🔥

[silvenon]

About the proposition to pass ref to the Transition component itself, it feels weird to pass the ref to the parent which forwards it to the child element that you yourself provided. 😕 So the element you're targeting is right in front of you, but instead you pass it to its parent. This pattern is common?

[JSteunou]

I have another solution, might not be the popular one because it requires to use forwardRef if your html is deeper in another component but we are using it at work and so far so good.

Here the patch we applied with patch-package https://gist.github.com/JSteunou/5e9e96981562747f760837a1e1d1e96a

And here is how it goes in general

function Main() {
    return (
        <Transition in appear>
            {transitionStatus => {
                return transitionStatus !== 'exited' ? <h1>Some title</h1> : null
            }}
        </Transition>
    )
}

and how it goes when your html is deeper in components

const Home = forwardRef(function Home({transitionStatus}, forwardedRef) {
    return (
        <div ref={forwardedRef}>
            <Background transitionStatus={transitionStatus} />
            <Foreground transitionStatus={transitionStatus} />
        </div>
    )
})

function Main() {
    return (
        <Transition in appear>
            {transitionStatus => <Home transitionStatus={transitionStatus} />}
        </Transition>
    )
}

So if the user is in the 1st case, no ref to handle :)

[silvenon]

I can understand the appeal of it because it can fix a lot of warnings without users having to do anything, but there will be some cases where forwarding ref is necessary, and for that reason I think it's better if passing the ref was explicit.

[JSteunou]

Yep, I know forwardRef is not sexy :)

That said I like solution 3 for it's simplicity and proximity to what I'm using now. I really think it is the best DX

The only drawback in 3 that 2 solves, is that you are not the creator of the ref, the lib is. Some would want to be the owner of it.

Maybe mixing solution 2 into 3 by allowing the user to give the ref to prevent the lib to create it for you, as an optional prop would be the better of the two worlds. It would only be for advanced usage.

[silvenon]

Yep, I know forwardRef is not sexy :)

It's not that, with the 3rd solution we'd need forwardRef just as much, I just think it might be better to make this deliberate. But I'm not dead set on this, it's pretty hard to decide when there are so many different combinations, each arguably better than the other ones. 😄

Some would want to be the owner of it.

Why? What's the use case? 🤔

Maybe mixing solution 2 into 3 by allowing the user to give the ref to prevent the lib to create it for you, as an optional prop would be the better of the two worlds. It would only be for advanced usage.

What would the usage look like?

[unrevised6419]

What would the usage look like?

  const divRef = React.useRef(null);

  return (
    <Transition appear in domRef={divRef}>
      {(status, innerRef) => (
		// here `innerRef` is not created by `Transition`
		// but it is `divRef` that was passed to `Transition`
		// so `innerRef === divRef`
		// if `domRef` is not passed to `Transition`
		// then `innerRef` will be created internaly by `Transition`
        <div ref={innerRef}>
          {status}
        </div>
      )}
    </Transition>
  )

But here with all this examples we forget about the case when children is not a function. Or it will be dropped children as react element?

[JSteunou]

Yep, I know forwardRef is not sexy :)

It's not that, with the 3rd solution we'd need forwardRef just as much, I just think it might be better to make this deliberate. But I'm not dead set on this, it's pretty hard to decide when there are so many different combinations, each arguably better than the other ones. smile

Some would want to be the owner of it.

Why? What's the use case? thinking

Again, very advanced usage and maybe very rare but I was thinking about others libs that need a ref like Draggable or likewise. The user can be in a situation he does not want or cannot use multiple div and multiple ref, one for each lib. So maybe he would create the ref and re-use it. It might not be a good practice, but it could save users :)

Also it could be added as a feature in a N+1 release, minor version, not breaking the API :)

Maybe mixing solution 2 into 3 by allowing the user to give the ref to prevent the lib to create it for you, as an optional prop would be the better of the two worlds. It would only be for advanced usage.

What would the usage look like?

@iamandrewluca was faster, see above ⬆️

[JSteunou]

So I keep my vote on solution 3 and hope for a solution 2 as a nice addition :)

[unrevised6419]

To keep in mind that 3rd solution does not work for this case

<Transition>
  <div>content</div>
</Transition>

While 2nd solution will work for this case.

[JSteunou]

Why not keeping the actual code/DX when children is not a function or ref capable and make it transparent for the user?

[silvenon]

For advanced usage that you described I would like to allow this behavior:

const childRef = React.useRef(null)
return (
  <Transition in appear ref={childRef}>
    {(status, ref) => (
      // ref === childRef
      <div ref={ref} />
    )}
  </Transition>
)

but not this:

const childRef = React.useRef(null)
return (
  <Transition in appear ref={childRef}>
    {/* ref is passed internally */}
    <div />
  </Transition>
)

because someone with the edge case that you described, where they can't combine refs (I don't know how that can happen, but let's say), might attempt to do this:

const childRef = React.useRef(null)
const anotherRef = React.useRef(null)
return (
  <Transition in appear ref={anotherRef}>
    <div ref={childRef} />
  </Transition>
)

Now refs would be combined internally and things wouldn't work as expected.

The 1st snippet is combining solutions 2 and 3 because you're the owner of the ref. What do you think?

The reason why I want to avoid passing ref internally is because I want to make a step towards the potential new hooks API.

[JSteunou]

Agreed !

[silvenon]

I love it! It has the key benefits of both solutions IMO. We'll see what others think.

[silvenon]

Just to set a deadline for objections: Wednesday, then I'll start working on this. @iamandrewluca if you're particularly interested in pivoting from your PR to implement this in a separate PR, let me know. 😉

[unrevised6419]

Ok. So from what I understood. This is the desired API?

function App() {
  const divRef = React.useRef(null)
  return (
    <Transition in appear domRef={divRef}>
      {/* ref === divRef */}
      {(status, ref) => <div ref={ref}>{status}</div>}
    </Transition>
  )
}

function App1() {
  return (
    <Transition in appear>
      {/* ref is created internally in Transition */}
      {(status, ref) => <div ref={ref}>{status}</div>}
    </Transition>
  )
}

function App2() {
  return (
    <Transition in appear>
      {/* still use findDOMNode */}
      <div />
    </Transition>
  )
}

[silvenon]

Almost, I think it's cleaner if the outside ref object is being passed through ref, not domRef:

<Transition in appear ref={divRef}>

Simply because it's the only ref that Transition can have. (Honest) thoughts? Is this breaking some React convention, or is there another reason why this is unintuitive that I overlooked?

[unrevised6419]

This should imply a breaking change.

https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers

There are may be some users who use ref to get a reference to instance created by Transition

[silvenon]

Oh, that's right 🤦 I've been dealing with function components so much that I forgot about what ref does for class components.

Now that the ref prop is out of the question (I think causing a major version bump is unnecessary for this), the proposed syntax of passing ref object from outside looks awkward to me: we'd pass a ref object to domRef, then pass another ref object, which is actually just a reference to that same object, to the React element. I can imagine this getting annoying in practice, or even worse, confusing and error-prone.

And, now that some time has passed, for the 3rd solution I no longer feel that the semantic benefit (scoping the ref only to children) outweighs the downside of its cluttered syntax (having to use a render prop and skip the often unused state argument).

I would like to proceed with the 2nd solution, #559, I believe it's the easiest to use.

Considering that I have made a decision, I'm closing this RFC and continuing the conversation in that PR, but people are still free to share their opinions about this here.

[unrevised6419]

@silvenon I investigated this more, and it seems we don't have to add more code, we actually kind of have to remove code, and simplify API. What we are trying to do in most components, is to get a ref from the user, and pass it back to him.

ReplaceTransition src/ReplaceTransition.js:31

Is using findDOMNode to get a ref, and pass it back to props this.props.onEnter(node). We can just call this.props.onEnter() and user can get himself a ref to his node if is needed.

TransitionGroup src/TransitionGroup.js:75

Same thing, it needs node to pass it back to the user.

Transition src/Transition.js:213

As I see is actually used only here src/Transition.js:316 and I think this is used because of some state sync problems, in all other places, is used just to pass it back to user

CSSTransition

Here node is actually used to add/remove class names only, maybe only here it makes sense to add a domRef, or maybe use inversion of control, and let user to set classes this.props.setClasses(aggregatedClasses).

Maybe we can think a solution and get rid completely of node?
What are your thoughts?

https://github.com/iamandrewluca/react-transition-group/commit/e5efe1e787c4ccae4a8723c71bed22193ea60567

[silvenon]

Yes, now that we’d be passing in the ref ourselves, returning it from callbacks doesn’t really make sense, but in my opinion adding domRef should just be a quick fix for the Strict Mode issue, a single prop that people can add to make the warnings go away, and considering that we’re planning to migrate towards hooks anyway, it might not make sense to spend more effort on improving the component API, especially to cause a breaking change, if I understood correctly.

[silvenon]

But it makes sense to return the node back only if ReactDOM.findDOMNode is used. Or is that what you meant?

[unrevised6419]

Yes, now that we’d be passing in the ref ourselves, returning it from callbacks doesn’t really make sense, but in my opinion adding domRef should just be a quick fix for the Strict Mode issue, a single prop that people can add to make the warnings go away, and considering that we’re planning to migrate towards hooks anyway, it might not make sense to spend more effort on improving the component API, especially to cause a breaking change, if I understood correctly.

Totally agree.

But it makes sense to return the node back only if ReactDOM.findDOMNode is used. Or is that what you meant?

Is not what I meant, but actually it makes sense.

Then I'll go with the implementation of domRef. I'll make some reviews, and ping you when is done.