React-popper: Infinite render loop with Downshift

[a-type]

Reproduction demo

https://codesandbox.io/s/wzn6ppylml

Steps to reproduce the problem

1. Open the reproduction above
2. OPEN DEVTOOLS, or the tab will hang
3. Start typing into the input

What is the expected behavior?

Popper element appears and doesn't cause any trouble.

What went wrong?

There's an infinite state update loop which seems related to the ref from Popper being combined with the internal ref from Downshift.

As you can see, I'm using Downshift's recommended pattern of passing all props (including ref) through their getMenuProps() helper function.

This runs Popper's ref through their combiner function (caller) and spits out a new, composed function to the final ref prop.

As soon as that ref is called, Popper begins an infinite loop. The key problem is this guard. We run into the infamous functional ref behavior where the same ref is called twice with null, then the element. Since it's the same function, this forces Popper to oscillate between null and the correct element reference and constantly update its state.

Any other comments?

We have been using this library for a while and only encountered this problem recently. I suspect this was because of this change which may have removed a guard that stopped the loop early.

Also, apologies for the terse tone here. It's been a long day of trying to diagnose what's going on here. I mean no ill will toward anyone and would be happy to dig in a bit into the root of this behavior and possible mitigation. Cheers for having very readable source code.

Why am I opening this against Popper and not Downshift?

This problem isn't isolated to Downshift specifically, but is a product of how react-popper works and some assumptions it makes. Downshift happens to encourage patterns which break it.

That said, I'm going to try some experiments with memoization and see if I can find a good mitigation strategy. If it turns out that memoizing getMenuProps is reasonable and could help solve the problem, I may float that by the Downshift maintainers.

I'd be happy enough if you decided to close this as wontfix, being satisfied that I documented this behavior so that hopefully future searchers might find some specifics (including, hopefully, some workarounds which I will comment here if I find them).

I still think Popper could document this gotcha more explicitly though, to aid others who run across the issue in different contexts. I've seen other issues here along this same line (#199, etc), but if I didn't know the problem I was looking for, I would not have realized they were related.

Perhaps there's also a change the library could make to prevent this happening in the first place (like checking for a null popperNode and not updating state).

Packages versions

• Popper.js: whatever's bundled
• react-popper: 1.3.0

[a-type]

Memoized Downshift props: https://codesandbox.io/s/kq3kqzml3

Unfortunately, Downshift complains that its ref was not attached correctly. I can only assume this warning is meaningful and so this is probably not a valid solution.

This makes sense, since the params I am memoizing against are all from Popper, so if Downshift changed its state internally, my memoized function would clobber that.

I'm interested in pursuing the possibility of being smarter about a null popperNode internally in the library. When I have some time I'll try forking to experiment with that.

[seancurtis]

We've run into this too. I noticed 1.1.0, 1.2.0, and 1.3.0 were released recently, but there's no tag or changelog for it. The change that @a-type mentioned looks to be the root cause. We noticed infinite recursion in a promise callback. We've had to pin to 1.0.2 in the meantime.

[a-type]

We pinned 1.0.2 as well. I should have mentioned that is a valid workaround for now. I haven't had time to dig into this to see if Popper could guard against this condition more effectively.

[piecyk]

@FezVrasta just wondering way we can't just assert that the popperNode need to be there? here
https://github.com/FezVrasta/react-popper/blob/master/src/Popper.js#L77

basic

  setPopperNode = (popperNode: HTMLElement | null) => {
    if (!popperNode || this.popperNode === popperNode) return;

    safeInvoke(this.props.innerRef, popperNode);
    this.popperNode = popperNode;

    this.updatePopperInstance();
  };

if we get new popperNode that is different from instance.popperNode we will call updatePopperInstance. When Popper component is unmounted componentWillUnmount kiks in and PopperInstance is destroyed 🤔

[piecyk]

The same effect would be when asserting the ref before passing it https://codesandbox.io/s/qkyv72n60w
looks like this fix the problem

[FezVrasta]

@piecyk if that fixes the problem may you submit a PR? If we manage to also add a unit test to prevent regressions it'd be awesome

[piecyk]

Yes, will submit soonish!