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

crypto: add randomULID function #49074

Closed
wants to merge 3 commits into from
Closed

Conversation

Xstoudi
Copy link
Contributor

@Xstoudi Xstoudi commented Aug 8, 2023

I propose to land randomULID in the crypto subsystem.

ULID are lexicographically sortable identifiers contaning both the time it was generated and a random part. Spec can be found here: https://github.com/ulid/spec

ULID implementation exists in userland, currently getting around 768'000 downloads per week: https://www.npmjs.com/package/ulid
Since its release, popularity constantly grew: https://npmtrends.com/ulid

I will be happy to get feedbacks on the implementation and I'm obviously open to discuss if you have reluctance to land it in Node.js.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 8, 2023
@mscdex
Copy link
Contributor

mscdex commented Aug 8, 2023

Why does this need to belong in node core?

@Xstoudi Xstoudi force-pushed the feat/ulid branch 4 times, most recently from 3ea8b1e to 7929db6 Compare August 8, 2023 23:15
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @Xstoudi!

randomUUID() exists in core for various reasons. UUIDv4 is a very popular format, to the point where DBMS have added data types to represent it. Perhaps most importantly, randomUUID() is standardized through the Web Cryptography API. If APIs for some other format (newer UUID formats or even ULID) were to be standardized through W3C, Node.js would likely adopt the relevant APIs as well.

Even though Node.js, Deno, Bun, and 90% of browsers have supported randomUUID() for years, the uuid package on npm still sees more than 90 million downloads per week. That's more than 100 times the popularity of the ulid package despite the feature already being in core.

UUIDv4 is the most common variant of UUIDs. Not only is randomUUID() standardized as mentioned above, but also the UUID format itself is specified in RFC 4122.

ULID has a few advantages over UUIDv4 in some scenarios, most of which are minor. However, some are significant for a certain applications. But even then, there are alternatives to ULID. The IETF draft "New UUID Formats" describes multiple new UUID formats, some of which have similar characteristics.

At this point, I don't see why ULID belongs in core. Compared to UUIDv4, its popularity is marginal at best, and there are several other candidates for standardization. Lastly, adding randomULID() only to the node:crypto module but not to the WebCryptoAPI crypto object seems confusing.

If you are optimistic about this feature, I'd suggest to perhaps discuss this in an issue in the Web Crypto API repository first. Based on the little information I have about ULID, I don't expect it to be accepted there, but if it is, that would justify addition to Node.js and several other JavaScript runtimes.

@Xstoudi
Copy link
Contributor Author

Xstoudi commented Aug 9, 2023

Hello @tniessen , thanks a lot for your answer!
I completely agree on the may be popularity lack of ULID and if it is the sole reason for it to not land in core, I'd comprehend that.

randomUUID() exists in core for various reasons. UUIDv4 is a very popular format, to the point where DBMS have added data types to represent it. Perhaps most importantly, randomUUID() is standardized through the Web Cryptography API. If APIs for some other format (newer UUID formats or even ULID) were to be standardized through W3C, Node.js would likely adopt the relevant APIs as well.

I am not really sure DBMS data type example is relevant as it is often just a sugar alias for fixed-size text or blob/binary. By that I mean that it's most of the time not needed, but nice to have.
Concerning the "standardized" part, Web Crypto API's randomUUID is definitely standardized, yes. But crypto's is not as far as I know.

Even though Node.js, Deno, Bun, and 90% of browsers have supported randomUUID() for years, the uuid package on npm still sees more than 90 million downloads per week. That's more than 100 times the popularity of the ulid package despite the feature already being in core.

core's randomUUID and uuid package is not a 1-1 feature match, I think it may be important to keep that in mind at some degree. uuid package offer v1, v3, v4, v5, parsing, validation, compatibility with other platforms as well.

That say, using this as an argument, we could argue that userland packages pify, util.promisify, @google-cloud/promisify each somehow cumulating millions of weekly downloads may invalide util.promisify being in the core, but they each offer bonus things like bringing the feature elsewhere as it's not standardized, retro-compatibility with older Node.js version (yet I genuinely hope there is not 13M weekly download on util.promisify userland package for the only sake of supporting Node.js < 8.0.0). And let also admit that, beside core's util.promisify historically being here for smoothing the transition to promise API (for as much as I can remember, correct me if I'm wrong), it's also here because it's convenient to have it.

It leads me to think that only focusing on package popularity can be both a reason to land in core and to not do it, depending on individual opinion about the feature itself.

UUIDv4 is the most common variant of UUIDs. Not only is randomUUID() standardized as mentioned above, but also the UUID format itself is specified in RFC 4122.

Yes, but I argue that randomUUID in crypto is not standardized for as much as I know. It's just there and it's fine.

ULID has a few advantages over UUIDv4 in some scenarios, most of which are minor. However, some are significant for a certain applications. But even then, there are alternatives to ULID. The IETF draft "New UUID Formats" describes multiple new UUID formats, some of which have similar characteristics.

I'd be more than happy to try to propose an implementation for UUIDv7 if the draft being a draft is not fully blocking, and if current lack of popularity (as obviously it's still a draft) is not either.
Thus, I completely agree that UUIDv7 would be a sweet spot for time-based random identifier being available conveniently in core.

At this point, I don't see why ULID belongs in core. Compared to UUIDv4, its popularity is marginal at best, and there are several other candidates for standardization. Lastly, adding randomULID() only to the node:crypto module but not to the WebCryptoAPI crypto object seems confusing.

Marginal popularity: agreed.
As a user of Node.js for some years now, certainly not being the most implicated in the development of Node.js, the confusing part to me about this is having two crypto APIs in core both covering, at some extent, the same needs. Yet I trust collaborator to ensure it makes sense. That said, if some node:crypto <--> Web Crypto API functionnality link must be respected when adding to one or the other, it may be useful to indicate it somewhere (I also may have missed it).

If you are optimistic about this feature, I'd suggest to perhaps discuss this in an issue in the Web Crypto API repository first. Based on the little information I have about ULID, I don't expect it to be accepted there, but if it is, that would justify addition to Node.js and several other JavaScript runtimes.

Well if I wanted to add it in Web Crypto API, I would probably have done that, but time-based UUID were already briefly noticed as not so relevant to browser in the randomUUID discussion about supporting future UUID formats : WICG/uuid#36 so ULID have definitely no chance to land if UUIDv7 didn't and would have definitely no reason to land if UUIDv7 would have. Yet I still think some time-based random identifier is relevant in Node.

Anyway, thanks @tniessen for taking the time to review this!

@jasnell
Copy link
Member

jasnell commented Sep 14, 2023

I've got to agree with @tniessen on this. I'm -1 on landing this in core. Also, given the lack of further discussion for over a month, I think we can close this.

@jasnell jasnell closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants