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

fix: useId backward compatibility #165

Merged
merged 4 commits into from
Mar 14, 2024
Merged

Conversation

phsantiago
Copy link
Contributor

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
Copy link

vercel bot commented Aug 2, 2023

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
Copy link

gnapse commented Oct 4, 2023

Any word on the possibility to have this contribution accepted?

@LudovicGen
Copy link

Hello @pacocoursey and the team,

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

@julienba
Copy link

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

@pacocoursey
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link

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

@mt-deva
Copy link

mt-deva commented Mar 1, 2024

Any movement on this? Would be extremely useful

Copy link

vercel bot commented Mar 14, 2024

Deployment failed with the following error:

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

Copy link
Owner

@pacocoursey pacocoursey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like this actually works, thanks!

@pacocoursey pacocoursey merged commit 72e2137 into pacocoursey:main Mar 14, 2024
1 of 2 checks passed
@rafaell-lycan
Copy link

rafaell-lycan commented Mar 18, 2024

@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
Copy link

gnapse commented Mar 22, 2024

This is great. Thanks!

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

@ananth99
Copy link

ananth99 commented Apr 4, 2024

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

@varatep
Copy link

varatep commented Apr 22, 2024

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
Copy link

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

@mt-deva
Copy link

mt-deva commented Jun 28, 2024

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

@gnapse
Copy link

gnapse commented Jun 28, 2024

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.

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 this pull request may close these issues.