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.

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

feat!: rework utcOffset parameter #699

Merged
merged 5 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ Parameter Based
- `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 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.
- `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**: Probably don't use both `timeZone` and `utcOffset` together or weird things may happen.
sheerlox marked this conversation as resolved.
Show resolved Hide resolved
- `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.
- `from` (static) - Create a new CronJob object providing arguments as an object. See argument names and descriptions above.
- `start` - Runs your job.
Expand Down
32 changes: 10 additions & 22 deletions src/time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
TIME_UNITS_MAP
} from './constants';
import {
CronJobParams,
DayOfMonthRange,
MonthRange,
Ranges,
Expand All @@ -24,7 +25,7 @@ import { getRecordKeys } from './utils';
export class CronTime {
source: string | DateTime;
zone?: string;
utcOffset?: number | string;
utcOffset?: number;
realDate = false;

private second: TimeUnitField<'second'> = {};
Expand All @@ -35,9 +36,9 @@ export class CronTime {
private dayOfWeek: TimeUnitField<'dayOfWeek'> = {};

constructor(
source: string | Date | DateTime,
zone?: string | null,
utcOffset?: string | number | null
source: CronJobParams['cronTime'],
zone?: CronJobParams['timeZone'],
utcOffset?: CronJobParams['utcOffset']
) {
if (zone) {
const dt = DateTime.fromObject({}, { zone });
Expand Down Expand Up @@ -126,27 +127,14 @@ export class CronTime {
date = date.setZone(this.zone);
}

if (this.utcOffset != null) {
const offsetHours = parseInt(
// @ts-expect-error old undocumented behavior going to be removed in V3
this.utcOffset >= 60 || this.utcOffset <= -60
? // @ts-expect-error old undocumented behavior going to be removed in V3
this.utcOffset / 60
: this.utcOffset
);
if (this.utcOffset !== undefined) {
const offsetHours = Math.trunc(this.utcOffset / 60);

const offsetMins =
// @ts-expect-error old undocumented behavior going to be removed in V3
this.utcOffset >= 60 || this.utcOffset <= -60
? // @ts-expect-error old undocumented behavior going to be removed in V3
Math.abs(this.utcOffset - offsetHours * 60)
: 0;
const offsetMinsStr = offsetMins >= 10 ? offsetMins : `0${offsetMins}`;
const offsetMins = Math.abs(this.utcOffset - offsetHours * 60);
const offsetMinsStr = `${offsetMins < 10 ? '0' : ''}${offsetMins}`;

let utcZone = 'UTC';

// @ts-expect-error old undocumented behavior going to be removed in V3
if (parseInt(this.utcOffset) < 0) {
if (this.utcOffset < 0) {
utcZone += `${offsetHours === 0 ? '-0' : offsetHours}:${offsetMinsStr}`;
} else {
utcZone += `+${offsetHours}:${offsetMinsStr}`;
Expand Down
2 changes: 1 addition & 1 deletion src/types/cron.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export interface CronJobParams {
timeZone?: string | null;
context?: unknown | null;
runOnInit?: boolean | null;
utcOffset?: string | number | null;
utcOffset?: number | null;
unrefTimeout?: boolean | null;
}

Expand Down
58 changes: 4 additions & 54 deletions tests/cron.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -870,17 +870,8 @@ describe('cron', () => {
// Current time
const t = 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;

// UTC Offset decreased by 45 minutes
const utcOffset = t.offset - 45;
const job = new CronJob(
`${t.second} ${t.minute} ${t.hour} * * *`,
callback,
Expand All @@ -892,51 +883,10 @@ describe('cron', () => {
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();
// Current time
const t = DateTime.local();
// UTC Offset decreased by an hour (string format '(+/-)HH:mm')
// 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 CronJob(
`${t.second} ${t.minute} ${t.hour} * * *`,
callback,
null,
true,
null,
null,
null,
utcOffsetString
);

// tick 1 sec before minutesOffset
clock.tick(1000 * 60 * minutesOffset - 1);
// tick 1 sec before 45 minutes
clock.tick(1000 * 45 * 60 - 1);
expect(callback).toHaveBeenCalledTimes(0);

// tick 1 sec
clock.tick(1);
clock.restore();
job.stop();
Expand Down
12 changes: 0 additions & 12 deletions tests/crontime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -746,18 +746,6 @@ describe('crontime', () => {
clock.restore();
});

it('should accept 4 as a valid UTC offset', () => {
const clock = sinon.useFakeTimers();

const cronTime = new CronTime('0 11 * * *', null, 5);
const expected = 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();

Expand Down