Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After upgrade to latest, floating-ui portal gets id=undefined #2778

Closed
smellai opened this issue Feb 5, 2024 · 10 comments
Closed

After upgrade to latest, floating-ui portal gets id=undefined #2778

smellai opened this issue Feb 5, 2024 · 10 comments

Comments

@smellai
Copy link

smellai commented Feb 5, 2024

Describe the bug

After updating to .9, something looks wrong with the generation of the id for a floating ui portal

Expected behavior
I expect a floating-ui-N id to be generated.

Screenshots
Snapshot of the generated HTML:

Screenshot 2024-02-05 alle 10 32 51

Context:

  • Version 0.26.9
@atomiks
Copy link
Collaborator

atomiks commented Feb 5, 2024

Is this only in snapshot tests? In the browser it's defined with both conditional and unconditional rendering.

@smellai
Copy link
Author

smellai commented Feb 5, 2024

Yes, you're right, it happens only in jest snapshots, any idea why it could be?

@atomiks
Copy link
Collaborator

atomiks commented Feb 11, 2024

I couldn't reproduce this with React 17 or 18 with a basic snapshot test using Vitest (both conditional and unconditional FloatingPortal rendering based on an open state), using latest @testing-library/react and jsdom. Maybe it's a React 16 problem in a testing env, or something else, but it doesn't seem to be an actual bug. Feel free to provide a reproduction if you have the ability, but for now, I'm closing due to lack of ability to reproduce the issue.

@atomiks atomiks closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2024
@smellai
Copy link
Author

smellai commented Feb 22, 2024

Hi, I finally managed to set up a reproduction: https://stackblitz.com/edit/vitejs-vite-erxqfe?file=src%2F__snapshots__%2FPopover.spec.js.snap

you can see id="undefined" at line 19

@atomiks
Copy link
Collaborator

atomiks commented Feb 22, 2024

Have you tried:

  • Upgrading to React 18, which uses the native useId() hook and has better perf (I'm going to drop React 17 support when React 19 comes out in a few months anyway)
  • Adding act(async () => {}) after the render() call in the test async (it breaks in Stackblitz, not sure why)
  • Avoiding snapshot tests which can easily break when implementation details of this library change (such as here) even though it's not actually broken for real users

@smellai
Copy link
Author

smellai commented Feb 22, 2024

  • I'm maintaining an npm module so I'm not free to upgrade to React 18 right away, until my consumers are ready.
  • using act doesn't seem to change this behaviour.
  • I know snapshots tests are very limited, but they actually helped in finding bugs after library upgrades. They can be replaced for sure with better tests, but until we don't have the resources to do that, that's the best we have.

I understand my specific issue shouldn't be a priority for you and I'm glad you always take the time to answer in zero time. Thank you!

@mihkeleidast
Copy link
Contributor

@atomiks I see this error in the browser with React 17.

  1. Took the Tooltip example from docs
  2. Downgraded React to 17, upgraded floating-ui to latest
  3. https://codesandbox.io/p/sandbox/upbeat-lake-r5qdz4

In devtools, the ID is undefined:
image

@atomiks
Copy link
Collaborator

atomiks commented May 18, 2024

I don't see the issue with 17.0.1 on the latest? Demo (your Sandbox is private btw)

Edit: nvm, I see the issue if it's not conditionally rendered based on isOpen. The docs currently recommend you need to conditionally render the FloatingPortal. The Sandbox hasn't been updated. Simplest solution is to switch to conditionally-rendering the FloatingPortal or upgrading to React 18+ (19 is coming out very soon anyway! I was considering dropping support of <18 entirely. It's a pain to deal with different behavior like this.)

@atomiks
Copy link
Collaborator

atomiks commented May 18, 2024

Found fix here: #2908 (it happens in only a somewhat specific case)

@mihkeleidast
Copy link
Contributor

Thanks, the fix & reasoning makes sense!

your Sandbox is private btw

Ugh, codesandbox makes it too many clicks nowadays to fork and share something. Fixed it now.

Re: updating to React 18 - yeah, totally on the roadmap, just it takes time as we have some enormous amount of ancient enzyme tests to refactor, as it does not work very well with newer React versions. In a large company this takes a long time unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants