Allow ref objects to be passed

[maclockard]

Fixes #307

I tested this locally with my app using yarn link and everything worked!

[maclockard]

this is very unfortunate, but this issue with contravariance vs covariance with refs is a very long standing issue with TS that doesn't look like it will be resolved soon.

[FezVrasta]

Why can't we use React$Ref<HTMLElement> here?

[maclockard]

React$Ref<HTMLElement> includes string refs which are legacy and react-popper does not support.

[piecyk]

@maclockard would be awesome if you add test for this 👍

[maclockard]

Added tests!

Ended up needing to bump some of the test versions to properly support ref objects, so theres a bit more churn on the snapshots than I would like, but that's needed for #303 anyway.

Still something a little bit funky with the flow typings since it doesn't recognize React.createRef even though it exists and works, but not too concerned since its only used in tests. I'm a bit more of a typescript person so if you have thoughts, let me know.

[maclockard]

@FezVrasta rebased this PR on top of master, could you take a look?

[maclockard]

@FezVrasta do you think you will have time to look at this soon?

[shepmaster]

I have code like this:

      <Reference>
        {({ ref }) => <Button ref={(r) => { ref(r); buttonRef.current = r; }} toggle={toggle} />}
      </Reference>

Which now reports an error:

[tsl] ERROR in PopButton.tsx(39,45)
      TS2349: This expression is not callable.
  Not all constituents of type 'Ref<any>' are callable.
    Type 'RefObject<any>' has no call signatures.

Is there a suggested change I should make?

[maclockard]

@shepmaster What versions of react and react popper are you using? Is this in a function component or a class component?

IsbuttonRef a ref object or a callback ref?

[shepmaster]

@maclockard sorry for the initial lack of information!

• React: 16.12.0
• React Popper: 1.3.4 (attempting to update to 1.3.5)
• This is in a functional component
• buttonRef is from useRef<HTMLElement>();

The whole project.

The code currently appears to work (e.g the "Debug" menu button).

Thanks!

[maclockard]

@shepmaster the code you have is correct in the sense that it will run, but its just failing a type check. The ref you are being passed is a callback ref in actuality, but the type is React.Ref which is a union type of a callback ref and a ref object. So Typescript doesn't know which variant ref is and throws a type error since you can't call a ref object.

A way to fix this is for you to add a type guard to provide TS with enough information to know that ref is a callback ref and not a ref object. Here's an example of doing this:

function isRefObject<T>(ref: React.Ref<T>): ref is RefObject<T> {
  return ref !== null && typeof ref !== 'function';
}

An other (worse) solution would be to change the type of ref in react-popper to just be a callback ref instead of React.Ref. I personally do not like this solution since it would couple react-popper's API to be tied to using callback refs, meaning it would be a breaking change for react-popper to start using React.createRef or React.useRef in the future. By keeping the more general React.Ref type, react-popper becomes more future proof. By adding a type guard for this case, you code becomes more defensive in the case react-popper changes its ref type.

[maclockard]

Another (probably better) solution is that react-popper provides an innferRef prop on Reference. You could pass buttonRef as that prop and react-popper will correctly combine buttonRef and the ref it uses under the hood. Here's the code that does that: https://github.com/popperjs/react-popper/blob/master/src/Reference.js#L21-L24.

So your code would look like this:

<Reference innerRef={buttonRef}>
  {({ ref }) => <Button ref={ref} toggle={toggle} />}
</Reference>

[shepmaster]

Another (probably better) solution is that react-popper provides an innferRef prop on Reference

Ah, that works wonderfully, thank you! For future readers, there's also an innerRef on Popper.

Now I'll have to wonder if those were always there and I missed them on my original implementation or what... 😉