-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add useUniqueId hook and fix several needlessly incrementing ids #2079
Conversation
This hook returns a stable value across multiple rerenders, and can optionally be overridden with a fixed value Use this hook in several functional components that currently have unstable ids that change between rerenders. Populate the idFactory of this hook in the AppProvider, with eye to eventually allowing consumers to override the idFactory so it can be reset between server renders (making this public and configurable is a follow up step)
0d9addb
to
14fd04a
Compare
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.
A few minor suggestions for consistency but overall this is great! Thank you for doing this Ben 🎉
6c616fe
to
484472e
Compare
873affa
to
76a7808
Compare
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.
Agreed with Tim about tests being hard to read.
I also am a bit wary of overusing hooks, but I think this use case is fine. It's a simple hook that lets developers focus on the functionality they want to use instead of React internals, and prevents easy mistakes from happening.
- Use PascalCase for default global prefix - Fix typos - Rejig tests to create elements a little more like what we would write in real components
7669582
to
6ddf629
Compare
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.
The assertions are still not easy to read, but they're just tests so it's fine
* Add useUniqueId hook This hook returns a stable value across multiple rerenders, and can optionally be overridden with a fixed value Use this hook in several functional components that currently have unstable ids that change between rerenders. Populate the idFactory of this hook in the AppProvider, with eye to eventually allowing consumers to override the idFactory so it can be reset between server renders (making this public and configurable is a follow up step)
WHY are these changes introduced?
We want components to have stable ids between rerenders, otherwise their ids will constantly increment on every render which leads to additional wasteful dom reconciliation.
Currently that's not easily possible with functional components.
WHAT is this pull request doing?
Adds a useUniqueId hook that returns an ever incrementing ID when called multiple times but remains constant through rerenders. See the hook tests for examples of how this works in practice.
This hook returns a stable value across multiple rerenders, and can optionally be overridden with a fixed value for components that allow the user to provide their own id.
Use this hook in several functional components that currently have unstable ids that change between rerenders.
The idFactory for this hook is populated in the AppProvider, with eye to eventually allowing consumers to override the idFactory so it can be reset between server renders (making this public and configurable is a follow up step). See #1761 for info where we should go onwards with that. Eventually having this be a standalone library in quilt would be good but I've not got the time to split this out at the moment so I'm doing something just within Polaris for now
What comes next?
This converts all the currently functional components that use ids to use this hook. Class based components which call their generateId function within a render instead of calling it when initializing a private instance variable also exhibit this behaviour. We could either fix those now, or as part of migrating those components to use hooks - I'm leaning towards the latter.
We will want to make the generator factory we pass into the
UniqueIdFactory
in the AppProvider that gets passed into the by the consumer so that it can be reset on each server render to complete #1761How to 🎩
The following playground contains a bunch of components that trigger rerenderings of a component on a once per second basis thanks to setInterval incrementing a counter. See that only thing that updates within each component is the count content and that any id values remain constant across multiple renderings of the same component.
You can compare this by running this playground on master (but commenting out the
Test
component stuff) and seeing that various id values within the components also update.Copy-paste this code in
playground/Playground.tsx
: