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

🐛 typeid-js: fix zero not being a valid time #373

Merged
merged 8 commits into from
Sep 10, 2024

Conversation

lukas-runge
Copy link
Contributor

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

@LucilleH LucilleH self-requested a review September 5, 2024 16:35
Copy link
Contributor

@LucilleH LucilleH left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion! Looks reasonable to me.

prefix: "",
uuid: "00000000-0000-0000-0000-000000000020",
uuid: "00000000-0000-7000-b000-000000000020",
Copy link
Contributor

@LucilleH LucilleH Sep 5, 2024

Choose a reason for hiding this comment

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

@lukas-runge can these test cases stay the same, or do you need this to be changed? Prefer that all the test cases stay the same just to be completely sure this is not a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LucilleH, the test cases do need to be changed because they are not compliant with RFC 9562, the proposed standard defining UUIDv7. For a UUIDv7, the version field must always be set to 0b0111 (hex: 7), and the variant field must be 0b10 (hex: 8, 9, A, or B, as the other two bits are randomly assigned). These changes should not cause any breaking issues unless there is incorrect validation of UUIDv7s somewhere, which would be a bug that needs fixing regardless.

@LucilleH LucilleH merged commit 1edfc67 into jetify-com:main Sep 10, 2024
2 checks passed
@lukas-runge
Copy link
Contributor Author

@LucilleH thank you for merging! 🙏 Is there any ETA on when this will be published to npm? 🙌

Best regards,
@lukas-runge

@LucilleH
Copy link
Contributor

@lukas-runge published!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants