Repository: kelektiv/node-cron
Pull request #711 (2023-10-02 16:22:38)

fix: fix lastDate() value for intervals > 25 days
## Description

Update the internal `lastExecution` variable when running `onTick`, not when checking if we should run `onTick` or set a new timeout.

## Related Issue

Fixes https://github.com/kelektiv/node-cron/issues/710.

## Motivation and Context

See related issue.

## How Has This Been Tested?

Added test case. See [the failing test workflow](https://github.com/kelektiv/node-cron/actions/runs/6382788543/job/17322106840?pr=711) before pushing the fix.

## Types of changes

- [x] 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:

- [x] My code follows the code style of this project.
- [ ] 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).



Patch:
diff --git a/README.md b/README.md
index b2f482c..76d3d84 100644
--- a/README.md
+++ b/README.md
@@ -118,7 +118,7 @@ Parameter Based
   - `constructor(cronTime, onTick, onComplete, start, timeZone, context, runOnInit, utcOffset, unrefTimeout)`
     - `cronTime` - [REQUIRED] - The time to fire off your job. This can be in the form of cron syntax or a JS [Date](https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date) object.
     - `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.
+    - `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. **Note for TS users**: This should either be an arrow function, or a regular function cast to `() => void` (bug with generic type inference).
     - `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 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`. **Note**: Cannot be used together with `utcOffset`.
     - `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.
diff --git a/src/job.ts b/src/job.ts
index b03b58a..ca6f6f1 100644
--- a/src/job.ts
+++ b/src/job.ts
@@ -11,7 +11,7 @@ import {
 	WithOnComplete
 } from './types/cron.types';
 
-export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
+export class CronJob<OC extends CronOnCompleteCommand | null = null, C = null> {
 	cronTime: CronTime;
 	running = false;
 	unrefTimeout = false;
@@ -19,7 +19,7 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 	runOnce = false;
 	context: CronContext<C>;
 	onComplete?: WithOnComplete<OC> extends true
-		? CronOnCompleteCallback<C>
+		? CronOnCompleteCallback
 		: undefined;
 
 	private _timeout?: NodeJS.Timeout;
@@ -81,9 +81,7 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 			// casting to the correct type since we just made sure that WithOnComplete<OC> = true
 			this.onComplete = this._fnWrap(
 				onComplete
-			) as WithOnComplete<OC> extends true
-				? CronOnCompleteCallback<C>
-				: undefined;
+			) as WithOnComplete<OC> extends true ? CronOnCompleteCallback : undefined;
 		}
 
 		if (this.cronTime.realDate) {
@@ -100,7 +98,7 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 		if (start) this.start();
 	}
 
-	static from<C = null, OC extends CronOnCompleteCommand<C> | null = null>(
+	static from<OC extends CronOnCompleteCommand | null = null, C = null>(
 		params: CronJobParams<OC, C>
 	) {
 		// runtime check for JS users
@@ -196,7 +194,7 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 			callback.call(
 				this.context,
 				this.onComplete as WithOnComplete<OC> extends true
-					? CronOnCompleteCallback<C>
+					? CronOnCompleteCallback
 					: never
 			);
 		}
@@ -243,7 +241,6 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 			// again. This processing might make us miss the deadline by a few ms
 			// times the number of sleep sessions. Given a MAXDELAY of almost a
 			// month, this should be no issue.
-			this.lastExecution = new Date();
 			if (remaining) {
 				if (remaining > MAXDELAY) {
 					remaining -= MAXDELAY;
@@ -256,6 +253,7 @@ export class CronJob<OC extends CronOnCompleteCommand<C> | null, C = null> {
 				setCronTimeout(timeout);
 			} else {
 				// We have arrived at the correct point in time.
+				this.lastExecution = new Date();
 
 				this.running = false;
 
diff --git a/src/time.ts b/src/time.ts
index a87d456..1a32b9f 100644
--- a/src/time.ts
+++ b/src/time.ts
@@ -37,19 +37,19 @@ export class CronTime {
 	private dayOfWeek: TimeUnitField<'dayOfWeek'> = {};
 
 	constructor(
-		source: CronJobParams<null>['cronTime'],
-		timeZone?: CronJobParams<null>['timeZone'],
+		source: CronJobParams['cronTime'],
+		timeZone?: CronJobParams['timeZone'],
 		utcOffset?: null
 	);
 	constructor(
-		source: CronJobParams<null>['cronTime'],
+		source: CronJobParams['cronTime'],
 		timeZone?: null,
-		utcOffset?: CronJobParams<null>['utcOffset']
+		utcOffset?: CronJobParams['utcOffset']
 	);
 	constructor(
-		source: CronJobParams<null>['cronTime'],
-		timeZone?: CronJobParams<null>['timeZone'],
-		utcOffset?: CronJobParams<null>['utcOffset']
+		source: CronJobParams['cronTime'],
+		timeZone?: CronJobParams['timeZone'],
+		utcOffset?: CronJobParams['utcOffset']
 	) {
 		// runtime check for JS users
 		if (timeZone != null && utcOffset != null) {
diff --git a/src/types/cron.types.ts b/src/types/cron.types.ts
index 67fe1a6..426dbad 100644
--- a/src/types/cron.types.ts
+++ b/src/types/cron.types.ts
@@ -5,7 +5,7 @@ import { CronJob } from '../job';
 import { IntRange } from './utils';
 
 interface BaseCronJobParams<
-	OC extends CronOnCompleteCommand<C> | null,
+	OC extends CronOnCompleteCommand | null = null,
 	C = null
 > {
 	cronTime: string | Date | DateTime;
@@ -18,7 +18,7 @@ interface BaseCronJobParams<
 }
 
 export type CronJobParams<
-	OC extends CronOnCompleteCommand<C> | null,
+	OC extends CronOnCompleteCommand | null = null,
 	C = null
 > =
 	| BaseCronJobParams<OC, C> &
@@ -33,16 +33,14 @@ export type CronJobParams<
 				  }
 			);
 
-export type CronContext<C> = C extends null ? CronJob<null> : NonNullable<C>;
+export type CronContext<C> = C extends null ? CronJob : NonNullable<C>;
 
 export type CronCallback<C, WithOnCompleteBool extends boolean = false> = (
 	this: CronContext<C>,
-	onComplete: WithOnCompleteBool extends true
-		? OmitThisParameter<CronOnCompleteCallback<C>>
-		: never
+	onComplete: WithOnCompleteBool extends true ? CronOnCompleteCallback : never
 ) => void;
 
-export type CronOnCompleteCallback<C> = (this: CronContext<C>) => void;
+export type CronOnCompleteCallback = () => void;
 
 export type CronSystemCommand =
 	| string
@@ -56,9 +54,7 @@ export type CronCommand<C, WithOnCompleteBool extends boolean = false> =
 	| CronCallback<C, WithOnCompleteBool>
 	| CronSystemCommand;
 
-export type CronOnCompleteCommand<C> =
-	| OmitThisParameter<CronOnCompleteCallback<C>>
-	| CronSystemCommand;
+export type CronOnCompleteCommand = CronOnCompleteCallback | CronSystemCommand;
 
 export type WithOnComplete<OC> = OC extends null ? false : true;
 


Test patch:
diff --git a/tests/cron.test.ts b/tests/cron.test.ts
index 6ae9516..0ef6a88 100644
--- a/tests/cron.test.ts
+++ b/tests/cron.test.ts
@@ -329,7 +329,7 @@ describe('cron', () => {
 						expect(t.getSeconds()).toBe(d.getSeconds());
 						onComplete();
 					},
-					function () {
+					() => {
 						callback();
 						resolve();
 					},
@@ -344,6 +344,35 @@ describe('cron', () => {
 			expect(callback).toHaveBeenCalledTimes(2);
 		});
 
+		it('should run on a specific date and call onComplete from onTick using the object constructor', async () => {
+			const d = new Date();
+			const clock = sinon.useFakeTimers(d.getTime());
+			d.setSeconds(d.getSeconds() + 1);
+			const callback = jest.fn();
+
+			await new Promise<void>(resolve => {
+				const job = CronJob.from({
+					cronTime: d,
+					onTick: onComplete => {
+						const t = new Date();
+						expect(t.getSeconds()).toBe(d.getSeconds());
+						onComplete();
+					},
+					onComplete: function () {
+						callback();
+						resolve();
+					} as () => void,
+					start: true
+				});
+				clock.tick(1000);
+				clock.restore();
+				job.stop();
+			});
+
+			// onComplete is called 2 times: once in onTick() & once when calling job.stop()
+			expect(callback).toHaveBeenCalledTimes(2);
+		});
+
 		it("should not be able to call onComplete from onTick if if wasn't provided", () => {
 			expect.assertions(4);
 			const d = new Date();
@@ -1108,7 +1137,7 @@ describe('cron', () => {
 		clock.restore();
 	});
 
-	it('should give the last execution date', () => {
+	it('should give the correct last execution date', () => {
 		const callback = jest.fn();
 		const clock = sinon.useFakeTimers();
 		const job = new CronJob('* * * * * *', callback);
@@ -1120,6 +1149,23 @@ describe('cron', () => {
 		clock.restore();
 	});
 
+	it('should give the correct last execution date for intervals greater than 25 days (#710)', () => {
+		const callback = jest.fn();
+		const clock = sinon.useFakeTimers();
+
+		const job = new CronJob('0 0 0 1 * *', callback); // At 00:00 on day-of-month 1.
+		job.start();
+
+		// tick one tick before nextDate()
+		clock.tick(job.nextDate().toMillis() - 1);
+
+		expect(callback).toHaveBeenCalledTimes(0);
+		expect(job.lastDate()?.getTime()).toBeUndefined();
+
+		job.stop();
+		clock.restore();
+	});
+
 	it('should throw when providing both exclusive parameters timeZone and utcOffset', () => {
 		expect(() => {
 			// @ts-expect-error testing runtime exception