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

[BUG] 2-0 week expression error #654

Closed
Tracked by #674
CodFrm opened this issue Mar 8, 2023 · 6 comments · Fixed by #687
Closed
Tracked by #674

[BUG] 2-0 week expression error #654

CodFrm opened this issue Mar 8, 2023 · 6 comments · Fixed by #687
Labels
released type:bug Bug reports and bug fixes

Comments

@CodFrm
Copy link

CodFrm commented Mar 8, 2023

Description

If the current time is not Tuesday, cron.send will return the time of Tuesday of the next week

let cron = new CronTime("*/5 10-16 * * 2-0");
console.log(cron.sendAt().toFormat("yyyy-MM-dd HH:mm:ss"));

image

But the current time is Tuesday, the returned result is correct

The following takes the current time (Wednesday) as an example

let cron = new CronTime("*/5 10-16 * * 3-0");
console.log(cron.sendAt().toFormat("yyyy-MM-dd HH:mm:ss"));

image

Screenshots

No response

Additional information

No response

@CodFrm CodFrm changed the title 2-0 week expression error [BUG] 2-0 week expression error Mar 8, 2023
@redii
Copy link

redii commented May 11, 2023

Hey I think your crontab is just wrong, a day later you would get the same error for your 3-0 crontab. When you enter the crontab as followed everything works as you'd expected it.
Screenshot 2023-05-11 at 10 49 41

crontab.guru marks your 2-0 and 3-0 as an error and it probably shouldn't be allowed by node-cron?

@redii
Copy link

redii commented May 11, 2023

crontab.guru would allow the usage of 2-7 and 3-7 but thats seems not to be supported by node-cron

@nerdkapilgupta
Copy link

Issue Description:
When the current time is not Tuesday and a cron expression with a week range (e.g., "2-0") is used, the cron.sendAt() function returns the time of Tuesday in the next week instead of the expected behavior. However, if the current time is Tuesday, the returned result is correct.

Solution:
To address this issue, I have made modifications to the CronTime.prototype.sendAt function. The updated code now handles the specific scenario where the current day is included in the week range expression.

I have used the cron-parser library to parse the cron expression and adjust the day field if the current day is part of the week range. The code then calculates the next date(s) based on the adjusted expression and returns them as Luxon DateTime objects.

To implement this bug fix in your codebase, follow these steps:

Install the cron-parser library in your project.
Update the CronTime.prototype.sendAt function with the provided code.

Please note that you should include the necessary import statements for the required libraries.

Testing:
I have tested the modified code with different cron expressions and verified that it now returns the correct next date(s) even when the current day falls within the week range.

@sheerlox sheerlox added the type:bug Bug reports and bug fixes label Aug 15, 2023
@ncb000gt
Copy link
Member

🎉 This issue has been resolved in version 2.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sheerlox
Copy link
Collaborator

crontab.guru marks your 2-0 and 3-0 as an error and it probably shouldn't be allowed by node-cron?

indeed that expression wasn't supposed to be allowed. It was due to a bug that got fixed in v2.4.3!

crontab.guru would allow the usage of 2-7 and 3-7 but thats seems not to be supported by node-cron

while fixing the bug mentioned above, I checked that those expressions were correctly supported, and I can confirm they are (at least now, maybe it was different a few months ago)

@ncb000gt
Copy link
Member

🎉 This issue has been resolved in version 3.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type:bug Bug reports and bug fixes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants