React-popper: Getting ref to Popper element in v1.0.0-beta.5

[TLadd]

Reproduction demo

https://codesandbox.io/s/10nmjoq5jq

What is the expected behavior?

Is it possible with the current v1.0.0 api to get a reference to the popper element? In v0.10.1, an innerRef function existed that could be passed into the Popper component, and would be forwarded onto the ref passed down to children: https://github.com/FezVrasta/react-popper/blob/v0.10.1/src/Popper.js#L188

What went wrong?

On v1.0.0-beta.5, the innerRef prop is removed and trying to set it manually results in an infinite loop, since the ref callback is putting the ref in state: https://github.com/FezVrasta/react-popper/blob/master/src/Popper.js#L71, triggering componentDidUpdate.

Packages versions

• react-popper: v1.0.0-beta.5

[TheSharpieOne]

I believe it is because you are using an inline function. The new function being created every time triggers it to rerender which triggers react-popper call the child function which then creates a new ref function .... loop.
If you switch to a function which is "cached" you don't see this issue: https://codesandbox.io/s/p9x2myxymx
Not sure the best way to get the ref function passed in from react-popper to the cached function thought, in the example we just store the most recent function on the component's instance and use that. 🤷‍♂️

[TLadd]

It seems like adding back the innerRef prop to Popper might make sense?

[FezVrasta]

I'd rather use the new forwardRef utility, maybe using a polyfill to support old versions.

What's exactly the use case? Can't you do like this?

const MyPopper = ({ innerRef }) =>
  <Popper>
    {({ ref, style }) => <div ref={el => { ref(el); innerRef(el); }} style={style} /> }
  </Popper>

If this is what causes the loop, maybe it's better to add a shouldComponentUpdate that prevents the updates if the references are equal?

[TLadd]

@FezVrasta That's what I tried to do (in the codesandbox), but it ends up in an infinite loop.

The trouble is that inline function callback refs have a caveat where they will be called twice during updates. First it will be called with null, and then it will be called with the DOM element. Usually this behavior doesn't matter. In this case, since the ref callback from Popper calls setState, the first render oscillates back and forth infinitely.

@TheSharpieOne pointed out a workaround, but it requires saving the ref passed down form The Popper component on another instance variable, which gets around having to have an inline function.

My particular use-case, I'm saving a ref to the Popper contents so I can set up a click listener when it is open and determine if the clicked area is contained within the ref or not using this.popper.contains(event.target).

Another workaround I could do is just add another div, so then the ref and innerRef don't have to share, which would also allow for not having an inline function passed to ref. That might be the easiest fix.

[FezVrasta]

I'd rather find a way to avoid the loop actually, I think it may bite us in different ways.

After it's fixed, we can add some innerRef or forwarded-ref to tidy the API.

Does it make sense?

[TLadd]

Yeah, I agree that avoiding the opportunity for the infinite loop would be ideal; just seems like a trap that will probably pop up elsewhere too.

[FezVrasta]

I tried to move the popperNode and arrowNode from the state to two properties of the component but the loop still happens... I'm a bit out of ideas.

[giladgray]

@FezVrasta how about innerRef props on Reference and Popper that receive the result of the ref children prop?

<Popper innerRef={this.myRefHandler}>{({ ref }) => <div ref={ref} />}</Popper>

[FezVrasta]

As said I think I'd prefer to use the new forwardRef utility to make ref return the inner node.

[giladgray]

i don't think that's the best option as it locks consumers in to React 16 or a shim, whereas a simple inputRef prop works for everyone (_edit:_ while still allowing for users to forwardRef if they want).
don't forget that _this_ package declares support for reacts 14, 15, and 16.

[FezVrasta]

There's a lightweight polyfill we could use:
https://github.com/soupaJ/create-react-ref

I was thinking about a way to structure our code to output two different dist targets depending by the desired React version, so that people on React 16 don't have to rely on the polyfills (even if they fallback to the native implementation, the code would still get imported).

[giladgray]

React.createRef is orthogonal to this problem: it's a solution for this.myRefElement, not for providing a way to access the ref instance. i'm still in favor of adding the innerRef prop as it solves the problem with a minimum of dependencies.

[FezVrasta]

Ok let's go this way, if anyone has time to set up a PR with this new feature please do it.

[giladgray]

@FezVrasta i'm on it!

[giladgray]

@FezVrasta PR forthcoming!

FWIW, I strongly prefer TypeScript's tooling to Flow's. This change has been an uphill battle with missing type information and incomplete editor feedback.

[piecyk]

Just as note, one option is to memoize that function, tip from @jlongster https://twitter.com/jlongster/status/984844969751072768

example
https://codesandbox.io/s/2vrjjlm7jr