-
Notifications
You must be signed in to change notification settings - Fork 53
Adding support for randomUUID #197
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
base: master
Are you sure you want to change the base?
Conversation
.map((e) => e.toRadixString(16).padLeft(2, '0')) | ||
.join() | ||
.replaceAllMapped( | ||
RegExp(r'(\w{8})(\w{4})(\w{4})(\w{4})(\w{12})'), | ||
(m) => '${m[1]}-${m[2]}-${m[3]}-${m[4]}-${m[5]}', | ||
); |
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.
I've done this other places the classical implementation is:
final bytes = _randomBytes(16);
// Set V4 bits according to:
// https://tools.ietf.org/html/rfc4122#section-4.4
bytes[6] = (bytes[6] & 0x0f) | 0x40;
bytes[8] = (bytes[8] & 0x3f) | 0x80;
// Encode as UUIDv4
final s = hex.encode(bytes).substring;
return '${s(0, 8)}-${s(8, 12)}-${s(12, 16)}-${s(16, 20)}-${s(20)}';
Maybe, using toRadixString
is nicer, since it avoids a dependency on package:convert
.
final out = scope<ffi.Uint8>(16); | ||
_checkOp(ssl.RAND_bytes(out, 16) == 1); | ||
|
||
var bytes = out.asTypedList(16); |
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.
Maybe, we could just use fillRandomBytes
, then we have less FFI stuff going on.
check(uuid[8] == '-'); | ||
check(uuid[13] == '-'); | ||
check(uuid[18] == '-'); | ||
check(uuid[23] == '-'); |
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.
Check the bits too
/// } | ||
/// | ||
/// ``` | ||
String randomUUID() => webCryptImpl.random.randomUUID(); |
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.
This generates a UUIDv4, there is a specification let's explain that:
https://tools.ietf.org/html/rfc4122#section-4.4
/// } | ||
/// | ||
/// ``` | ||
String randomUUID() => webCryptImpl.random.randomUUID(); |
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 name of the function in Dart should follow conventions:
https://dart.dev/effective-dart/style#do-capitalize-acronyms-and-abbreviations-longer-than-two-letters-like-words
randomUuid()
, however, ugly 🙈
We could choose to just call it uuid()
, I don't know. We do change names from Javascript when implementing them in Dart, like how getRandomValues
became fillRandomBytes
.
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.
@HamdaanAliQuatil any thoughts on the name?
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.
uuid()
looks good.
To add to this - based on the last discussion on randomUUID
on w3c
, support for UUIDv7 (or other variants), if added, would be in the form of parameters.
If someday webcrypto introduces ULID, it will likely be another in another class and that would be an issue for another day.
As for now, and for the near future as well, uuid()
solves our purpose.
Thank you for your feedback! I am currently in the middle of my examinations and will make the requested changes as soon as I get time. |
Implement
randomUUID
inwebcrypto.dart
Changes
randomUUID()
method.randomUUID
implementation, so implemented it based on the W3C spec.Testing
Notes