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

Duplicated useId values #3757

Closed
1 task
grefrit opened this issue Oct 5, 2022 · 7 comments · Fixed by #3758
Closed
1 task

Duplicated useId values #3757

grefrit opened this issue Oct 5, 2022 · 7 comments · Fixed by #3758

Comments

@grefrit
Copy link

grefrit commented Oct 5, 2022

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
Sometimes useId() values are duplicated. I saw this strange behavior in @headlessui/react components. Headlessui uses useId only when available (to support React 17). In development I use React 17 and everything looks normal, but in production I'm switching React to Preact 10.11.1 and see this in some situations
image
As you can see values are duplicated. I can confirm that is it issue in Preact, not headlessui, because it works when headlessui not using useId hook. It is very strange that this does not happen in all elements, but only in some. I said about headlessui, because I'm not using useId hook myself, so I don't know how to reproduce this behavior. I will try to do my best and provide some reproduction later. Also I can add that this strange behavior happens in this two elements
image

So to sum up.

  1. This happens sometimes, I can't find some logic when it happens, it just duplicates value, very strange
  2. I told about headlessui, because I don't use useId hook myself and I don't know how to reproduce this

To Reproduce
Try to provide soon
UPD: #3757 (comment)

Expected behavior
Values of useId should be unique

@marvinhagemeister marvinhagemeister added needs-more-info The issue doesn't contain enough information to be able to help and removed needs-more-info The issue doesn't contain enough information to be able to help labels Oct 5, 2022
@marvinhagemeister
Copy link
Member

marvinhagemeister commented Oct 5, 2022

Found a minimal reproduction case:

import { useId } from "preact/hooks";

function Foo() {
  const id = useId();
  return <p>{id}</p>;
}

function App() {
  return (
    <div>
      <Foo />
      <>
        <Foo />
      </>
    </div>
  );
}

@JoviDeCroock
Copy link
Member

Hmm that makes sense, we have a condition to skip fragments. We should disable that and ensure we leverage a root-wrapping fragment in render to string

@marvinhagemeister
Copy link
Member

@JoviDeCroock currently trying that out, but getting mismatches with render to string. My hunch so far is that the implicit Fragment wrapping we do for array children is different from core.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Oct 5, 2022

Here is a test case where I can force mismatches with RTS:

function Foo() {
  const id = useId();
  return <p>{id}</p>;
}

function Bar(props) {
  return props.children;
}

function App() {
  return (
    <Bar>
      <Foo />
      <Fragment>
        <Foo />
      </Fragment>
    </Bar>
  );
}

@JoviDeCroock
Copy link
Member

Concretely to solve this I think we need to:

I might be forgetting a case with the implicit fragment wrapping though 😅 don't have a lot of time this week but can look over the weekend.

@Armster15
Copy link

Unless if I'm doing something wrong, this issue seems to still be occurring even with 10.11.12.
Like grefrit, I too am using Headless UI and the IDs generated by Preact seem to be non unique.

A reproducible repository: https://github.com/Armster15/preact-duplicate-id
Live Demo: https://preact-duplicate-id.vercel.app/
An image showing the duplicate IDs:
Screenshot 2022-11-12 at 2 04 11 PM

For others coming across this issue, downgrading to Preact 10.10.6 worked for me. This release does not include a useId hook, and as such, Headless UI will provide its own implementation of the hook, which seems to be working as intended.

@marvinhagemeister
Copy link
Member

@Armster15 That's because we haven't cut a new release yet. Will do so momentarily.

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

Successfully merging a pull request may close this issue.

4 participants