fix: added fractional offset support

[rharshit82]

Description

This is a fix for Issue - #568
There was a stale pull request in the past but since it was not active, made a fresh PR with support for negative as well as positive offset with minute support - #685

Related Issue

Fixes #568.

Motivation and Context

It allows negative as well as positive offset with minute spport

How Has This Been Tested?

Tested in node 16.16.0, ran all the jet test cases and added 2 test cases where it was needed

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• If my change introduces a breaking change, I have added a ! after the type/scope in the title (see the Conventional Commits standard).

[sheerlox]

I've updated the PR title since this PR will introduce a breaking change: previously strings were parsed as minutes, now they would be parsed as "HH:mm" strings.

[rharshit82]

Also, wanted to highlight here that previously also string behaviour worked fine, just minute support was not there.
Offset : "5:30" translates to "UTC+5"
Offset: "330" also translates to "UTC+5"

[sheerlox]

left a few comments requesting some changes, otherwise lgtm 👍

[sheerlox]

Also, wanted to highlight here that previously also string behaviour worked fine, just minute support was not there. Offset : "5:30" translates to "UTC+5" Offset: "330" also translates to "UTC+5"

oh my .. you're absolutely right, "magic" code right here ..

the kind of hidden behaviors that I was talking about earlier, those can really be a real pain 🤦

especially since the doc clearly states:

utcOffset - [OPTIONAL] - This allows you to specify the offset of your time zone rather than using the timeZone param. This should be an integer amount representing the number of minutes offset (like 120 for +2 hours or -90 for -1.5 hours) Probably don't use both timeZone and utcOffset together or weird things may happen."

so one might be tempted to use "30" for 30 minutes, then get an offset of 30 hours 🤣

[rharshit82]

Have done the changes and tested it. Let me know if documentation changes are required for string support.

One thing to note is if we are adding string support, there will be cases where someone might put '330' as string offset where our code will take 330 hours as offset. I am thinking of handling this by using string as minutes in cases where : is not provided. What do you think?

[sheerlox]

Have done the changes and tested it. Let me know if documentation changes are required for string support.

Thanks for your changes, and yes, please update the documentation to reflect your changes.

One thing to note is if we are adding string support, there will be cases where someone might put '330' as string offset where our code will take 330 hours as offset. I am thinking of handling this by using string as minutes in cases where : is not provided. What do you think?

I think the best for now is to avoid introducing a breaking change in this PR, so we can release it on the v2 while we work on v3. I'll convert the codebase to Typescript soon, and I will be handling that issue at that time, which will then be released on v3 :)

[sheerlox]

Well actually there's a breaking change since you removed the "magic" code I referenced earlier (treating utcOffset as hours if it's <-60 || >60 ..

What do you think about reintroducing that behaviour for now, so this PR contains no breaking change, then start working on another PR targeting the beta branch to start redesigning that feature?

[sheerlox]

I ran the test pipeline for this PR, and there's some codestyle compliance issues.

You can run npm run lint -- --fix locally to autofix some of the issues, but there's (at least) one that'll still need to be fixed manually (your console will tell you which one after running the command).

[rharshit82]

Thank you for reviewing the PR in such a short time. I will be doing all the changes asked above.

[intcreator]

I took a look as well. good conversation and this PR is moving in a good direction. I just made one small suggestion

[rharshit82]

We should definitely work on changing the dependence on local time zones for test cases in the future as it is very easy to miss edge cases. I checked your changes, Since only HH is supported in the string utcOffset, the test case does not support +xx:yy where yy is >0 and < 60. My timezone is 5:30 so the test case is not passing on my machine.
Have solved this in this Pr - https://github.com/rharshit82/node-cron/pull/1/files. Please review this and approve it so that I can merge it otherwise you can also make this change in this PR.

[sheerlox]

good catch, I didn't put much effort into that test since it's going to be removed soon. I've reviewed and tested your changes, I'll merge your fork PR now.

[ncb000gt]

🎉 This PR is included in version 2.4.4 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[ncb000gt]

🎉 This PR is included in version 3.0.0-beta.5 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀