kelektiv / node-cron

Cron for NodeJS.
MIT License
8.31k stars 617 forks source link

fix: set runOnce when actual date is given to setTime #740

Closed sheerlox closed 8 months ago

sheerlox commented 8 months ago

Description

initially, set the internal variable runOnce correctly when passing an actual date to setTime. previously this was only done in the constructor.

since it is only used in one place and as to limit this kind of error, I then removed the runOnce property completely (we now check directly if this.cronTime is a real date). we do not consider this a breaking change since it was not referenced in the API documentation).

Related Issue

Fixes #739.

Motivation and Context

Fixes an issue when after creating a job with a cron expression and using setTime with an actual date, the job attempts to run multiple times.

How Has This Been Tested?

added test case provided by #739's OP.

Screenshots (if appropriate):

Types of changes

Checklist:

ncb000gt commented 8 months ago

:tada: This PR is included in version 3.1.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

mirogajdos commented 8 months ago

Hello. Using Cron together with with @nestjs/schedule and now my application won't build. Removal of property in PATCH release will ruin typechecks and builds for many people. It's definitely breaking change even if you didn't mention it in the docs as it was part of the type definitions. Now Job class is not backward compatible with previous versions. 😿

sheerlox commented 8 months ago

thanks for the heads up @extrememito, and sorry about the issue we caused in your build. not releasing these kinds of changes as breaking changes was probably okay for JS, but you're right that TS types actually are documentation on their own, and we should not break any public interfaces in patch or minor releases. we'll be more cautious about this in the future.

751 is going to fix the issue once it's merged and released in 3.1.5.

as a side note, I'm curious what your setup looks like, because @nestjs/schedule uses pinned dependencies and didn't upgrade to 3.1.4 yet. would you mind expanding a bit on this?

mirogajdos commented 8 months ago

Thank you!! Sure. I'm running NX monorepo where some apps are using node-cron by itself and others via @nestjs/schedule. I'm using cron as described in this exmaple which requires importing CronJob class from node-cron and adding an instance into the registry. Therefore I have node-cron explicitly installed. I have also renovate bot setup to update Patch versions which bumped node-cron to 3.1.4 which resulted in:

TS2345: Argument of type 'import("/apps/node_modules/cron/dist/job").CronJob<null, null>' is not assignable to parameter of type 'import("/apps/node_modules/@nestjs/schedule/node_modules/cron/dist/job").CronJob<null, null>'.
  Property 'runOnce' is missing in type 'import("/apps/node_modules/cron/dist/job").CronJob<null, null>' but required in type 'import("/apps/node_modules/@nestjs/schedule/node_modules/cron/dist/job").CronJob<null, null>'.
    191 |       }
    192 |     })
  > 193 |     this.schedulerRegistry.addCronJob(expenseBlueprint.id, job)
sheerlox commented 8 months ago

@extrememito 3.1.5 is out, effectively restoring the runOnce property on CronJob! sorry again for the trouble.