feat!: rework utcOffset parameter

sheerlox commented 1 year ago

Description

This PR aims to make the utcOffset parameter behavior more explicit.

Before these changes, strings were accepted and resulted in unpredictable behavior due to the comparison of string & number and the parseInt method being used. See https://github.com/kelektiv/node-cron/pull/685#issuecomment-1676394391 for more details.

utcOffset between -60 and +60 were treated as hours behind the scenes, which wasn't initially documented.

Changes introduced: utcOffset must now be a string, and will always be treated as minutes.

Motivation and Context

These changes follow the discussions on #685 about the poor design choices made while this parameter was implemented.

How Has This Been Tested?

Test suites.

Types of changes

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

Checklist:

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

Breaking changes

BREAKING CHANGE: utcOffset parameter no longer accepts a string BREAKING CHANGE: utcOffset values between -60 and 60 are no longer treated as hours BREAKING CHANGE: providing both timeZone and utcOffset parameters now throws an error

sheerlox commented 1 year ago

cc @rharshit82 for review/comments since you authored #685 :wink:

sheerlox commented 1 year ago

the parsing logic for utcOffset is still a little confusing but fixing it might be out of scope for this PR

@intcreator can you expand on that? I'm going to restrict the type so if I can do something while at it let me know!

intcreator commented 1 year ago

so that's a different issue than the type, it's just when parsing the utcOffset number into the sign, hours, and minutes it's a little tricky to understand what's going on. there's some if/else logic and some ternary logic and it's not super obvious what the final format should be. I would prefer if the sign, hours, and minutes were all parsed separately and then all combined into a template literal at the very end

sheerlox commented 1 year ago

@intcreator addressed your concerns in the last commit :wink:

intcreator commented 1 year ago

I love the refactored UTC string builder! very easy to tell what's going on now. I have a small nitpick which is just a naming thing and not a blocker to merging the PR

ncb000gt commented 1 year ago

:tada: This PR is included in version 3.0.0-beta.9 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

kelektiv / node-cron