-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor(uuid): [noticket]: Replace uuid package with generateId util #184
Conversation
@everyone I'm very open to other proposals regarding this refactoring. |
describe('generateId', () => { | ||
it('should return two different ids', () => { | ||
const firstId = generateId(); | ||
const secondId = generateId(); | ||
|
||
expect(firstId).not.toEqual(secondId); | ||
}); |
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.
Not sure if it'd be worth it to write regression tests for the format xxxx-xxxx-xxx-xxxx
and/or length of the string? (only if it's relevant)
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.
You mean regression as a way to make sure it's compatible with previous usages or for future proof?
Instead of cooking up our own solution here which, in theory, could still produce identical ids, would maybe using https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID be an alternative here? It looks like browser support is solid |
fc79e49
to
40251b1
Compare
Seems like Updated it to use this function by broofa that is one of the contributors to the uuid library and, I believe, part of the team that implemented randomUUID. Still defaulting to Math random for places where we might run into an issue. Testing should be one those cases. |
That's a shame, but I guess that's all that can be done at this point. Thanks 👍 |
100%, pretty annoying that we have to go through such hassle just to create an ID when there's even a function already for it 😅 |
Can you elaborate on this? The |
|
After digging up a bit, I think this might be related to the fact that we were using the v0.8.3 instead of the most recent one where the minified UMD build was removed so there shouldn't be an issue with it. I'll take a look and try to replicate it again later and let you know of what I find. |
What this PR does
Replaces uuid package with a custom and simpler generateId util.
Why is this needed?
uuid package can't be tree-shaked and was adding a giant additional size to repos where it was being imported.
Usually, using random for random ids is not the most efficient way but since this ids are not persistent and used for runtime only (generate label/input ids, temporary ids for error entries) this downgrade from uuid might make sense.
Named the util generateId so it doesn't suggest it is a fully and standardised uuid and replaces the function.
The id generated is based on a random number that is then parsed into an hex string for each of the characters.
Checklist:
Or give a reason why this does not apply:
Or give a reason why this does not apply:
Browser support
My code works in the following browsers: