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

feat(web-analytics): Add timestamp utils and uuidv7 code to plugin-server #27070

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

robbie-c
Copy link
Member

Problem

Pulled out of #25915 to make it easier to review

Changes

Added a UUIDv7 class, and some extra timestamp utils which will be used in cookieless mode

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Added some tests

@robbie-c robbie-c changed the title feat(web-analytics): Add timestampp utils and uuidv7 code to plugin-server feat(web-analytics): Add timestamp utils and uuidv7 code to plugin-server Dec 19, 2024
@robbie-c robbie-c requested review from rafaeelaudibert and a team December 19, 2024 17:05
Copy link
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

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

LGTM, left a non-blocking comment

Comment on lines +136 to +138
if (!year || !month || !day) {
throw new Error('Failed to get year, month, or day')
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this can ever happen, formatToParts will always return a value. If you attempt to pass anything that couldn't be formatted it will raise RangeError

image

Are you doing this for TS purposes? If so, because formatToParts never fails to return a value we might do this to collect the values instead - it's deterministic AFAICT

const [month, , day, , year] = parts

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is just to make typescript happy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it was - I looked through the docs and I do think the order is guaranteed https://tc39.es/ecma402/#sec-formatdatetimetoparts but I'm not confident enough to assume this

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks like code 👍

// we set fully random values for rand_a and rand_b

const array = new Uint8Array(16)
// 48 bits for time, WILL FAIL in 10 895 CE
Copy link
Collaborator

Choose a reason for hiding this comment

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

JS integers are also 48 bits (out of a 64 bit number), so there will be bigger problems

Comment on lines +136 to +138
if (!year || !month || !day) {
throw new Error('Failed to get year, month, or day')
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is just to make typescript happy?

@robbie-c robbie-c merged commit 8035e01 into master Dec 20, 2024
92 checks passed
@robbie-c robbie-c deleted the feat/add-timestamp-utils-and-uuidv7 branch December 20, 2024 13:22
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.

3 participants