-
Notifications
You must be signed in to change notification settings - Fork 440
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
Fix/avoid hazards when upserting job schedulers #2892
Fix/avoid hazards when upserting job schedulers #2892
Conversation
9d086ee
to
937b8f8
Compare
…ps://github.com/taskforcesh/bullmq into fix/avoid-hazards-when-upserting-job-schedulers
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, added some non-blocker comments
@@ -268,3 +295,13 @@ export const defaultRepeatStrategy = ( | |||
// Ignore error | |||
} | |||
}; | |||
|
|||
function removeUndefinedFields(obj: Record<string, any>) { |
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.
This is a duplicated method. It's added in utils but it was not removed from here
@@ -464,11 +465,11 @@ export class Job< | |||
* @returns | |||
*/ | |||
asJSON(): JobJson { | |||
return { | |||
return removeUndefinedFields<JobJson>({ |
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.
ok I get the intention, what if instead of iteration yo all these option at the end, do something like
const jobJson = {
id: this id,
// all the other attributes that don't be unfefined
};
if(this parent) {
jobJson.parent = this.parent;
}
// same idea with the other atrributes
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.
Because it is easier to forget and it will be more code to maintain. I discovered that we have a lot of empty fields which just consume space for no reason before I added this feature.
Fixes #2876