Open sheerlox opened 1 year ago
mryellow
commented
1 year ago Given the state of #467 and the rate at which it keeps me entertained with report after report, I think this whole thing needs to be abandon. There are other options and they work.
p.s. TypeScript won't solve anything.
sheerlox
commented
1 year ago @mryellow if you want to move to another library, please feel free to do so :) If, on the other hand, you'd like to start being constructive and helpful, then useful comments and PRs are welcome.
mryellow
commented
1 year ago Have you considered removing the calculations for "next scheduled" and using a library instead?
cron-parser will handle the while loop in a way which doesn't fail under the same conditions.
intcreator
commented
1 year ago we're looking at a rewrite of that function yes because this has been an issue for a while. if you have any ideas for a specific approach we're open to it. I kind of would prefer not to use a library if possible because it's better if this utility stays as lightweight as it can although since we're already using Luxon maybe there's something there we can use
mryellow
commented
1 year ago if you have any ideas
That's the thing, at a glance the structure of _getNextDateFrom looks fairly similar to approaches used elsewhere. Any re-write would probably just involve taking a very close look at those functions elsewhere and incorporation whatever differences there are. At which point you may as well just use a proven library rather than copy/pasting it's code.
Roadmap
Based on the bellow observations (see Context), here's what I think would be a good way going forward:
V2 features and bugfixes
V3 release (breaking changes)
betachannel)betachannel)post V3 release (ordered by priority)
Misc/Rehabilitation
Candidates for next major release
C&OCgeneric type parameters on every type (QoL)runOnceas aCronJoboptionLess urgent
Context
Click to expand
## Motivation In my opinion, the OpenSource spirit is more about trying to maintain/improve existing projects (especially when they are "industry leaders") than trying to create a new project overseeding the existing ones. --- On February 12, [this issue](https://github.com/nestjs/schedule/issues/1164 ) was created to replace `cron` with `croner` (by the creator of the later) in the `@nestjs/schedule` package. Work is being done by the same individual in [this PR](https://github.com/nestjs/schedule/pull/1165) to meet that goal. In [this `@nestjs/nest` issue](https://github.com/nestjs/nest/issues/9654) (one year ago), OP mentioned [this issue from our repository](https://github.com/kelektiv/node-cron/issues/232), that is closed but according to some comments, it might still be occurring (see issue https://github.com/kelektiv/node-cron/issues/467 for follow-up on this), although we cannot be sure users are using the latest version. We should implement test cases that somehow limit/overload cpi & memory resources to test for this kind of problem (this has been successfully implemented, see https://github.com/kelektiv/node-cron/issues/467#issuecomment-1580693046). Here are the arguments to migrate to `croner` according to its creator (from [the `@nestjs/schedule` PR](https://github.com/nestjs/schedule/pull/1165)): > supports both ESM and CJS. - migrating to Typescript will enable supporting both ESM and CJS > is ready for the future, supporting Node, Deno, Bun and Browser environments. - I never tried to use this library in the browser, not sure what are the use cases but I guess we can make sure it is portable. This should be taken care of during the Typescript migration as well. - Regarding Deno & Bun, I have no experience with those but we would need to look further into that (looks like Deno only supports ESM and that it's the only blocking point. Bun should already be working according to their docs) - Browser-support related PR: https://github.com/kelektiv/node-cron/pull/629 > has more predictive behaviour and less bugs. (not affected by Scheduled jobs execute multiple times #454 as an example) - this issue (https://github.com/nestjs/schedule/issues/454) has been resolved for two years (not explicitly but if it was still the case we would have heard about it), but there might be a good point regarding predictability and bugs. > uses the same pattern as vixie cron (see EVERY_YEAR Cronexpression is invalid #1159) - we could add the support for `L` for the last day and weekday of the month in cron strings? - there is an issue opened about this: https://github.com/kelektiv/node-cron/issues/435 > has no dependencies (see [Vulnerability found in dependency #1147](https://github.com/nestjs/schedule/issues/1147)) - I personally don't think having `luxon` as a dependency is an issue, it's even a feature since it allows us to have support for the `DateTime` object. That is debatable though, and I'm welcoming any objecting point of view so we can find the solution that best fits everyone's use cases. - We definitely need to add some dependencies scanning & automated version bumps PR tool (Renovate/Dependabot), as is standard in the Javascript OpenSource ecosystem. As another useful reference, here's [a link to the features comparison](https://github.com/Hexagon/croner#why-another-javascript-cron-implementation) from `croner`. Two features I think are particularly interesting: - `Over-run protection`: an option to prevent the next occurrence from running if the first one is still processing (long-running jobs) - `Error handling`: this is something I've seen multiple times in our issues' comments, if something goes wrong it seems to go unnoticed most of the time, maybe we should think about a better way of detecting and reporting potential errors to the user *(EDIT: error handling in `croner` is actually providing a way to handle errors that might occur in the `tick` function, see [Hexagon/croner/EXAMPLES.md#error-handling](https://github.com/Hexagon/croner/blob/master/docs/EXAMPLES.md#error-handling))* ---This issue was initially discussed on Discord.