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

feat: move ID utilities to react-uid #142

Merged
merged 1 commit into from Jan 15, 2020

Conversation

austingreendev
Copy link
Contributor

@austingreendev austingreendev commented Dec 19, 2019

Description

Our current ID management solution uses a shared variable that is incremented on each call. This works well for basic use-cases, but is dependent on the render order to be consistent. This can cause issues with SSR and code-splitting scenarios.

We have also seen issues with consumers upgrading dependencies and having duplicate IDs from conflicting container-utilities versions.

Detail

A popular solution seems to be the uuid library, but this isn't viable for SSR and reproducible snapshot testing.

Looking through a react issue I found a reference to https://www.npmjs.com/package/react-uid which seems to match our use-case well.

Some highlights:

  • Provides TS types 😄
  • Includes SSR viable hooks for consumption in our standard hook containers
  • ID seed generation for more advanced use-cases if we need them in the future
  • Several context providers for consumers to customize and reset ID's based on their environment

@ryanseddon this seems like it could be a good utility for usage in the product specific codebases.

Checklist

  • 🌐 Storybook demo is up-to-date (yarn start)
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@austingreendev austingreendev requested review from ryanseddon and a team December 19, 2019 01:14
@austingreendev
Copy link
Contributor Author

Also, this PR is exploratory. There's no need to merge if we don't feel it's the right path.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Works for me. Will be interested in feedback from @ryanseddon.

@hzhu
Copy link
Contributor

hzhu commented Dec 19, 2019

Also, this PR is exploratory. There's no need to merge if we don't feel it's the right path.

There's a pull request to add a useUniqueId hook in React that is under technical discussion at the moment 👀.

@austingreendev
Copy link
Contributor Author

I've updated this to now include the package name and PACKAGE_VERSION with the UID seed generator. This allows multiple versions of react-uid to coexist correctly.

@ryanseddon
Copy link
Contributor

LGTM will be interesting if this can be superseded by a react built in hook proposed in facebook/react#17322

Is the reason to pull it out of container-utilities to avoid dependency mismatches?

@austingreendev
Copy link
Contributor Author

will be interesting if this can be superseded by a react built in hook proposed in

This would be the best path since react-dom is a peerDependency and could help ensure we only have a single state. It should be a drop-in replacement once it's ready.

Is the reason to pull it out of container-utilities to avoid dependency mismatches

Yes, but the main fix for that is including the package name and version in the seed generators. Moving to react-uid also helps with our SSR scenarios.

@austingreendev austingreendev merged commit 9f30572 into master Jan 15, 2020
@austingreendev austingreendev deleted the agreen/id-manager-deprecation branch January 15, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants