floating-ui / react-popper

🍿⚛Official React library to use Popper, the positioning library
https://popper.js.org/react-popper/
MIT License
2.52k stars 225 forks source link

Virtual Elements inside a function body without Memoization will trigger an infinite loop #391

Open conoremclaughlin opened 4 years ago

conoremclaughlin commented 4 years ago

Reproduction demo

https://codesandbox.io/s/react-popper-v2x-issue-template-forked-vx46f?file=/src/index.js Sandbox will never finish loading because of the infinite loop

Steps to reproduce the problem

If you are using a virtual element and place it inside a functional component without the use of useMemo or some other caching mechanism, it causes an infinite loop.

  1. Place virtualReference in the functional component
  2. Mount component

What is the expected behavior?

Two potential solutions:

  1. Update the docs to note that the virtual element needs to be memoized or cached outside the render body to avoid others tripping over this.
  2. If possible, don't continue re-rendering and updating if the positional values are the same or follow the render cycle of the parent component.

What went wrong?

Infinite loop with virtual elements specified inside a functional component body.

Any other comments?

Popper.js and React are great! Bringing them together is extra awesome. Thanks for all the hard work :)

Packages versions

FezVrasta commented 4 years ago

A PR to improve the docs would be great.

conoremclaughlin commented 4 years ago

I can potentially take this on once I have a working React-idiomatic example. I'm a bit concerned about this line from the React docs:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

https://reactjs.org/docs/hooks-reference.html

One additional re-calculation pass should be fine, but I haven't dived deep enough into the react-popper code to see what is triggering the infinite update loop.

FezVrasta commented 4 years ago

Fixing the root cause would be great, but Popper relies on re-renders to work on the most recent DOM properties (stylings, positioning, etc) so it's often required to trigger a re-render before we can compute the new position.

cmdcolin commented 3 years ago

Here is a working codesandbox with the useMemo with a follow-the-mouse just in case anyone is interested. https://codesandbox.io/s/react-popper-v2-x-issue-template-forked-kyoc9?file=/src/index.js

The thing about relying on the useMemo is interesting but hopefully this should be ok...

conoremclaughlin commented 3 years ago

As a follow up:

We currently DO use useMemo in production and we haven't had any issues since this was opened almost a year ago now.