-
-
Notifications
You must be signed in to change notification settings - Fork 440
Use a named timezone as the default for unsafeMakeZoned #5676
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fe42df8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… system-derived timezone settings
|
Hmm, having dug into it more I think the root cause might be that That said, I think that manually supplying an offset-based time zone to the Cron methods will still produce the original bug. |
| const zoneName = Intl.DateTimeFormat().resolvedOptions().timeZone | ||
| const parsedZone = zoneFromString(zoneName) | ||
| if (Option.isNone(parsedZone)) { | ||
| throw new IllegalArgumentException(`Invalid time zone: ${zoneName}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe fallback to a offset zone here
Type
Description
Okay, so this is timezone-related, so it's kind of confusing. I think the gist is: the root cause of is that when you do
Cron.sequencewith implicit timezones, you get into a situation in which,Which is how you get odd consecutive cron executions around time zones.
This PR makes 'implicit' timezones explicit internally using
Intl.DateTimeFormat().resolvedOptions().timeZone. Is this good and the "right way to do it"? I'm not sure - this is pretty tricky stuff! But it does make crons consistent and a cron that should fire once a week does fire once a week reliably with this method.Related