-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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(cron) implement Deno.cron() #21019
Conversation
[[package]] | ||
name = "saffron" | ||
version = "0.1.0" | ||
source = "registry+https://github.com/rust-lang/crates.io-index" | ||
checksum = "03fb9a628596fc7590eb7edbf7b0613287be78df107f5f97b118aad59fb2eea9" | ||
dependencies = [ | ||
"chrono", | ||
"nom 5.1.3", | ||
] |
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.
Is saffron
maintained anymore? cloudflare/saffron#20
Anyways, we should ask saffron
to upgrade to nom
7.1
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.
I got confirmation that Cloudflare is happy to merge PRs, as it is still in use at CF.
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.
@littledivy can we do this after this PR is landed?
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.
Sure
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.
Working on an alternative to saffron that might be of interest - with second granularity and support for timezoned DateTimes. Also nicknames for @hourly
. Extended syntax like L for last occurrence of weekday in month. Standards-compliant weekday entry (1 is monday, in saffron 0 seem to be monday). Early stuff, started yesterday, and (disclaimer) my first Rust project ever, but starting to look like something - https://crates.io/crates/croner/0.0.3 https://github.com/hexagon/croner-rust
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.
@igorzi ready to run (or to be roasted 😉):
https://github.com/Hexagon/croner-rust/releases/tag/v1.0.0
https://github.com/Hexagon/croner-rust#why-croner-instead-of-cron-or-saffron
cli/tests/unit/cron_test.ts
Outdated
try { | ||
assertThrows( | ||
() => Deno.cron("abc", "*/20 * * * *", () => {}), | ||
"Invalid cron name", |
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 use a different error message, Invalid cron name
for a duplicate name is confusing - e.g. a dependency of dependency registers a cron and suddenly you get this error in your own code
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.
Oh, this test is wrong. 2nd arg to assertThrows
should be the exception type. I'll fix the tests and make sure that it's checking for the correct error message.
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.
fixed
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.
LGTM
This PR adds unstable
Deno.cron
API to trigger execution of cron jobs.options.backoffSchedule
.