Support continueOnError

[floatdrop]

It would be nice, if map can continue after error emitted.

[dominictarr]

Hmm, I'm not really sure that this is a good idea.
what are you using this for, and why do you think this is helpful?

I mean, ignoring errors isn't something I want to encourage by having a configuration option,
If you really want to do that, you should ignore the errors from your own code.

[floatdrop]

This isn't ignoring errors - they will be emitted anyway, but stream processing will be continued.

We using this in compiling separate files through coffee-script. Source stream emitting file paths and map processing them through coffee-script compiler. If one file failed to compile we want to continue process, just show the error (which, again, will be emitted).

[yocontra]

👍

[dashed]

@dominictarr In reference to findings in gulpjs/gulp#75 (comment), the map-stream's behaviour is that it'll stop iterating on error. The behaviour as it is currently implemented assumes that the next data chunk depends on the current. This is useful for streaming lines of text from a file. However, in the general case, this doesn't cover collections where items are independent of each other (e.g. collection of files).

The following snippet demonstrates the issue:

var es = require('event-stream');

// a hack to continue on error for streams
var continueOnError = function(stream) {

    return stream
    .on('error', function() {})
    .on('newListener', function() {
       this.listeners('error').forEach(function(f) {
           if(f.name == 'onerror') this.removeListener('error', f);
       }, this);
    });
};

// something that returns stream using es.map internally
var plugin = function(filter, filter_func) {

    if(filter_func == void 0)
        filter_func = function(n) { return n; };

   return es.map(function (data, cb) {

      if(filter_func(data) == filter) return cb(new Error(filter+''));

      console.log('caught in es.map: ' + data)
      return cb(null, data);

   });
};

es.readArray([1,2,3,4,5])
    .pipe(continueOnError(plugin(3)))
        .on('error', console.log)
    .pipe(es.through(function(n) {

        console.log('caught in es.through: ' + n);

        this.emit('data', n);
    }));

Output:

caught in es.map: 1
caught in es.through: 1
caught in es.map: 2
caught in es.through: 2
[Error: 3]
caught in es.map: 4
caught in es.map: 5

From the output, despite our best efforts to do return cb(null, data); within a map-stream, the data is dropped since an error has been emitted previously.

In addition, the error is emitted and caught from map-stream fine.

The desired output is the following:

caught in es.map: 1
caught in es.through: 1
caught in es.map: 2
caught in es.through: 2
[Error: 3]
caught in es.map: 4
caught in es.through: 4
caught in es.map: 5
caught in es.through: 5

Hopefully this explains the need for this pull-request.

[sindresorhus]

👍

[dominictarr]

This is a bit fragile, and not a very clean change, since you need to include the hack to make pipe not stop on an error. this assumes that it can reliably detect pipe's internal error handler based on listener being named onerror. I think what would work much better is emit a different event, for example, itemerror or lineerror or which you could listen on, but wouldn't need to make a change es.map or use a weird hack that depends on internal aspects of node's .pipe implementation.
you easily make handleable map like this:

module.exports = function (map) {
  var stream  
  return stream = es.map(function (data, cb) {
    map(data, function (err, data) {
      if(err) stream.emit('lineerror', err) cb()
      else cb(null, data)
    })
  })
}

I'd consider an option to emit a different event on error,
to one that isn't error and so doesn't stop the stream.

[dominictarr]

the question is, what is the best name for a handleable error?

[dominictarr]

also, question: in the context of your build system, how do you handle errors? do you retry?
can you really get a correct build if there is an error? or do you just want to collect all the errors so you can display a more useful response to the user?

[yocontra]

@dominictarr how about "failure"? There are a lot of synonyms for error that could be used

As for handling errors it depends on the plugin or the user. gulp itself doesn't do any error handling at all. I think users still expect it to keep going even when there is an error.

[dashed]

Also to note that errors handled/behaved as it's implemented by node.js stream.

[dominictarr]

@contra can you give me a concrete example of something you are building with gulp, a type of error that may occur, and why you'd want to keep on going in that situation?

I see from your website that gulp is a bulid tool, but I didn't see at first glance what sort of of things it is intended to build. In most cases I can imagine, like, a compiler, or even building css etc,
I probably wouldn't want a partial success - because if one file had a syntax error then the rest is useless.

The best case I can think of where sometimes I'd want this is when testing. Sometimes I might want to know all the tests that fail, rather than stopping on the first failed test. On the other hand, it's much simpler to just stop on the first error so normally that is fine, even in testing.

[yocontra]

@dominictarr Personally I agree with you and I think the whole thing should fail when one file does - people keep opening issues about this though and they want this behavior so I'd like to give them an option somehow to do things the way they want.

I think most people want this option for testing/linting where an error may not be a reason to stop the whole show.

[dominictarr]

I'm serious about wanting an concrete example. Can you link me to one of these issues?

[yocontra]

@dominictarr So am I but I'm not the one who's asking for this - gulpjs/gulp#75 (comment)

[dashed]

@contra
I thought about this some more, and came to the conclusion that this PR wouldn't help much in the long-run for gulp.

Since, if a gulp-plugin uses this internally, plugin devs would need to expose the map's config options through the plugin for this to be of any use.

[yocontra]

@dominictarr Also related gulpjs/gulp#91

[floatdrop]

@dashed you can modify this property from outside module.

[dashed]

@floatdrop But you shouldn't really know what kind of stream the plugin returns; even if you looked at the source code. You just assume the stream it returns is compatible with nodejs streams.

[floatdrop]

@dashed well, sometimes you should do if (source.opts && source.opts.continueOnError !== false) { source.opts.continueOnError = true; } - or emit non errors.

The problem is - we already got tons of plugins on map-stream - and this option could get desired behaviour in gulp-watch with minimum of changes/PR's to plugins.

[dominictarr]

Aha, okay now this all makes sense. a watch stream is the best example.
This is a lot like browserify transforms.
So, how are you going to collect errors?
how do they get through to the end of the output if you have a long pipeline of transformations.

maybe you need something that works like stdout and stderr?

Oh, by the way, I'd recommend using map-stream directly, instead of using event-stream and then only using map. I wrote event stream before @substack had convinced us all that "kitchen sink" utility collections where a bad idea.

[yocontra]

@dominictarr Yeah I realized this too - event-stream is useful for rapid prototyping though when you don't know exactly what you'll need. I've refactored all of my wiki examples to use through

[dashed]

@dominictarr errors are collected in the usual way where users have to attach .on('error').

Something like https://github.com/floatdrop/gulp-plumber monkey-patch the pipe method, and addresses the error collection you mentioned.

[yocontra]

@dominictarr I'm interested in any ideas you have for error management using pipelines like this - this has been one of the only pain points for gulp so far

[dashed]

I'd like to clarify that gulp-plumber doesn't actually pass errors downstream. Just attach a default error listener.

[dominictarr]

yeah, this is a tricky problem. node's streams are very simple, and don't propagate errors.

I've also experimented with alternative stream models, pull-stream that propagate errors - pull-streams work great for object streams.

They are lazy (pull only) and they are easy to combine: you can describe streams in terms of
other streams: transform = pull(transform1, transform2, transform3)

you can do this with node, but it feels clumsy: https://github.com/dominictarr/stream-combiner

One way I'd be tempted to implement gulp is to make a stream of streams,
but I have a feeling that really, the answer is unix. unix has a stream for data and another stream for errors.

maybe instead of using pipe, you could have a different method that handled errors also,

gulp(
  gulp.src('./src/*.coffee'),
  coffee(),
  gulp.dest('./dist')
)

The streams can still be normal node streams, but gulp would handle adding the error listeners, etc.

[dashed]

Is it necessary to propagate errors?

[dominictarr]

Propagating errors is very useful in the sorts of things that I've used pull-stream for.
I mentioned it because although node gets streaming basically right, it still leaves something to be desired.

Say for example, you do an http request, and then stream that to a file via multiple transforms
(say, decrypt then uncompress) suppose there is an error some where along the pipeline,
the pipe stops, and when each unit sees the error it tidies up, say, by closing the http connection,
or deleting the partically written file. You have to do this manually with node streams, and it's quite tricky.

However, I think this isn't important for gulp. you really just need a way to see errors, but keep everything flowing.

[floatdrop]

@dominictarr yep, this is what we want and this is what through for. But for now map-stream is intentionally stops processing data on error.

[dominictarr]

okay, so If you send me a pull request that optionally uses a different error name and continues,
I'll merge that.

[floatdrop]

Added option for different error name and some tests.

[dominictarr]

having a configurable error name is bad (because everyone will pick a different one),
can you make it just one boolean option {failures: true} that enables the failure event,
and continues?

You should try and avoid configuration options - because each option adds edgecases.
But at least having a single boolean option you have less edge cases than a boolean and a string.

[floatdrop]

Sure thing. Done.

[dominictarr]

oh, just one thing, can the opts argument be the second argument?
mandatory arguments are usually first and optional arguments to the right.
(but to the left of the callback(err, data) which is always last)

All my stream modules have optional arguments in that order,
and so do most core node apis.

[floatdrop]

Done (imo: this will be harder to read, but options is very rarely will be used).

[dominictarr]

Maybe, but i think being consistent is more important.
merged into 0.1.0

[dominictarr]

available in event-stream@3.1.0