-
Notifications
You must be signed in to change notification settings - Fork 645
refactor(useId): update useId to use React.useId, update ssr utils #3982
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
Conversation
🦋 Changeset detectedLatest commit: d10aa5f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
broccolinisoup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR
Looks beautiful! ✨
Would appreciate a look at the type signatures for the changed exports to make sure they are equivalent
All makes sense to me ✅
Verify that id generation still works as expected in components which use this hook
Checked the Tooltip and UnderlineNav, both are golden ⭐
Double-check the rollout plan to see if I missed anything 👀
Based on https://github.com/github/primer/issues/1715#issuecomment-1830411602, I just wanted to say that I LOVE the migration plan. Great example of how to gradually move from A to B and from B to C 😊
One question about the deprecating the SSRProvider and the useSSRSafeId , should we do it in this PR or a follow up PR is better? Curious to hear your thoughts.
siddharthkp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Background for these changes: https://github.com/github/primer/issues/1715#issuecomment-1830411602
Update SSRProvider, useSSRSafeId to use the native React 18 useId() instead of @react-aria/ssr
Changelog
New
Changed
SSRProviderto noopuseSSRSafeIdto use our internaluseIduseIdto use the nativeReact.useId()hookRemoved
@react-aria/ssrfrom the projectRollout strategy
Hopefully this can be contained in a minor release as the changes should all be compatible 🤞 This PR does end up removing support for
@react-aria/ssrand replaces it but we could also keep the dependency in the project and remove it later if it ends up being problematic. This work should definitely be tested out in dotcom to see what the impact is.Testing & Reviewing