fix: useId backward compatibility

pacocoursey / cmdk

Fast, unstyled command menu React component.

https://cmdk.paco.me

MIT License

9.04k stars 259 forks source link

fix: useId backward compatibility #165

Closed phsantiago closed 3 months ago

phsantiago commented 11 months ago

Projects not using react 18 are not able to use CMDK lib. #162

This PR implements retro-compatibility for one of the hooks from React 18 used by the lib.

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
cmdk-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 10, 2024 4:08pm

gnapse commented 9 months ago

Any word on the possibility to have this contribution accepted?

LudovicGen commented 8 months ago

Hello @pacocoursey and the team,

Just a quick follow up on PR #165 for React backwards compatibility. Any news on its evaluation?

julienba commented 5 months ago

Hi, that would be great if you could merge this :)

pacocoursey commented 5 months ago

This doesn't seem to work at all. Try testing on the preview deployment — everything is broken. I believe React.useId is a strict requirement for this package to work in the way that it does.

phsantiago commented 4 months ago

Thank for the comment @pacocoursey. I fix some problems from the previous version:

It was generating a new id every render. Fixed with useState
It wasn't using useId from react when available. Fixed by checking it before using

Try testing on the preview deployment — everything is broken.

Could you test again? Now seems to be working fine. Should I duplicate the tests for running in react-16? Because now all tests passes but they are not using the compatibility code

I believe React.useId is a strict requirement for this package to work in the way that it does.

The only problem I couldn't solve was to have same Id from the ones server side generated, but all the rest is working fine now.

ntheanh201 commented 4 months ago

LGTM, I'm using the way you solved it to project running React 17

mt-deva commented 4 months ago

Any movement on this? Would be extremely useful

vercel[bot] commented 3 months ago

Deployment failed with the following error:

Resource is limited - try again in 1 hour (more than 100, code: "api-deployments-free-per-day").

rafaell-lycan commented 3 months ago

@phsantiago how about the useSyncExternalStore, have you found a decent workaround for it?

I've found a shim use-sync-external-store that could partially solve the issue, but I'm open to a different approach if you've found one.

gnapse commented 3 months ago

This is great. Thanks!

May I ask, when is this is expected to make it to a new package release?

ananth99 commented 3 months ago

Currently working on a project with React 16 and want to know when will the changes be available for release?

varatep commented 2 months ago

Heya @pacocoursey - bump on this, are we able to get a new release out that includes this change to accommodate React versions under 18? Is there anything else that needs to be done?

TIA!

hrishikeshjain commented 2 weeks ago

@pacocoursey - bump on this, please put on a new release so that we can use cmdk on React < 18 version.

mt-deva commented 1 week ago

@pacocoursey - another bump - waiting on this as we're stuck on react@17

gnapse commented 1 week ago

It's indeed such a pity that this is still stuck without a release. I had to go with an alternative, but I'm still hoping to try this one out. The fact that this library is explicitly about this use case makes me hope it'll be better than what I'm using now.