Added complete method, just like toArray but passes first error if occurs

[robertstettner]

This request is related to #484. I wanted to have way of consuming the stream and calling a standard callback which takes an error if occurred as the first parameter.

[vqvu]

@robertstettner Thanks for this. I have two comments.

1. Can you add a link to Clearer way of consuming a stream and stopping on errors #484 in the changelog? (i.e., Fixes [#484](...).).
2. Can you update the implementation to something like what I provided in Clearer way of consuming a stream and stopping on errors #484 (comment). The rationale for the change is also in that post.

[robertstettner]

@vqvu No problem. I have updated with your comments.

[vqvu]

I merged #487 before this, and it looks like master conflicts with the changes now (probably from the CHANGELOG.md). Can you rebase?

Also, I've been thinking about the name, and complete doesn't seem terribly descriptive. How about toArrayOrError to mirror the existing toArray operator.

[robertstettner]

@vqvu I see your point. However, I feel that toArrayOrError doesn't suggest that the method will completely consume the stream, and return you an error as first argument. This method is more about the first argument than anything else. A standard Node.js callback. How about a simple end? or maybe then?

But then has connotations of passing it a resolve and reject handler, the only thing going for it is readability, eg.

const done = (err, x) => {
    if (err) console.err(err);
    // do something
};
s.then(done);

EDIT: maybe even finish?

[vqvu]

However, I feel that toArrayOrError doesn't suggest that the method will completely consume the stream, and return you an error as first argument.

It doesn't completely consume the source stream when there's an error. For example,

_((push, next) => {
    push(new Error("Error"));
    push(null, 1);
    push(new Error("Error2"));
    push(null, _.nil);
})
.doto(_.log)
.errors((err, push) => {
    _.log(err);
    push(err);
})
.complete(_.log)

// This should only print the single error (though it'll happen twice).
// => [Error: Error]
// => [Error: Error] undefined

Both the current and your original implementation does this.

This is the only reasonable way to implement this operator. That is, we assume that the caller wants stopOnError to happen. Doing anything else would involve possibly executing the callback multiple times.

We can't use then because it may conflict with promise-detecting code. We do this ourselves in Highland to detect whether or not something is a promise.

finish/endare just synonyms for complete. I'm not sure it conveys any more information. More importantly, they're both synonyms for done. Without referencing the docs, someone not intimately familiar with the library wouldn't be able to tell that finish/end/complete requires a (err, x) => {} and done requires a () => {}. This is my main concern.

toArrayOrError at least tells you that you either get an error or an array (if no errors). To me, it suggests that this is a version of toArray that doesn't require you to attach an error event handler.

I agree that this method is about the first argument. So shouldn't we call that out?

[0chroma]

ooh, I love naming problems. I had a hard time finding a close match, but conclude and undertake might be good starting points in a thesaurus hunt. I feel that conclude isn't great since it is kind of close to complete, so suffers from the same type of confusion. undertake is slightly better, but isn't incredibly descriptive of what result we're returning, just that it could either return a result of some kind or fail.

It might be hard to make it a single word without there being some ambiguity though, so maybe something like attemptToArray could work as well (or maybe replace ToArray with some other verb). toArrayOrError is also pretty good, I just wish we could make it less verbose.

[vqvu]

Welcome to the party! 😄

I don't think either conclude or undertake works. As you said, conclude is too similar to complete and undertake implies starting something more than ending it.

I'm not too happy about the verbosity of toArrayOrError either, but I'm not sure if there's anything better given the need to clearly differentiate between this operator and existing ones. Plus, it's not without precedence. toArrayOrError is 14 characters and we already have

• batchWithTimeOrCount (20 characters)
• mergeWithLimit (14 characters)
• attemptToArray (14 characters 😉)

[robertstettner]

I quite like attemptToArray, because you are acknowledging that it could fail, and give you an error first. Adding my extra 2 pennies, what do you think of catchWithToArray?

EDIT: @vqvu I do see your point about complete now, and the fact that it won't complete (i.e. completely consume the stream) when there is an error.

[0chroma]

I just had a weird idea, since the intension is to make it compatible with a callback-return pattern, maybe call it _(...).callback(cb) or something similar? It doesn't convey that it stops on the first error through the name, but you should be able to derive the behavior based on what the pattern entails I think (either it succeeded with a result or it threw an error during some step). It's also much more concise.

Maybe for versatility we could also have an optional shouldStopOnError argument or something, but since the callback pattern doesn't really have a way of passing back multiple errors, it's probably not the best idea (it's something we could always add later as well).

[vqvu]

You make a good point about the intention to convert to the Node callback pattern. I really like toCallback (being the dual of fromCallback). attemptToArray is also growing on me.

Maybe for versatility we could also have an optional shouldStopOnError argument or something, but since the callback pattern doesn't really have a way of passing back multiple errors, it's probably not the best idea.

Yeah. I'm not sure how we would do this and still stay in the Node callback pattern.

For versatility, though, we could require users to use this operator only when there is a single value in the stream. That is, rather than forcing a collect(), we can allow users to do whatever is necessary to get the stream down to a single value. So they can do

stream.collect().toCallback(cb) // or
stream.take(1).toCallback(cb) // or
stream.reduce(...).toCallback(cb) // or
stream.through(customOperator()).toCallback(cb) // or simply
stream.toCallback(cb)

We'd have to call it toCallback, since arrays aren't in the picture anymore. We'd also want to test catch the cases where the stream doesn't have exactly one value and emit an error for it. So the behavior would be something like

1. call stopOnError.
2. consume the stream, recording errors and/or values.
3. When we receive _.nil,
   1. If there is exactly one error or value, execute cb(err, x).
   2. If there are no errors or values, execute cb(new Error('Stream emitted no values or error')).
   3. If there are multiple values or there was both an error and a value, execute cb(new Error("Stream emitted more than one value or error.")).

[0chroma]

I think allowing the stream to emit no values is okay, although it's not a common case, there are some libraries that call back with only an error argument. The other cases sound good, but I'm worried about introducing hard-to-detect runtime errors by requiring only one item to be in the stream. Maybe just callback with the first emitted item instead?

[vqvu]

That's a good point about allowing no values.

I'm not too concerned about emitting errors for the other cases. There's only so many call-sites for the operator, especially since it's a consumption method, which are called much more rarely. Plus, it feels wrong to be ignoring such an obvious and easily detectable bug.

Users can stick a take(1) before this operator if they want to be defensive.

[0chroma]

Yeah, that's a fair point, I guess if the end-developer is properly testing their code it should only help and not hurt if we throw when multiple values are emitted.

[robertstettner]

@vqvu attemptToArray is it then. For the toCallback functionality, shouldn't that be it's own PR? Or would you like me to get it done with this one?

[vqvu]

I'd like to change this PR to the toCallback functionality. attemptToArray would then be equivalent to collect().toCallback(cb), so there's no longer a need for it.

[0chroma]

mhm, +1 to fewer methods in the library in favor of chaining for equivalent functionality

[vqvu]

Closing this since #493 was merged.