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.

Sign up for GitHub

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

Jump to bottom

feat!: rework utcOffset parameter #699

Merged
merged 5 commits into from
Sep 29, 2023
Merged

feat!: rework utcOffset parameter #699

merged 5 commits into from
Sep 29, 2023

Conversation

sheerlox
Copy link
Collaborator

@sheerlox sheerlox commented Sep 27, 2023 •
edited

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 #685 (comment) 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)
  • 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).

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
feat: rework utcOffset parameter
b4dc82c 
BREAKING CHANGE: `utcOffset` parameter no longer accepts a string
BREAKING CHANGE: `utcOffset` values between -60 and 60 are no longer treated as hours
@sheerlox sheerlox added the type:feature New feature or feature improvement & requests label Sep 27, 2023
@sheerlox sheerlox self-assigned this Sep 27, 2023
@sheerlox sheerlox mentioned this pull request Sep 27, 2023
Open
34 tasks
@sheerlox
Copy link
Collaborator Author

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

@sheerlox sheerlox mentioned this pull request Sep 27, 2023
Merged
rharshit82
rharshit82 reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@rharshit82 rharshit82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. In future, we can break the sendAt function into several smaller methods to improve clarity and maintainability but that is out of scope for TS migration for now.

intcreator
intcreator approved these changes Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@intcreator intcreator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated Show resolved Hide resolved
@sheerlox
Copy link
Collaborator Author

sheerlox commented Sep 28, 2023 •
edited

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
Copy link
Collaborator

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
feat!: make timeZone and utcOffset parameters mutually exclusive
55d66fa 
BREAKING CHANGE: using both timeZone and utcOffset now throws an error
sheerlox
sheerlox commented Sep 28, 2023
View reviewed changes
src/job.ts Show resolved Hide resolved
@sheerlox
Copy link
Collaborator Author

@intcreator addressed your concerns in the last commit 😉

intcreator
intcreator reviewed Sep 29, 2023
View reviewed changes
src/time.ts Outdated Show resolved Hide resolved
@intcreator
Copy link
Collaborator

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

@sheerlox sheerlox merged commit 72d3d36 into kelektiv:beta Sep 29, 2023
11 checks passed
@sheerlox sheerlox deleted the feat/utc-offset-rework branch September 29, 2023 22:52
ncb000gt pushed a commit that referenced this pull request Sep 29, 2023
@semantic-release-bot
Release v3.0.0-beta.9 [skip ci]
ab210d7 
## [3.0.0-beta.9](v3.0.0-beta.8...v3.0.0-beta.9) (2023-09-29)

### ⚠ Breaking changes

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

### ✨ Features

* rework utcOffset parameter ([#699](#699)) ([72d3d36](72d3d36))

### ♻️ Chores

* improve GitHub community standards ([#698](#698)) ([6bdef77](6bdef77))

### 💎 Styles

* fix linting issues ([b48c1b2](b48c1b2))
@ncb000gt
Copy link
Member

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

The release is available on:

Your semantic-release bot 📦🚀

sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
feat!: rework utcOffset parameter (kelektiv#699)
a6f976a 
## 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 added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
feat!: rework utcOffset parameter (kelektiv#699)
78c3136 
## 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 added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
feat!: rework utcOffset parameter (kelektiv#699)
72b1baa 
## 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 added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
feat!: rework utcOffset parameter (kelektiv#699)
96c6f46 
## 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 added a commit that referenced this pull request Sep 30, 2023
@sheerlox
feat!: rework utcOffset parameter (#699)
671e933 
## 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
ncb000gt pushed a commit that referenced this pull request Sep 30, 2023
@semantic-release-bot
Release v3.0.0 [skip ci]
a7dee32 
## [3.0.0](v2.4.4...v3.0.0) (2023-09-30)

### ⚠ Breaking changes

* `utcOffset` parameter no longer accepts a string
* `utcOffset` values between -60 and 60 are no longer
treated as hours
* providing both `timeZone` and `utcOffset` parameters
now throws an error
* removed `cron.job()` method in favor of `new CronJob(...args)` /
`CronJob.from(argsObject)`
* removed `cron.time()` method in favor of `new CronTime()`
* `CronJob`: constructor no longer accepts an object as its first and
only params. Use `CronJob.from(argsObject)` instead.
* `CronJob`: callbacks are now called in the order they were registered
* return empty array from nextDates when called without argument (#519)
* UNIX standard alignments (#667)

### ✨ Features

* expose useful types ([737b344](737b344))
* rework utcOffset parameter ([#699](#699)) ([671e933](671e933))
* UNIX standard alignments ([#667](#667)) ([ff615f1](ff615f1))

### 🐛 Bug Fixes

* return empty array from nextDates when called without argument ([#519](#519)) ([c2891ba](c2891ba))

### 📦 Code Refactoring

* migrate to TypeScript ([#694](#694)) ([2d77894](2d77894))

### 📚 Documentation

* **readme:** remove outdated informations ([#695](#695)) ([b5ceaf1](b5ceaf1))

### 🚨 Tests

* update new test for cron standard alignments ([4a406c1](4a406c1))

### ♻️ Chores

* improve GitHub community standards ([#698](#698)) ([6bdef77](6bdef77))
* update contributors list ([dab3d69](dab3d69))

### 💎 Styles

* fix linting issues ([47e665f](47e665f))
@sheerlox sheerlox removed their assignment Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intcreator intcreator intcreator approved these changes

@rharshit82 rharshit82 rharshit82 approved these changes

Assignees
No one assigned
Labels
released type:feature New feature or feature improvement & requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sheerlox @intcreator @ncb000gt @rharshit82