Skip to content
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

Support pushing multiple results to success handler #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jessetane
Copy link
Owner

No description provided.

@jessetane
Copy link
Owner Author

@dcsim0n I was making a few small changes to the example and realized the success handler isn't supporting multiple results - I took a pass at fixing that and wondered if you'd be willing to have a look at what I came up with?

@jessetane
Copy link
Owner Author

of course these changes would require a major version bump, which is fine by me

@coveralls
Copy link

coveralls commented Jun 18, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling a772e5e on success-results into 691f70e on master.

@jessetane
Copy link
Owner Author

another thought: we could instead drop support for multiple results and change the api so it's up to the user if they want to return an array?

@dcsim0n
Copy link
Contributor

dcsim0n commented Jun 18, 2019

My first suggestion is something like this

callback(null,1,2,3,4) \passing multiple results

q.on('success', function(job, result1, result2){
//result1 === 1
//result2 === 2
//ignores other arguments that are passed
}
q.on('success',function(job,...results){
/// results === [1,2,3,4]

I understand this would break the current api by reversing the order that the job is passed, but it does make for an elegant implementation. The Rest operator syntax only exists for ES6 and up, not sure if that breaks anything for you.

@jessetane
Copy link
Owner Author

good point, that is a little cleaner. will add a patch to try it when i get a chance, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants