wrapAsync

[lpww]

Add wrapAsync. A function that wraps a function that returns a promise, transforming it to a function which accepts the same arguments and returns a Highland Stream instead.

Addresses #517

@vqvu I didn't add any additional error handling because it looks like the constructor already handles promise errors in the same way as _.fromError (by creating an error stream with a single value). I've included that code for reference:

function promiseStream(StreamCtor, promise) {
    if (_.isFunction(promise['finally'])) { // eslint-disable-line dot-notation
        // Using finally handles also bluebird promise cancellation
        return new StreamCtor(function (push) {
            promise.then(function (value) {
                return push(null, value);
            },
                function (err) {
                    return push(err);
                })['finally'](function () { // eslint-disable-line dot-notation
                    return push(null, nil);
                });
        });
    }
    else {
        // Sticking to promise standard only
        return new StreamCtor(function (push) {
            promise.then(function (value) {
                push(null, value);
                return push(null, nil);
            },
            function (err) {
                push(err);
                return push(null, nil);
            });
        });
    }
}

[vqvu]

Trying out the new code review feature. See my comments.

[vqvu]

You don't need a self here. Just pass this to f.apply directly.

[lpww]

Fixed.

[vqvu]

You can save an allocation by passing arguments to f.apply directly.

[lpww]

Cool! I've always assumed that arguments must be converted to a proper array before you can use it like this.

[vqvu]

Technically f.apply() should not throw, but we should wrap it in a try-catch just in case.

Furthermore, stream can throw an error if the result of f.apply(...) is not an acceptable type. Since stream is less restrictive than we are, we should also check that the result is a promise before passing it to stream. If it is not a promise, we should return a _.fromError with an appropriate error.

We don't want the wrapped function to ever throw since it is supposed to be async.

[lpww]

I think I followed along on this part. Please take a look at my updates

[vqvu]

Yep. What you did was correct.

[vqvu]

Promise.resolve and Promise.reject are standard functions, so I'd prefer not to mention a specific Promise library here.

[lpww]

Good point. Removed the bluebird reference.

[vqvu]

The current tests are all over the place, but please use the nodeunit grouping feature:

exports.wrapAsync = {
    'basic functionality': function (test) {
        ...
    },
    'context': function (test) {
        ...
    },
    ...
};

This will group all of the wrapAsync tests together and allow us to manually run all only the tests in the group. It'll also be useful when I go in and finally split up this giant file.

---

Also, please add a test.expect(numAsserts) at the start of every test so that omitted assertions will fail the test.

[lpww]

Yup. I just copied the wrapCallback tests. It's good to know that you can group them like this and pass in the expected number of assertions.

[vqvu]

I don't think this test verifies that the context is correctly passed. You need to use this inside f in some way. This is probably a copy-paste error since the wrapCallback version checks the context with test.equal(this, o);.

[lpww]

You are correct. This is fixed now too.

[lpww]

Also, I'm aware that I have not updated the changelog. I have some questions about that and am planning to do it after the rest of the work is compete.

[vqvu]

From a code style standpoint, I don't like explicitly throwing exceptions from within a try-catch like this. It looks wrong at first glance. You should just return _.fromError(...) here.

[vqvu]

Just one minor point. Everything else looks good.

[lpww]

@vqvu I removed the throw from the try block and added an extra test for that specific error. I'm ready to update the changelog but I'm not sure what version to put this under. Would it be a new version 2.11.0?

[vqvu]

It should be under 3.0.0.