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

🐛 fix: zero not being a valid time #14

Closed
wants to merge 2 commits into from

Conversation

lukas-runge
Copy link

Hi @LiosK,

when running some of my unit tests using time mocking I discovered that a time of 0 currently spawns a RangeError even tho it's a perfectly valid time. This PR aims to fix that. 🙌

Best regards,
@lukas-runge

@lukas-runge lukas-runge changed the title 🐛 fix: zero is a valid time 🐛 fix: zero not being a valid time Sep 1, 2024
@LiosK
Copy link
Owner

LiosK commented Sep 2, 2024

Hi! It's currently a documented (positive integer) limitation of the generator.

Zero unixTsMs would break this condition when the generator is in the initial state (timestamp = 0). This limitation could possibly be lifted by initializing the generator with a negative timestamp, but at this stage I believe this limitation doesn't do a lot of harm because setting unixTsMs at zero is not a realistic use case.

@LiosK
Copy link
Owner

LiosK commented Sep 2, 2024

P.S. This limitation is introduced to this library because I reused the design of generator implemented in other languages, where timestamp is unsigned or initializing int at zero is very convenient.

@lukas-runge
Copy link
Author

lukas-runge commented Sep 2, 2024

Hi @LiosK,

thank you for your fast reply and for pointing out that I broke that condition in my special case and wasn't even generating proper uuids. 73cc1ab should fix this without impacting performance or handling, thereby lifting the limitation. 🥳 Even tho you are absolutely right, that this special case will most likely not occur in production, being able to test with mocked time starting from timestamp 0 is very convenient. As all data is usually initialized at the beginning of a every test, this would otherwise require initializing system time to some arbitrary value, which sounds more like a workaround than a solution. 🙌

Best regards,
@lukas-runge

@LiosK
Copy link
Owner

LiosK commented Sep 2, 2024

Hi! I'm currently not planning to lift this limitation, unfortunately. I'm maintaining several implementations that share the same design in different languages, so at this stage I don't really like to specialize the typescript implementation only.

LucilleH pushed a commit to jetify-com/opensource that referenced this pull request Sep 10, 2024
Greetings @jetify-com Team, 👋

when running some of my unit tests using time mocking I discovered that
a time of `0` currently spawns a `RangeError` in `uuidv7` even tho it's
a perfectly valid time. This special case will most likely not occur in
production, nevertheless being able to test with mocked time starting
from timestamp zero is very convenient. As all data is usually
initialized at the beginning of a every test, this would otherwise
require initializing system time to some arbitrary value, which sounds
more like a workaround than a solution. As this is a limitation of the
`uuidv7` library I proposed a fix to @LiosK that would lift this
limitation without impacting performance or handling:
LiosK/uuidv7#14 (comment) Sadly he
is not interested in lifting this limitation and won't merge the fix.

This PR aims to fix this issue on your end by migrating from
https://github.com/LiosK/uuidv7 to https://github.com/uuidjs/uuid which
seems to be more widely adopted and better maintained anyway. I also
tested performance and found it to be marginally better.

For a a quick reproduction of the issue checkout:
https://github.com/lukas-runge/typeid-zero-not-being-a-valid-time 🙌

Best regards,
@lukas-runge
jetpack-io-bot pushed a commit to jetify-com/typeid-js that referenced this pull request Sep 10, 2024
Greetings @jetify-com Team, 👋

when running some of my unit tests using time mocking I discovered that
a time of `0` currently spawns a `RangeError` in `uuidv7` even tho it's
a perfectly valid time. This special case will most likely not occur in
production, nevertheless being able to test with mocked time starting
from timestamp zero is very convenient. As all data is usually
initialized at the beginning of a every test, this would otherwise
require initializing system time to some arbitrary value, which sounds
more like a workaround than a solution. As this is a limitation of the
`uuidv7` library I proposed a fix to @LiosK that would lift this
limitation without impacting performance or handling:
LiosK/uuidv7#14 (comment) Sadly he
is not interested in lifting this limitation and won't merge the fix.

This PR aims to fix this issue on your end by migrating from
https://github.com/LiosK/uuidv7 to https://github.com/uuidjs/uuid which
seems to be more widely adopted and better maintained anyway. I also
tested performance and found it to be marginally better.

For a a quick reproduction of the issue checkout:
https://github.com/lukas-runge/typeid-zero-not-being-a-valid-time 🙌

Best regards,
@lukas-runge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants