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

wrapCallback and callbacks with more than two arguments #246

Closed
d4h0 opened this issue Mar 16, 2015 · 9 comments
Closed

wrapCallback and callbacks with more than two arguments #246

d4h0 opened this issue Mar 16, 2015 · 9 comments
Assignees

Comments

@d4h0
Copy link

d4h0 commented Mar 16, 2015

Hi,

Why does wrapCallback ignore the third argument if there is one?

Wouldn't it be easy to check if there are more than two arguments and if so to put an array of these arguments into the stream?

Best regards,
Daniel

@apaleslimghost
Copy link
Collaborator

I agree that it would be good to support callbacks with multiple parameters, and it's easy enough to implement, but I think it feels a bit weird to emit single values in some cases and arrays in others. We might end up in a case where a callback is called with a different number of arguments at different times, and that would get hard to abstract over.

Perhaps a wrapCallbackN would work? You'd pass it a function and expected number of arguments and it would always emit an array of that length. Something like:

_.wrapCallbackN = function(fn, n) {
  return _.wrapCallback(function(...args, cb) {
    fn(...args, function(e, ...results) {
      cb(e, ensureArrayLength(n, results));
    });
  });
}

(Note that, unlike other cases such as uniq/uniqBy, wrapCallbackN(_, 1) would not be equivalent to wrapCallback)

@d4h0
Copy link
Author

d4h0 commented Mar 16, 2015

Isn't this the same problem that the EventEmitter Stream constructor had?

Maybe that code can be reused?

@apaleslimghost
Copy link
Collaborator

Ah, good find. Adding mappingHint to wrapCallback is a good idea, I'll send a pull request in a bit.

p.s. We usually avoid optional parameters, because they don't work with currying, but since this is a constructor function, not a stream method, that doesn't apply here (unless anyone disagrees!)

@apaleslimghost apaleslimghost self-assigned this Mar 16, 2015
@apaleslimghost
Copy link
Collaborator

#247

@d4h0
Copy link
Author

d4h0 commented Mar 17, 2015

Thanks, @quarterto!

@vqvu
Copy link
Collaborator

vqvu commented Mar 17, 2015

Reopening until #247 is merged.

@vqvu vqvu reopened this Mar 17, 2015
@dypsilon
Copy link

dypsilon commented Jul 5, 2015

Please consider solving #334 with mappingHint.

@vqvu
Copy link
Collaborator

vqvu commented Jul 5, 2015

@quarterto, you you have time to finish up #247?

@apaleslimghost
Copy link
Collaborator

Wow, sorry, that totally slipped off my radar. I'll have a look this afternoon.

@vqvu vqvu closed this as completed in 531eea6 Jul 6, 2015
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

No branches or pull requests

4 participants