Conversation
lib/index.js
Outdated
| _.tap = _.doto; | ||
|
|
||
| /** | ||
| * Applies a function to each value from the source, and re-emits the |
There was a problem hiding this comment.
Can you mention that
- The function must return a Highland stream.
- The source value will not be re-emitted until the stream that the function returns ends.
lib/index.js
Outdated
| * @param {Function} f - the function to apply | ||
| * @api public | ||
| * | ||
| * _([1, 2, 3]).flatTap(httpLog) |
There was a problem hiding this comment.
Can you add an example for how httpLog would need to be implemented? Maybe using promises + setTimeout + console.log to illustrates the fact that the stream will wait until the stream completes before it continues..
lib/index.js
Outdated
| addMethod('flatTap', function (f) { | ||
| return this.flatMap(function (x) { | ||
| return f(x) | ||
| .flatMap(function () { |
There was a problem hiding this comment.
I know I mentioned that flatTap could be implemented with flatMap like this, but a complete implementation should account for the fact that the stream f(x) may emit zero values or more than one values. You're better off implementing with consume and looking for the _.nil sentinel instead.
test/test.js
Outdated
| test.done(); | ||
| }; | ||
|
|
||
| exports.flatTap = function (test) { |
There was a problem hiding this comment.
Can you implement the tests using the object syntax (see the tests for pull as an example). This will group them together into a suite that makes it easier to run separately.
test/test.js
Outdated
| var seen; | ||
| function record(x) { | ||
| var y = x * 2; | ||
| seen.push(y); |
There was a problem hiding this comment.
seen.push(y) should be done asynchronously. You want to verify that the stream actually waits until the completion of the async action before continuing.
There was a problem hiding this comment.
Oh, I see you do this in the GeneratorStream and ArrayStream tests. You can just remove this test then, since it doesn't seem to add any value.
test/test.js
Outdated
| var f = function (x) { | ||
| return _(function (push, next) { | ||
| setTimeout(function () { | ||
| push(null, x * 2); |
There was a problem hiding this comment.
Can you verify that this code executes before toArray completes? The seen.push(y) approach that you use above is fine.
Also do this for the GeneratorStream test.
|
Hi @tomwhale, Sorry for dropping the ball on this. I made some comments when you first sent this PR, but got confused by Github's interface and never ended up posting them. |
|
Sorry, had a chance to pick this up again. I think I have resolved all your comments. If I have misunderstood what you meant, lemme know! |
vqvu
left a comment
vqvu
left a comment
There was a problem hiding this comment.
This looks good. I just have a comment about error handling.
lib/index.js
Outdated
| * @param {Function} f - the function to apply | ||
| * @api public | ||
| * | ||
| * const httpLog = H(new Promnise(res => { |
There was a problem hiding this comment.
Use _ instead of H for the Highland object.
| * const httpLog = H(new Promnise(res => { | |
| * const httpLog = _(new Promnise(res => { |
lib/index.js
Outdated
| return this.flatMap(function (x) { | ||
| return f(x).consume(function (err, y, push, next) { | ||
| if (err) { | ||
| // next(); |
There was a problem hiding this comment.
I think you should push the err in this case and continue on with the stream. That's what most transforms do. The user can use stopOnError if they want to stop the stream early.
There was a problem hiding this comment.
Okay, this is an interesting one though, as it is a tap you could argue the error should be silenced? I think I do agree with you though.
There was a problem hiding this comment.
There's definitely a lot of uses of tap where you want to ignore errors. I just don't like unilaterally silencing errors in library functions. That's something that developer using the library should decide.
Also, your fix isn't entirely correct. It pushes nil immediately after an error. In general, Highland streams don't end after the first error, so you need to call next instead of push(null, _.nil).
| s.pull(valueEquals(test, 3)); | ||
| test.done(); | ||
| }, | ||
| 'flatTap - argument function throws': function (test) { |
There was a problem hiding this comment.
Can you add a similar test to this, but for the stream (i.e., the return value of the argument function) emitting an error instead.
Something like this,
_([1, 2, 3, 4]).flatTap((x) => _((push) => {
push(err);
push(null, _.nil);
}));2 participants
From Issue #616
This method creates a short hand for the following:
stream.flatMap(item => save(item).map(_ => item))where save returns a highland stream.