-
-
Notifications
You must be signed in to change notification settings - Fork 903
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
uuid v7 #681
uuid v7 #681
Conversation
note, husky pre-commit hook was bypassed for this commit
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.
Thanks for the contribution @pmccarren ccarren. This looks promising!
In addition to the inline comments, can you add a unit test alongside https://github.com/uuidjs/uuid/blob/main/test/unit/v1.test.js, with similar assertions. Specifically, I'd like to see similar tests for sort order, uniqueness, and options handling. (And anything else you think may be relevant.)
The sorting will be of particular interest, since producing UUIDS that sort the same whether sorted by timestamp or lexicographically was pretty much the whole point of version 7. 😄
Note: This should not close #580, as the new RFC adds v6 and v8 formats, too, as well as the MAX_UUID constant. v7 is definitely the majority of what's needed, but we'll want to keep 580 open until we fill in the remaining items.
Lastly, the new RFC is still making it's way through the review process. I don't expect substantive changes at this point, so this is an appropriate time to be adding this. But as I noted in 580, I think it's prudent to go with an experimental version release tag of some sort.
@ctavan: Thoughts on how best to accept this? Should we create a v9-experimental
branch that we merge this into rather than master
? Then publish experimental releases from that?
@broofa - glad to lend a hand! I appreciate your time reviewing the implementation. This PR has been updated with your suggested changes, most notably:
In regards to sorting, I've been considering a few different approaches and while not presently implemented in this PR, I absolutely agree it should be :) I'll work on implementing this as well the associated unit tests. |
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.
Code looks good 👍
Lastly, the new RFC is still making it's way through the review process.
Thoughts on how best to accept this? Should we create a v9-experimental branch that we merge this into rather than master? Then publish experimental releases from that?
Personally think that we should keep this PR open until the spec has been ratified. If we merge into a v9
branch that would stop us from making a v9 release until that has happened, and that might potentially be a long time?
I think that it would be nice to backport the "version 2 is valid" bugfix right away to the current release though?
Thanks an lot for the contribution! I initially thought this new feature could land in the v9 release, but given we’re broadening the validation regex it’s a breaking change and we’ll have to bump the major version. I think it would be nice to somehow release it as a prerelease so that people have a chance to try it and provide feedback, but I think our current release and branching process is not yet ready for multiple active major releases. If anyone’s willing to take a look at this I’m happy to support (we were considering better automation for this for a while… #636). I agree however, that we should not release this in a public major release until the draft has been accepted. |
Note: I've just created the Wish I could give you an ETA for that, but it's not been a priority for either of us. As he says... "Help Wanted". |
Co-authored-by: Linus Unnebäck <[email protected]>
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.
@ctavan @LinusU Any other comments? If not, I'd like to get this merged (pending @pmccarren commiting his suggested change for the regex to allow 1-8
).
Regarding the plan for the rfc4122bis
branch, I've created a project to capture the remaining work, with brief notes on possible implementation details.
Honestly, there's not all that much left... if anyone is feeling inspired. ;-)
Lastly, thoughts on inviting Patrick to join the UUIDJS org? He's done some solid work here that's much appreciated, and seappears to have good street cred. If people could weigh in on that idea (including you, Patrick), we can move forward or not as appropriate. [cc'ing @TrySound here]
@broofa just merged the As for UUIDJS Org, I'll let my work speak for itself but will add that if you'll have me I'll be an active participant :) |
@broofa I just finished adding monotonic support, and it's ready for review! Took a bit of finesse but I'm rather happy with how it turned out. In addition to the monotonicity related unit tests, I manually tested sorting preservation during generation of 100M uuids. couple of sequence counter implementation notes:
Pending review, I believe this PR is ready to go now! |
Eagerly awaiting this to be released as well. Thanks for the great library and work on it! |
I'll play ChatGPT here and summarize the current status:
My take would be UUIDs become less useful if you're going to alter the date input and don't strictly stick to modifying the sequence when sequencing. The primary driving factor of this is that a randomly initiated sequence could initialize 1 count below the max sequence and leaving padding atop the sequence reduces the entropy of the sequence. This Hacker News thread has more discussion on the ULID spec which UUIDv7 was largely based on: https://news.ycombinator.com/item?id=36447837
This discussion seems to imply other implementations ALWAYS re-seed the random bits and increment the counter. I'm not really sure if I would prioritize sequential over randomness and perhaps this does need to be a preference. Generally, those seeking UUIDv7 for randomness security through obscurity/incalculability should be using another UUID. Edit 1: Multi-node behavior should be considered as well as single-node sequence generator node behavior because I believe UUIDv7 in this library could be used in both forms. Each has slightly different preferences. For multi-node, all of this discussion goes out the window and it's best to not modify the Edit 2: Within each node of a multi-node cluster, I may wish to prefer sequential sort and it might be acceptable to eat into the MS up to 1-3 milliseconds for my usecases but I still feel uncomfortable. I'd rather OPT IN to sacrifice 10% of my sequence entropy than have this behavior though. Edit 3: Percona notes UUID_SHORT from MySQL allows the sequence to rollover. |
Rumor has it uuid v7 is now part of the proposed standard What would it take to get this over the finish line? |
Any updates on this? |
Did another review pass. Pushed a couple minor documentation tweaks to update things now that RFC9562 is official. @ctavan: In the interests of moving forward with 9562, I'm merging this. If there are questions or concerns, we can address those in followup PRs. @pmccarren: Thanks for the great work on this. My apologies for letting this stagnate for so long. Now that 9562 is official, my goal is to get this pushed out ASAP. |
I‘m all for it, thanks for taking care! But let’s make use of the conventional commit message format in the squash commits so we get proper autogenerated changelogs. |
Ugh... I keep screwing that up. :-( |
Does GitHub have an option to run actions (commitlint) before squash merges? |
There's probably a way, but... 🤷 |
@broofa and @ctavan I appreciate your time in reviewing and assisting with the PR. I'm excited we're collectively one step closer to uuid7 being widely available :) If there is a need for assistance, I have time I can dedicate towards experimental deploys. Just let me know if it'll be helpful, and I'm happy to dive in. |
@pmccarren That'd be awesome. On the deploy front, there's two nuts we'd like to crack:
We haven't tackled the former yet, but if you know a way to do that, we're all for it. For the latter, I put up #725 a couple years ago before getting distracted by other things. The main goal there was to get away from our current ad-hoc release process that has us creating PRs just to update
I've sent you an invite to the And thank you for stepping up to help (again). |
@broofa / @pmccarren Now that the changes for uuid v7 are merged, what's the plan for release on npm? |
See #760 |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [uuid](https://github.com/uuidjs/uuid) | dependencies | major | [`^9.0.0` -> `^10.0.0`](https://renovatebot.com/diffs/npm/uuid/9.0.1/10.0.0) | --- ### Release Notes <details> <summary>uuidjs/uuid (uuid)</summary> ### [`v10.0.0`](https://github.com/uuidjs/uuid/blob/HEAD/CHANGELOG.md#1000-2024-06-07) [Compare Source](uuidjs/uuid@v9.0.1...v10.0.0) ##### ⚠ BREAKING CHANGES - update node support (drop node@12, node@14, add node@20) ([#​750](uuidjs/uuid#750)) ##### Features - support support rfc9562 MAX uuid (new in RFC9562) ([#​714](uuidjs/uuid#714)) ([0385cd3](uuidjs/uuid@0385cd3)) - support rfc9562 v6 uuids ([#​754](uuidjs/uuid#754)) ([c4ed13e](uuidjs/uuid@c4ed13e)) - support rfc9562 v7 uuids ([#​681](uuidjs/uuid#681)) ([db76a12](uuidjs/uuid@db76a12)) - update node support matrix (only support node 16-20) ([#​750](uuidjs/uuid#750)) ([883b163](uuidjs/uuid@883b163)) - support rfc9562 v8 uuids ([#​759](uuidjs/uuid#759)) ([35a5342](uuidjs/uuid@35a5342)) ##### Bug Fixes - revert "perf: remove superfluous call to toLowerCase ([#​677](uuidjs/uuid#677))" ([#​738](uuidjs/uuid#738)) ([e267b90](uuidjs/uuid@e267b90)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or rename PR to start with "rebase!". 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
UUID V7
This PR implements UUID V7,
closingref #580V7 is implemented, tested and documented and ready for review! No changes to the other versions were made.
Relevant RFC documents:
Benchmarks
V7 appears as fast as V4 when not using native crypto.randomUUID generation.
Results:
TODO: