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(core): replace uuid dependency with crypto.randomUUID() #976

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

manzt
Copy link
Member

@manzt manzt commented Sep 28, 2023

Fix #
Toward #

Change List

  • Removes uuid and @types/uuid as direct dependencies
  • Adds src/core/utils/uuid.ts, which exports a uuid function

Checklist

  • Ensure the PR works with all demos on the online editor
  • Unit tests added or updated
  • Examples added or updated
  • Documentation updated (e.g., added API functions)
  • Screenshots for visual changes (e.g., new encoding support or UI change on Editor)

@@ -723,7 +723,7 @@ function Editor(props: RouteComponentProps) {
}
return (
<div
key={v4()}
key={uuid()}
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a side observation. I'm guessing the uuid is used here to please eslint-react, but I don't think key-ing with a UUID is desirable. Ideally it would be some piece of data from the array of objects that is deterministic between renders.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for finding this! This should have used the id of the view or track instead. I will update this after testing it.

@manzt manzt changed the title feat: replace uuid dependency with crypto.randomUUID() feat(core): replace uuid dependency with crypto.randomUUID() Sep 28, 2023
@etowahadams etowahadams mentioned this pull request Oct 6, 2023
5 tasks
@manzt manzt requested a review from etowahadams October 26, 2023 19:18
@manzt manzt merged commit 9b1e89b into master Oct 26, 2023
4 checks passed
@manzt manzt deleted the manzt/uuid branch October 26, 2023 21:37
@etowahadams
Copy link
Contributor

Looks good! Wondering if in the future we should also replace instances of HGC.libraries.slugid.nice(); with this new uuid function too. Some of the datafetchers use this.

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.

3 participants