search

xrutayisire / react-js-cron

A React cron editor built with antd
https://xrutayisire.github.io/react-js-cron/?path=/docs/reactjs-cron--demo
MIT License
238 stars 105 forks source link

Select a day that doesn't exist for a month creates and invalid cron exp #11

Closed diogohertzberg-tc closed 2 years ago

diogohertzberg-tc commented 2 years ago

Describe the bug The user can select a date that does not exist. When the user selects Every "year" in "February", or any other month, the days dropdown are loaded always with 31 days. This leads to creating a cron that might never run.

Expected behavior If the user selects a month, only load the number of days that the month has. If February is chosen, only load 29. If March is chosen first, load 31 days but disable all months that have 30 days.

xrutayisire commented 2 years ago

Hello,

Thanks for the issue. I saw this problem during the development of the component and I decided not to handle it.

What you described as a bug is a valid Cron expression, and I tried to handle all valid Cron expressions, even if it doesn't make sense.

The issue is a bit more complicated than your solution. You're suggesting to disable months that don't have 31 days in some cases. But the user may want to select January, February, March, April and run a script on the 1, 2, 29, 30 and 31 day of the month. If the month doesn't have the day selected, it will simply not run, and that may be desired. What I want to show you is that in some tricky use cases, the user may want this.

So disabling some values for me is not an option.

One of the possibility to improve this is to handle it with error and / or warning messages. If the user selects only one month, and we know that the day selected doesn't exist, we could display an error. And if the user selects multiple months and for one or more months the day selected doesn't exist, we could display a warning.

BUT, because there is always a but, it's kind of tricky, and I think it's too much for this component that strictly follows Cron syntax. What I mean by that is, If a user copy-paste a VALID Cron expression in an Input linked to the Cron component as in the demo, it should still be VALID with this component.

I let you reply to this message if you can demonstrate that this will never block a user that want to create a valid (but tricky) Cron expression.

Have a good day 🙂

diogohertzberg-tc commented 2 years ago

Hey, thanks for the fast response. And yes, it's a tricky one that I've imagined you guys had to have come across before. And I agree that Invalid wasn't the proper word for that. Anyways, some warning message to the user would be nice to have, in case the user choses only one date that will never run. Thanks again!

xrutayisire commented 2 years ago

Since, a warning message displayed when there is only one month selected will add a new entry in the locale, it will be a breaking change. I will keep this in my personal improvement list, but If you really need this feature for your work, you can create a feature request issue describing what you want.

Have a good day :)