wrapCallback doesn't work with single-argument-callbacks

[dypsilon]

WrapCallback provides a callback function which expects two arguments:

1. An error/null.
2. The actual value.

This doesn't work with functions like fs.exists or http.get because they call the function only with the result. See the code in question.

This behavior breaks the example of flatFilter():

var checkExists = _.wrapCallback(fs.exists);
filenames.flatFilter(checkExists)

This code will always throw 'Uncaught, unspecified "error" event.'

[vqvu]

Fixed in #247.

[dypsilon]

Looking at the code, not sure it fixes the issue:

var cb = function (err) {
    if (err) {
        push(err);
    }
    [...]
};

This will always push the value as an error in case of fs.exists and other callbacks with single argument.

[vqvu]

Oops. You're right. You mentioned mappingHints so i just assumed it would work. I think this is a wontfix though, since these types of callbacks seem rare to me. And wrapCallback isn't meant for arbitrary callback protocols anyway. You should just implement your own. It's not hard.

[dypsilon]

Other libraries (Rx) deal with this problem by introducing wrapCallback and wrapNodeCallback functions. Maybe we could implement something like wrapCallback1? This use case appears quite often.

Also we should probably remove the flatFilter example from the docs which requires exactly that behavior to work.

[dypsilon]

I expected, that the mapper would allow me to map all arguments including the first. I don't see any reason why it shouldn't.

[vqvu]

It doesn't because wrapCallback has to deal with errors from node callbacks gracefully, and the mapper has no way of pushing errors. It's not reasonable to pass the error to a value mapper. Rx#fromNodeCallback has a mapper function too, and they still have two versions for the same reason.

Even if we add a non-error version, we can't call it wrapCallback1, since the 1 signifies the number of arguments to wrapCallback and not the number of arguments to the argument's callback. I'm not convinced it's worth the effort coming up with a proper name. Like I said, it's fairly easy to roll your own for this special case.

Can you give other examples of this usecase (beyond fs.exists)? Please cite the API of common libraries or the node standard library.

Good point on the flatFilter docs though. I'll get it fixed.

[dypsilon]

Not easy to cite the APIs off the top of my head :) The reason I ran into this problem in the first place was the HTTP core API. For example http.request and http.get methods. I assure you, there are tons of APIs in the wild with generic callbacks, especially on the clientside.

I guess the way to use them would be:

          .flatMap(function(pkg) {
                var requestOptions = { ... };

                return _(function(push) {
                    http.get(requestOptions, function(response) {
                        push(null, response);
                        push(null, _.nil);
                    });
                });
            });

That's what I do now.

[vqvu]

Fair enough. I'm not too familiar with client-side libraries, so that may be why I don't see them very often.

We may be able to support this in 3.0 by renaming wrapCallback to wrapNodeCallback. And implementing wrapCallback for the generic version. I'd like at least one of the other collaborators to chime in (+1/-1?) since this involves a name change.

I guess the way to use them would be:
...

Yeah. That's how you'd do it. You can also define a custom getStream if you find yourself doing it often.

function getStream(requestOptions) {
    return _(function (push) {
        http.get(requestOptions, function (response) {
            push(null, response);
            push(null, _.nil);
        });
    });
}

[apaleslimghost]

I'm -1 on renaming wrapCallback. We could name the new function wrapUnaryCallback? Or something similar.

@dypsilon: you could also wrap http.request with a function that makes its callback node-style, something like:

function cbToNode(fn) {
  return function(...args) {
    var cb = args.pop();
    fn(...args, function(...cbArgs) {
      cb(null, ...cbArgs);
    });
  }
}

var request = _.wrapCallback(cbToNode(http.request));

edit: released a quick npm module for this: @quarterto/cb-to-node

[vqvu]

Well, if we were going to support this, it'd be for generic non-node callbacks of arbitrary number of arguments, so "unary" wouldn't cut it. wrapGenericCallback?

[dypsilon]

IMHO wrapGenericCallback sounds better.

Also this introduces some API inconsistencies. The usual way to create a stream is _.(source) which is great, but very limited. Following the wrapCallback logic, there should rather be different methods for each source type:

• wrapPromise
• wrapNodeStream
• wrapNodeCallback
• wrapNodeEventEmitter
• ...

Just a thought.

[dypsilon]

Maybe it even makes sense to create a different library, something like highland-interop and extract all source methods into this one library. The constructor _.() would only support a generator function in this case. What do you think about this idea for 3.0?

[vqvu]

I'm not opposed to an interop library, but the things you mention aren't inconsistencies, so I'm not sure it's worth it. The _() constructor takes a data object and returns a stream. wrapCallback takes a function and returns another function. If wrapCallback took a node function and returned a stream, then I'd agree with you.

[vqvu]

By the way, since wrapGenericCallback doesn't break anything, can you submit a PR for it? If you don't want to, then I can do it when I have time.