Clarification on nodeRef

[eps1lon]

When changing key prop of Transition in a TransitionGroup a new nodeRef need to be provided to Transition with changed key prop (see test/CSSTransition-test.js).

-- https://reactcommunity.org/react-transition-group/transition

Could you clarify why and how we would accomplish this (the linked test doesn't use nodeRef)?

It sounds like useRef wouldn't work for that since that one never changes. Instead we'd have to do something like const nodeRef = React.useMemo(() => ({ current: null }), [key]);.

Isn't the point of a "ref" that you don't need to change it because the pointer (current) always points to the latest value?

[eps1lon]

Another point: It was never mentioned in the RFC that this prop would change the function signature. It's unnecessary and would've been a good opportunity to fix #469.

#469 does not change function signature. It will be a breaking change for people relying on 4.4 behavior of on* callbacks.

[iamandrewluca]

Isn't the point of a "ref" that you don't need to change it because the pointer (current) always points to the latest value?

#559 (comment)

[iamandrewluca]

Another point: It was never mentioned in the RFC that this prop would change the function signature. It's unnecessary and would've been a good opportunity to fix #469.

#559 (comment)

There is no reason to send back node to user, because he already has access to the node.

Can you edit #469 to have the same behaviour?

[eps1lon]

There is no reason to send back node to user, because he already has access to the node.

This is assumes a lot about the usage of Transition. In your opinion the following usage is not a valid use case for Transition:

function Collapse({ onEntered }) {
  const nodeRef = React.useRef(null);
  return <Transition onEntered={onEntered}><div ref={nodeRef} /></Transition>;
}

Even if what you were saying is correct, I think you're introducing unnecessary churn by changing the function signature. We could've simply applied

-<Transition />
+<Transition nodeRef={nodeRef} />

but now we have to go through every single callback of an untyped library. Getting the types right is another can of worms.

Isn't the point of a "ref" that you don't need to change it because the pointer (current) always points to the latest value?

#559 (comment)

That is fixed by #469.

[silvenon]

Hi @eps1lon, I just wanted to confirm that you're right; now the burden of managing references to nodes is on people using the library, which was unnecessary.

I'll have to think about the next steps.

[iamandrewluca]

In your opinion the following usage is not a valid use case for Transition

Is a valid case but it will use findDOMNode inside.

I think you're introducing unnecessary churn by changing the function signature. We could've simply applied

Yes, I agree.

but now we have to go through every single callback of an untyped library. Getting the types right is another can of worms.

Yes this change is propagating to most end user.

A ugly solution would be

function Collapse({ onEntered, innerRef }) {
  const nodeRef = useMergedRef(innerRef)
  
  const _onEntered = useCallback((appearing) => {
    return onEntered(nodeRef.current, appearing)
  }, [nodeRef])
  
  return <Transition onEntered={_onEntered}><div ref={nodeRef} /></Transition>;
}

// https://github.com/reactjs/react-transition-group/blob/master/stories/transitions/Bootstrap.js#L143
function useMergedRef(ref) {
  const nodeRef = React.useRef()
  useEffect(function () {
    if (ref) {
      if (typeof ref === 'function') {
        ref(nodeRef.current)
      } else {
        ref.current = nodeRef.current
      }
    }
  }, [ref])
  return nodeRef
}

[eps1lon]

A ugly solution would be

You don't need the useMergeRef. const handleEntered = (appearing) => onEntered(nodeRef.current, appearing) is sufficient. It was just unnecessary to introduce this change.

[jquense]

hey folks, thanks for working through this and trying to keep it calm and civil. I think it's fine that the new API made an attempt to be minimal, for new things that's often the better approach and it's much easier to add things in vs removing them. Ideally we DO want Transition to be agnostic to the host node and want the consumer to manage them as needed. We also want to make sure transition (ha) to newer API's is easy so thanks for bringing up this use case @eps1lon and maybe it was a bit too much to try and simplify the API in a feature release like this.

To the above use case (just for my understanding) is the issue that if you need the node in a callback it's a bit of a pain to get now?

function Collapse({ onEntered }) {
  const nodeRef = React.useRef(null);
  return <Transition onEntered={status => onEntered(nodeRef, status)} ref={nodeRef} ><div /></Transition>;
}

This seems ok to me considering that Transition is usually used to build higher level transition components. But I'd like to hear more about how ya'll using it.

To me it makes sense to me for CSSTransition to alway passes the node through it's callbacks since it's by definition operating on a DOM node, but it doesn't make sense for Transition itself to, since it's more of an abstraction. And just to clarify the reason the current API does pass node through is mostly laziness on my part when i wrote it originally (and hooks didn't exist yet)

[iamandrewluca]

@silvenon on twitter DM explained the problem to me cause I did not understood from start

What we could’ve done with is implement it in a way that wouldn’t require creating one ref object for each key, by accessing this.props.nodeRef.current at the same time as findDOMNode used to, “freezing” that reference at that time in the lifecycle.

Also, if you do something like:

<Transition onEntering={() => {
  somethingAsync()
    .then(() => {
      nodeRef.current
    })
}}>

nodeRef.current might not be the same, it might point to a different element. If we had continued to pass the “node” argument as before (taken from this.props.nodeRef.current), the user wouldn’t have to think about that.

[eps1lon]

I'm not that invested into the issue since you can hide the new API and keep a single function signature instead of having to keep track of the props shape to determine what type transition callbacks have. It just required so much work that it was indistinguishable from a breaking change and wasn't discussed in the original RFC. That's why this was somewhat frustrating for me.

To go forward: The documentation is not very clear with the transition callbacks now.

Note: when nodeRef prop is passed, node is not passed.
type: Function(node: HtmlElement, isAppearing: bool) -> void

This could mean type: Function(node: undefined, isAppearing: bool) -> void. I think we should be very clear here what the function signature is for each type of nodeRef.

I'll update #469 so that it's not a breaking change compared to 4.4.0 since I still believe that bug is not fixed.

[silvenon]

It just required so much work that it was indistinguishable from a breaking change and wasn't discussed in the original RFC. That's why this was somewhat frustrating for me.

At the time I wasn't aware that the function signature will be changed, so I didn't include it when presenting the solution, but when I found out I underestimated it and didn't consider the full range of use cases of this library. So I completely understand your frustration.

To go forward: The documentation is not very clear with the transition callbacks now.

I was aware of this problem immediately 😣 so I wanted to clarify by including some custom HTML, but our documentation setup isn't flexible enough at the moment.

---

It's just a mess. I would most rather release a shameful major version where nodeRef is properly implemented, but I assume that ship has sailed already.

[jquense]

Was this work required to upgrade? It should be opt in, which certainly doesn't reduce the amount of work but does mean you can chunk it and spread it out. I'd really like to reiterate that a breaking release that removes node from the callbacks for any case is likely give where we expect the API to move. So it's well placed work at least!

[silvenon]

I don't follow 😅 Are you saying that this would be too much considering that the library will move towards a simpler (and breaking) API anyway?

[jquense]

I'm more mildly defending the asymmetrical API nodeRef went with. I do think the 'new nodeRef' requirement is not great tho.