refactor runtime check & typings of exclusive parameters timeZone & utcOffset

[gomez-git]

Does it? First condition precludes the second

node-cron/src/job.ts

Line 56 in a7dee32

if (timeZone != null && utcOffset != null) {

node-cron/src/job.ts

Line 57 in a7dee32

throw new ExclusiveParametersError('timeZone', 'utcOffset');

node-cron/src/job.ts

Line 65 in a7dee32

this.cronTime = new CronTime(cronTime, timeZone, utcOffset);

[sheerlox]

hey @gomez-git 👋 thank you for your suggestion!

I liked the solution you first suggested:

if (timeZone != null && utcOffset != null) {
	throw new ExclusiveParametersError('timeZone', 'utcOffset');
} else if (timeZone != null) {
	this.cronTime = new CronTime(cronTime, timeZone, null);
} else {
	this.cronTime = new CronTime(cronTime, null, utcOffset);
}

but then I thought (again), why not do:

if (timeZone != null && utcOffset != null) {
	throw new ExclusiveParametersError('timeZone', 'utcOffset');
} else {
	this.cronTime = new CronTime(cronTime, null, utcOffset);
}

the reason we can't is that even though TypeScript knows timeZone and utcOffset are mutually exclusive in the CronJobParams interface, it seems impossible to implement a similar behavior outside of interfaces (like two variables or function arguments).

so I'm thinking of a helper function, which would take timeZone and utcOffset as parameters, throw the error if both are defined and run the if/else if/else logic you suggested to return an object {timeZone, utcOffset} were they are mutually exclusive (like in CronJobParams).

this way we centralize the timeZone != null && utcOffset != null and error throwing into a single place, and that would theoretically allow us to refactor this horror I made into a single new CronJob call 🎉

would you be interested in submitting a PR for this change? this repository is going to be part of Hacktoberbest, so if you're planning to participate we can register that issue/PR as a participation 😉

[rharshit82]

picking this up

[Victor1890]

Hi @gomez-git and @sheerlox, I have seen that issue and this is my proposal to refactor code using your suggestion

PR: #841