feat(ulid): port /x/ulid module

[lino-levan]

Closes #3522.

I think this is valuable, particularly for Deno KV.

[iuioiua]

We probably only want to export the following from mod.ts:

• decodeTime()
• factory()
• monotonicFactory()
• PRNG
• ULID
  I think the others should be moved to a _util.ts or similar file.

[iuioiua]

We also probably want to include the examples from the README in the JSDocs.

[iuioiua]

Nice!

[iuioiua]

LGTM again

[kt3k]

@iuioiua Do you know the usage of monotonicFactory? I'm not sure what it's used for.

I'm also unsure we need to export factory. I guess nobody would replace pseudo random number generator.

I'd suggest we should only expose ulid and decodeTime functions (and related types) at least for first pass. What do you think?

[iuioiua]

I think monotonicFactory() is worth keeping. It's used for generating monotonically increasing ULIDs, which would be valuable in guaranteeing no conflicts when high numbers of ULIDs are generated within the same millisecond.

I agree with the rest of your points.

[kt3k]

Ok. Monotonicity looks useful. (This doc also mentions about guaranteeing of generation order).

While monotonicity is useful, monotonicFactory looks still redundant as it is for specifying alternative random number generator. I'd suggest we should replace it with non-factory version (such as ulidMonotonic or maybe generateMonotonic)

[iuioiua]

Yeah, it seems a little funny. It only increments monotonically if the last timestamp is less than or equal to the current timestamp (now). I think it's fine how it is with the name monotonicFactory. However, that's not a strong opinion.

Half-related: I think deviating from the ULID's JavaScript implementation is fine. Otherwise, we'd be putting ourselves into a box regarding capability. E.g. I found a possible tweak that could improve the performance of ULID generation by 10x while maintaining cryptographically random strings. Though, I'd have to validate it's correctness first 🤣

[iuioiua]

WDYT, @kt3k?

[lino-levan]

I would almost flip the naming to have ulid be monotonically increasing and some sort of non-monotonic ulid exist on the side? It feels like outside of migration, everyone should really use monotonic ulids.

[kt3k]

With monotonic version, the random bits are not random anymore, and it seems having far higher possibility of collision in distributed usage. I think monotonic version is not something we should recommend by default.

[lino-levan]

With monotonic version, the random bits are not random anymore

I missed that. This makes sense then. Completely agree that no one will replace the PRNG function. I'll rework the PR tonight if I can.

[iuioiua]

Oh, I completely missed that you suggested removing the random number generator parameter. Yes, that parameter looks mostly useless to me.

[lino-levan]

Return of the flaky tests I guess. This is ready for review @iuioiua.

[iuioiua]

Cool to see that change implemented! Few things:

• I don't think monotonicFactory() should have an encodeRand parameter or any parameter.
• This implementation deviates from the original JS implementation (e.g. faster encodeRandom() and no prng parameter. I think these differences should be pointed out in the JSDocs somewhere.

[lino-levan]

I don't think monotonicFactory() should have an encodeRand parameter or any parameter.

Strongly disagree. It is much harder to test without this parameter. It is not exposed to end users so I don't think this matters.

This implementation deviates from the original JS implementation (e.g. faster encodeRandom() and no prng parameter. I think these differences should be pointed out in the JSDocs somewhere.

Again, not exposed to the user so I don't know if that's relevant.

[iuioiua]

Regardless of my previous unreachable line comment, LGTM! This implementation is actually better than the original JavaScript implementation because:

1. Random strings are generated 10x faster.
2. The redundant prng parameter is removed. Fewer options are better.
3. Monontic ULIDs are possible without having first to create the factory.

Good stuff, @lino-levan and @kt3k!

[iuioiua]

I totally misread your initial suggestion, @kt3k. Now, I understand, and I like it 😎

[iuioiua]

Anything else pending for this PR?

[lino-levan]

Nothing pending afaik. Waiting for any comments from @kt3k.

[iuioiua]

I've noticed a bug with monotonicUlid() in denoland/saaskit#514. For some reason, ULIDs aren't respecting their seedTime parameters. However, ulid() works fine.

[lino-levan]

Could you write a reproduction? I suspect it may be a feature but I'm not too sure. @kt3k may know more about it.

[iuioiua]

Yeah, I'll get a repro snippet when I can.

[lino-levan]

Alright I've been staring at this code for too long. I don't see this bug. I'd appreciate a fresh set of eyes.

[iuioiua]

Sorry, I haven't had the chance to look into this further. @kt3k, would std/ulid be considered an unstable module once merged? If so, I think it'd be worth merging as-is.

[kt3k]

LGTM. Thank you for your efforts! @lino-levan

Note: Core team also looked in favor of including this.

[kt3k]

would std/ulid be considered an unstable module once merged?

Yes, we are planning to keep ulid unstable (versioned as 0.x) for a while.