-
Notifications
You must be signed in to change notification settings - Fork 28
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
Feature/0002 more typings #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks :)
src/types/JobParameters.ts
Outdated
@@ -6,13 +6,12 @@ export interface IJobParameters<DATA = unknown | void> { | |||
|
|||
name: string; | |||
priority: number; | |||
nextRunAt: Date | null; | |||
nextRunAt?: Date | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now | null
can be reverted again right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never $unset nextRunAt and there are multiple places where nextRunAt is set explicitly to null
The reason to make nextRunAt optional was the delete of nextRunAt in line 297 in JobDbRepository.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right nextRunAt: Date | null;
there is .toJson() used (which is kinda weird anyway), but the reason behind this is that it returned a partial result before, you have changed the result of it to a non partial return value, what was the motivation behind that? Could we just let it return a partial result? Otherwise we could also just change line 252 to:
from ...job.toJson()
to ...(job.toJson() as Partial<IJobParameters>),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted that ? making nextRunAt mandatory in typings. I changed in line 297 of JobDbRepository.ts the typing when deleting that field. This makes imho the most sense.
fix: ensure type declarations are in build
second try