harrisiirak / cron-parser

Node.js library for parsing crontab instructions
MIT License
1.33k stars 156 forks source link

Incomplete reset() when prev() was called before makes next() skip a day #338

Open njoyard opened 11 months ago

njoyard commented 11 months ago

I noticed that next() skips a day when prev() was called before, even if reset() has been called.

Here is a test case, tested with cron-parser@4.9.0. Local TZ is Europe/Paris (so +1 right now). Not sure if TZ is involved in the issue.

const cronParser = require('cron-parser')
const expr = cronParser.parseExpression('0 0 22 * * *')
const now = new Date() // 2023-11-21T21:04:39Z
expr.reset(now)
expr.next().toDate() // 2023-11-21T21:00:00Z - correct (that's T22:00:00+01:00 today)
expr.reset(now)
expr.prev().toDate() // 2023-11-20T21:00:00Z - correct (yesterday)
expr.reset(now)
expr.next().toDate() // 2023-11-22T21:00:00Z - incorrect (tomorrow)

My understanding is that the last return value should be the same as the first. My current workaround is to re-parse the expression every time.

harrisiirak commented 11 months ago

Hi @njoyard!

If I removed reset() calls (to be precise, the one after prev() call was messing things up), it seems to be working:

const parser = require('./');

// Initial `2023-11-21T21:04:39Z` will not be matched, so I had to move the date one hour before
const currentDate = new Date('2023-11-21T20:04:39Z');
const options = {
  currentDate,
  // You shouldn't need to set this; I just needed that to test out the timezone
  tz: 'Europe/Paris' 
};

const expr = parser.parseExpression('0 0 22 * * *', options);

console.log(expr.next().toDate()); // 2023-11-21T21:00:00.000Z
console.log(expr.prev().toDate()); // 2023-11-20T21:00:00.000Z
console.log(expr.next().toDate()); // 2023-11-21T21:00:00.000Z

The culprit seems to be reset() call itself that won't work properly after prev() call.

markNZed commented 11 months ago

That might be because reset is not passing a timezone into CronDate at https://github.com/harrisiirak/cron-parser/blob/fa94e7f67870db1a36f09a526129d986ae966f47/lib/expression.js#L804 Maybe test if it works without the timezone.

harrisiirak commented 11 months ago

That might be because reset is not passing a timezone into CronDate at

When I tested it, I also also briefly tested with passing the tz in reset method - didn't see any difference. Definitely requires a bit more digging.