feat!: Add uuidv4 to platform. #56
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been linked to Shortcut Story #192052: Look at removing nanoid dependency.. |
kinyoklion
commented
Mar 13, 2023
| } | ||
|
|
||
| uuidv4() { | ||
| return randomUUID(); |
There was a problem hiding this comment.
Our targeted versions of node should have this built-in. Which is better than a dependency. But it isn't something we can promise will be in all server SDKs.
There was a problem hiding this comment.
Less code = more better
kinyoklion
commented
Mar 13, 2023
| @@ -103,7 +102,7 @@ export default class DiagnosticsManager { | |||
| this.startTime = Date.now(); | |||
| this.dataSinceDate = this.startTime; | |||
| this.id = { | |||
| diagnosticId: nanoid(), | |||
| diagnosticId: platform.crypto.uuidv4(), | |||
There was a problem hiding this comment.
Now we use the platform provided uuidv4. (Platform here being our implementation of the platform. Like server-node,)
yusinto
reviewed
Mar 13, 2023
yusinto
approved these changes
Mar 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Nanoid was dependent on
Bufferswhich is not standard and caused some problems with some edge providers. It seemed better to remove it now.Also originally I had switched from uuid to nanoid for a similar compatibility reason. Now that dependency is removed from common code and pushed to the platform. So the platform can use the UUID implementation that works for it.
For edge environments I hope most of them support
crypto.randomUUID. Assuming that the securecontext requirement is not applicable, because it is server side, or that it doesn't interfere with their operation.This change should generally make it easier to support uuidv4 generation without requiring a polyfill in the shared packages.