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: move to rrule-rust #10181

Merged
merged 6 commits into from
Sep 11, 2024
Merged

fix: move to rrule-rust #10181

merged 6 commits into from
Sep 11, 2024

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Aug 30, 2024

Description

fix #9722

rrule is stalling our gql executor whenever it runs. since it is sychronous, that means all pending requests get timed out.
image

there's a rust version without the bugs, so this moves to that.
The API is different, primarily dtstart and tzid only exist on RRuleSet.
Additionally, I found a ton of bugs with how we were handling timezones, so I fixed the ones I could find.

Most notably is how we handle timezones on the client.
Before, when you edited a rrule that was created in a different timezone, it kept the time the same, so it reported 11PM eastern as 11PM pacific.
Now it reports that accurately.

TEST

  • create gcal event
  • process
  • update

@tianrunhe tianrunhe removed their request for review September 3, 2024 17:32
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Works as far as I can tell. It would be nice to have fewer different date representations in the code base, like getting rid of the RRule package on the client side, but it's good for now.

dayjs.extend(utcPlugin)
dayjs.extend(timezonePlugin)

// the RRule package requires dstart to be a date object set to a negative UTC offset. It's ugly!
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I don't see where we set a negative UTC offset here or what that means.

Copy link
Member Author

@mattkrick mattkrick Sep 10, 2024

Choose a reason for hiding this comment

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

Same as the old helper functions in this file, the rrule package stores dtstart as a Date, but interprets the local time as UTC time. e.g. it'll create a Date of 1/1/2020 8:00AM PST and then interpret that as 1/1/2020 8:00AM UTC. It's pants-on-head stupid. jkbrzt/rrule#501 (comment).

The best thing we can do is use dayjs everywhere we can since it supports timezones & then when we have to ocnvert it to an rrule, we use the helper function.

Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
@mattkrick mattkrick merged commit 2952c3d into master Sep 11, 2024
7 checks passed
@mattkrick mattkrick deleted the fix/rrule branch September 11, 2024 17:57
@github-actions github-actions bot mentioned this pull request Sep 11, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: processRecurrence is slow
2 participants