Feature/pushb

[ankitAtVauld]

Bulk Push feature as supported by latest version of faktory.
https://github.com/contribsys/faktory/blob/main/Changes.md#160

• Added test case
• Minimal code ( I am new to TypeScript, so did my best to add this )
• The decision to assert for JID in the payload is to make sure that the caller knows about the Job IDs its submitting rather than abstracted away by current lib. This will help the caller to check easily which jobs got failed based on the response.

[ankitAtVauld]

@jbielick Hey Josh,
Could you please review the PR and provide your valuable comments. It would be important for us to have this feature. appreciate your time.

Thanks
Ankit Khandelwal

[jbielick]

Hi @ankitAtVauld . Thanks for the work and following the new server features.

At a high level, I think I would prefer to see this library generate JIDs for the payloads. That would align pretty well with the rest of the library and the push_bulk functionality in Sidekiq. Is there some reasoning for avoiding creating JIDs that I'm not seeing?

Additionally, how would you feel about naming the client method pushBulk to match the order of the command PUSHB (push-bulk).

Do you think the Job class or a new class could help here at all? That is, would a nicer developer experience be an API where you can successively add jobs to a local list and then push the whole list? JIDs would be automatically created and perhaps we can model the server response somehow to make it easier to identify which jobs (and their args) that failed? I think the JID mapping from client to server can be handled within the library, what's important to the developer is probably what jobtype and job args failed to submit.

I'd be happy to add on some of these changes, but thought I would get your feedback first. If you'd like me to take this a bit further before releasing I can merge as-is and then add some of the behavior or changes I'm describing and then release after adding tests.

[jbielick]

We should be able to use encode instead of making a new function. The type signature for encode would need to be updated to something like Record<string, unknown> | Array I think.

[ankitAtVauld]

Oh if that works, it would be great. As both of these do exactly same thing. I just figured below works as you mentioned:

export function encode(
  object: Record<string, unknown> | Array<Record<string, unknown>>
): string {
  return JSON.stringify(object);
}

Lack of experience on Typescript :)

[ankitAtVauld]

Appreciate your response Josh. Below are my comments on your suggestions.

At a high level, I think I would prefer to see this library generate JIDs for the payloads. That would align pretty well with the rest of the library and the push_bulk functionality in Sidekiq. Is there some reasoning for avoiding creating JIDs that I'm not seeing?

-- My thought process was to let the submitter match the submitted JIDs with failed JIDs in response. Never used sidekiq, so I would go with your way to align it pretty well with existing framework.

Additionally, how would you feel about naming the client method pushBulk to match the order of the command PUSHB (push-bulk).

-- I agree.

Do you think the Job class or a new class could help here at all? That is, would a nicer developer experience be an API where you can successively add jobs to a local list and then push the whole list? JIDs would be automatically created and perhaps we can model the server response somehow to make it easier to identify which jobs (and their args) that failed? I think the JID mapping from client to server can be handled within the library, what's important to the developer is probably what jobtype and job args failed to submit.

-- I think that would be the way to do it if we would like to let library handle the JIDs.

I'd be happy to add on some of these changes, but thought I would get your feedback first. If you'd like me to take this a bit further before releasing I can merge as-is and then add some of the behavior or changes I'm describing and then release after adding tests.

-- Would be great to see the changes the way you mentioned. Let me know if I can be of any help.

[jbielick]

@ankitAtVauld Great. Thanks for the feedback. I'm going to merge this and take it a bit further. I'll post here when I've got something to show.

[ankitAtVauld]

@jbielick Thanks Josh. Would you like to release it with a newer version soon, so that we can upgrade our dependency to use it.

[jbielick]

@ankitAtVauld Could you try out v4.3.0? It's published now with pushBulk. The major change is that the response from that method is an object like this:

{
  '123': {
      reason: "Reason from server",
      payload: {
        jid: '123',
        jobtype: 'XyzJob',
        args: [...]
      }
  }
}

[ankitAtVauld]

@jbielick Appreciate your valuable time. Thanks a ton. Yeah this works well. I already integrated with our code and will soon test out this.
Also, just to get your comments, could you please also look at below issue/request when you have time. Your comments would be great!
timeout param