Repository: kelektiv/node-cron
Pull request #685 (2023-08-12 13:31:09)

fix: added fractional offset support
<!--- Provide a general summary of your changes in the Title above (following the Conventional Commits standard) -->
<!-- More infos: https://www.conventionalcommits.org -->
<!-- Commit types: https://github.com/insurgent-lab/conventional-changelog-preset#commit-types-->

## Description
This is a fix for Issue - https://github.com/kelektiv/node-cron/issues/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 - https://github.com/kelektiv/node-cron/pull/685

## Related Issue
<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->
Fixes https://github.com/kelektiv/node-cron/issues/568.

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
It allows negative as well as positive offset with minute spport

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
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
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [x] 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).


github-actions[bot]: Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the [Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/) and it looks like your proposed title needs to be adjusted.

Details:

```
No release type found in pull request title "added fractional offset support". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit
```
<!-- Sticky Pull Request Commentpr-title-lint-error -->
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: > 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](https://github.com/sheerlox/node-cron/blob/main/lib/time.js#L155-L159) ..

the kind of hidden behaviors that I was talking about earlier, those can really be a real pain :facepalm: 
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
sheerlox: regarding the new test case `should run a job using cron syntax with number format with minute support`, it is failing on my local setup. My timezone is UTC+1, so `t.offset = 60` in the test. Here's a snapshot of the state in `sendAt` while the test is running:

![image](https://github.com/kelektiv/node-cron/assets/11234273/35e95b22-810f-4340-8dc7-ab975913fcab)

the resulting `utcZone` equals `UTC-20:00`, preventing the test success.

I think the changes in https://github.com/kelektiv/node-cron/pull/685/commits/d509e61b6e56a9e2c8fb9b2219ab951c1d8e28b2 need to be reverted. I'm going to work on that while I'm at it so we can push this through ASAP.
sheerlox: Found a pretty good solution so the test will run the same way regardless of the local timezone.
@rharshit82 @intcreator could you please review my change (https://github.com/kelektiv/node-cron/pull/685/commits/caf1cda67be366ba06219251363e2f205819ac02)?
sheerlox: I realized while reviewing the PR that we can specify a string offset in the `timezone` parameter (e.g. `UTC+5:30`), making string support on the `offset` parameter redundant, so I'll remove string support.
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.

Patch:
diff --git a/README.md b/README.md
index bd8e8cd..f5aa99f 100644
--- a/README.md
+++ b/README.md
@@ -94,10 +94,10 @@ Parameter Based
         - `onTick` - [REQUIRED] - The function to fire at the specified time. If an `onComplete` callback was provided, `onTick` will receive it as an argument. `onTick` may call `onComplete` when it has finished its work.
         - `onComplete` - [OPTIONAL] - A function that will fire when the job is stopped with `job.stop()`, and may also be called by `onTick` at the end of each run.
         - `start` - [OPTIONAL] - Specifies whether to start the job just before exiting the constructor. By default this is set to false. If left at default you will need to call `job.start()` in order to start the job (assuming `job` is the variable you set the cronjob to). This does not immediately fire your `onTick` function, it just gives you more control over the behavior of your jobs.
-        - `timeZone` - [OPTIONAL] - Specify the time zone for the execution. This will modify the actual time relative to your time zone. If the time zone is invalid, an error is thrown. By default (if this is omitted) the local time zone will be used. You can check all time zones available at [Moment Timezone Website](http://momentjs.com/timezone/). Probably don't use both `timeZone` and `utcOffset` together or weird things may happen.
+        - `timeZone` - [OPTIONAL] - Specify the time zone for the execution. This will modify the actual time relative to your time zone. If the time zone is invalid, an error is thrown. By default (if this is omitted) the local time zone will be used. You can check the various time zones format accepted in the [Luxon documentation](https://github.com/moment/luxon/blob/master/docs/zones.md#specifying-a-zone). Note: This parameter supports minutes offsets, e.g. `UTC+5:30`. **Warning**: Probably don't use both `timeZone` and `utcOffset` together or weird things may happen.
         - `context` - [OPTIONAL] - The context within which to execute the onTick method. This defaults to the cronjob itself allowing you to call `this.stop()`. However, if you change this you'll have access to the functions and values within your context object.
         - `runOnInit` - [OPTIONAL] - This will immediately fire your `onTick` function as soon as the requisite initialization has happened. This option is set to `false` by default for backwards compatibility.
-        - `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.
+        - `utcOffset` - [OPTIONAL] - This allows you to specify the offset of your time zone rather than using the `timeZone` param. This should be an integer representing the number of minutes offset (like `120` for +2 hours or `-90` for -1.5 hours). **Warning**: Minutes offsets < 60 and >-60 will be treated as an offset in hours. This means a minute offset of `30` means an offset of +30 hours. Use the `timeZone` param in this case. This behavior [is planned to be removed in V3](https://github.com/kelektiv/node-cron/pull/685#issuecomment-1676417917). **Warning**: Probably don't use both `timeZone` and `utcOffset` together or weird things may happen.
         - `unrefTimeout` - [OPTIONAL] - If you have code that keeps the event loop running and want to stop the node process when that finishes regardless of the state of your cronjob, you can do so making use of this parameter. This is off by default and cron will run as if it needs to control the event loop. For more information take a look at [timers#timers_timeout_unref](https://nodejs.org/api/timers.html#timers_timeout_unref)       from the NodeJS docs.
     - `start` - Runs your job.
     - `stop` - Stops your job.
diff --git a/lib/time.js b/lib/time.js
index 5c4a98a..b8af244 100644
--- a/lib/time.js
+++ b/lib/time.js
@@ -152,17 +152,26 @@ function CronTime(luxon) {
 			}
 
 			if (typeof this.utcOffset !== 'undefined') {
-				let offset =
+				const offsetHours = parseInt(
 					this.utcOffset >= 60 || this.utcOffset <= -60
 						? this.utcOffset / 60
-						: this.utcOffset;
-				offset = parseInt(offset);
+						: this.utcOffset
+				);
+
+				const offsetMins =
+					this.utcOffset >= 60 || this.utcOffset <= -60
+						? Math.abs(this.utcOffset - offsetHours * 60)
+						: 0;
+				const offsetMinsStr = offsetMins >= 10 ? offsetMins : '0' + offsetMins;
 
 				let utcZone = 'UTC';
-				if (offset < 0) {
-					utcZone += offset;
-				} else if (offset > 0) {
-					utcZone += `+${offset}`;
+
+				if (parseInt(this.utcOffset) < 0) {
+					utcZone += `${
+						offsetHours === 0 ? '-0' : offsetHours
+					}:${offsetMinsStr}`;
+				} else {
+					utcZone += `+${offsetHours}:${offsetMinsStr}`;
 				}
 
 				date = date.setZone(utcZone);


Test patch:
diff --git a/tests/cron.test.js b/tests/cron.test.js
index 4953e15..f2e7840 100644
--- a/tests/cron.test.js
+++ b/tests/cron.test.js
@@ -398,7 +398,7 @@ describe('cron', () => {
 	});
 
 	describe('with timezone', () => {
-		it('should run a job using cron syntax', () => {
+		it('should run a job using cron syntax with a timezone', () => {
 			const clock = sinon.useFakeTimers();
 			const callback = jest.fn();
 			const luxon = require('luxon');
@@ -436,6 +436,46 @@ describe('cron', () => {
 			expect(callback).toHaveBeenCalledTimes(1);
 		});
 
+		it('should run a job using cron syntax with a "UTC+HH:mm" offset as timezone', () => {
+			const clock = sinon.useFakeTimers();
+			const callback = jest.fn();
+			const luxon = require('luxon');
+
+			// Current time
+			const d = luxon.DateTime.local();
+
+			// Current time with zone offset
+			let zone = 'UTC+5:30';
+			let t = luxon.DateTime.local().setZone(zone);
+
+			// If current offset is UTC+5:30, switch to UTC+6:30..
+			if (t.hour === d.hour && t.minute === d.minute) {
+				zone = 'UTC+6:30';
+				t = t.setZone(zone);
+			}
+			expect(`${d.hour}:${d.minute}`).not.toBe(`${t.hour}:${t.minute}`);
+
+			// If t = 59s12m then t.setSeconds(60)
+			// becomes 00s13m so we're fine just doing
+			// this and no testRun callback.
+			t = t.plus({ seconds: 1 });
+			// Run a job designed to be executed at a given
+			// time in `zone`, making sure that it is a different
+			// hour than local time.
+			const job = new cron.CronJob(
+				t.second + ' ' + t.minute + ' ' + t.hour + ' * * *',
+				callback,
+				null,
+				true,
+				zone
+			);
+
+			clock.tick(1000);
+			clock.restore();
+			job.stop();
+			expect(callback).toHaveBeenCalledTimes(1);
+		});
+
 		it('should run a job using a date', () => {
 			const luxon = require('luxon');
 			let zone = 'America/Chicago';
@@ -828,6 +868,51 @@ describe('cron', () => {
 			expect(callback).toHaveBeenCalledTimes(1);
 		});
 
+		it('should run a job using cron syntax with numeric format utcOffset with minute support', () => {
+			const clock = sinon.useFakeTimers();
+			const callback = jest.fn();
+			const luxon = require('luxon');
+			// Current time
+			const t = luxon.DateTime.local();
+
+			/**
+			 * in order to avoid the minute offset being treated as hours (when `-60 < utcOffset < 60`) regardless of the local timezone,
+			 * and the maximum possible offset being +14:00, we simply add 80 minutes to that offset.
+			 * this implicit & undocumented behavior is planned to be removed in V3 anyway:
+			 * https://github.com/kelektiv/node-cron/pull/685#issuecomment-1676417917
+			 */
+			const minutesOffset = 14 * 60 + 80; // 920
+
+			// UTC Offset decreased by minutesOffset
+			const utcOffset = t.offset - minutesOffset;
+
+			const job = new cron.CronJob(
+				t.second + ' ' + t.minute + ' ' + t.hour + ' * * *',
+				callback,
+				null,
+				true,
+				null,
+				null,
+				null,
+				utcOffset
+			);
+
+			// tick 1 sec before minutesOffset
+			clock.tick(1000 * minutesOffset * 60 - 1);
+			expect(callback).toHaveBeenCalledTimes(0);
+
+			clock.tick(1);
+			clock.restore();
+			job.stop();
+			expect(callback).toHaveBeenCalledTimes(1);
+		});
+
+		/**
+		 * this still works implicitly (without minute support) because the string conversion
+		 * to integer removes everything after the colon, i.e. '(+/-)HH:mm' becomes (+/-)HH,
+		 * but this is an undocumented behavior that will be removed in V3:
+		 * https://github.com/kelektiv/node-cron/pull/685#issuecomment-1676394391
+		 */
 		it('should run a job using cron syntax with string format utcOffset', () => {
 			const clock = sinon.useFakeTimers();
 			const callback = jest.fn();
@@ -835,11 +920,12 @@ describe('cron', () => {
 			// Current time
 			const t = luxon.DateTime.local();
 			// UTC Offset decreased by an hour (string format '(+/-)HH:mm')
-			const utcOffset = t.offset - 60;
-			let utcOffsetString = utcOffset > 0 ? '+' : '-';
-			utcOffsetString += ('0' + Math.floor(Math.abs(utcOffset) / 60)).slice(-2);
-			utcOffsetString += ':';
-			utcOffsetString += ('0' + (utcOffset % 60)).slice(-2);
+			// We support only HH support in offset as we support string offset in Timezone.
+			const minutesOffset = t.offset - Math.floor((t.offset - 60) / 60) * 60;
+			const utcOffset = t.offset - minutesOffset;
+			const utcOffsetString = `${utcOffset > 0 ? '+' : '-'}${(
+				'0' + Math.floor(Math.abs(utcOffset) / 60)
+			).slice(-2)}:${('0' + (utcOffset % 60)).slice(-2)}`;
 
 			const job = new cron.CronJob(
 				t.second + ' ' + t.minute + ' ' + t.hour + ' * * *',
@@ -852,8 +938,8 @@ describe('cron', () => {
 				utcOffsetString
 			);
 
-			// tick 1 sec before an hour
-			clock.tick(1000 * 60 * 60 - 1);
+			// tick 1 sec before minutesOffset
+			clock.tick(1000 * 60 * minutesOffset - 1);
 			expect(callback).toHaveBeenCalledTimes(0);
 
 			// tick 1 sec
diff --git a/tests/crontime.test.js b/tests/crontime.test.js
index d6721bd..f09f696 100644
--- a/tests/crontime.test.js
+++ b/tests/crontime.test.js
@@ -648,18 +648,6 @@ describe('crontime', () => {
 		clock.restore();
 	});
 
-	it('should accept 4 as a valid UTC offset', () => {
-		const clock = sinon.useFakeTimers();
-
-		const cronTime = new cron.CronTime('0 11 * * *', null, 5);
-		const expected = luxon.DateTime.local().plus({ hours: 6 }).toSeconds();
-		const actual = cronTime.sendAt().toSeconds();
-
-		expect(actual).toEqual(expected);
-
-		clock.restore();
-	});
-
 	it('should detect real date in the past', () => {
 		const clock = sinon.useFakeTimers();