Continue gulp on error? about gulp HOT 42 CLOSED

@contra I put together a testbed at: https://github.com/Dashed/gulp-coffee-sandbox to demonstrate the issue.

Just npm install and gulp.

There are four coffeescript files:

subdir/a.coffee
subdir/b.coffee
subdir/c.coffee
d.coffee

The file b.coffee contains invalid coffeescript. Only c.coffee would not get compiled.

from gulp.

Isn't --continue supposed to do this? It doesn't, but isn't it supposed to?

from gulp.

My resolution is the following:

gulp.src('./**/*.coffee')
  .on('data', function(file) {

     // Custom dest dir
    destDir = ...;

    gulp.src(file.path)
      .pipe(coffee({bare: true}))
        .on('error', gutil.log)
        .on('error', gutil.beep)
      .pipe(gulp.dest(destDir));
  });

Let me know if there is a better way.

from gulp.

gulp.src('./**/*.coffee')
  .pipe(coffee({bare: true})
    .on('error', gutil.log)
    .on('error', gutil.beep)
  )
  .pipe(gulp.dest(destDir));

should work

from gulp.

Nesting .on() methods doesn't make a difference. It shouldn't since, from what I read from node docs, .pipe() and .on() return the stream of the current context which in this case is currently coffee({bare: true}).

I think it makes more sense to create new individual streams for each coffee file; much what I have.

from gulp.

I am able to catch the error events fine. It's that once an error event occurs (regardless of whether it is caught), the entire process stops.

from gulp.

@dashed probably event-stream behavior. Will check it out

from gulp.

I did a little introspection on gulp-coffee. It seems that for es.map(...), once you pass an error via callback(error), the map stops processing.

Not sure how to get around this.

I think this would be critical for something like #13.

from gulp.

@dashed where do you see that? I looked at it earlier and it seemed to keep going

from gulp.

This is not issue with gulp, but rather in map-stream/gulp-coffee. If you add more debug output, you can see...

gulp] Using file /Users/floatdrop/gulp-coffee-sandbox/gulpfile.js
[gulp] Working directory changed to /Users/floatdrop/gulp-coffee-sandbox
[gulp] Running 'default'...
[gulp] Finished 'default' in 16 ms
[gulp] Prepearing to compile '/Users/floatdrop/gulp-coffee-sandbox/coffee/d.coffee
<File "d.coffee" <Buffer 64 20 3d 20 34 0a>>
[gulp] Prepearing to compile '/Users/floatdrop/gulp-coffee-sandbox/coffee/subdir/a.coffee
<File "subdir/a.coffee" <Buffer 61 20 3d 20 31 0a>>
[gulp] Prepearing to compile '/Users/floatdrop/gulp-coffee-sandbox/coffee/subdir/b.coffee
<File "subdir/b.coffee" <Buffer 62 20 3d 20 32 73 61 64 73 61 64 0a>>
[gulp] [Error: /Users/floatdrop/gulp-coffee-sandbox/coffee/subdir/b.coffee:1:6: error: unexpected IDENTIFIER
b = 2sadsad
     ^^^^^^]
[gulp] Prepearing to compile '/Users/floatdrop/gulp-coffee-sandbox/coffee/subdir/c.coffee
[gulp] Compiled 'coffee/d.coffee' to 'src/d.js'
[gulp] Compiled 'coffee/subdir/a.coffee' to 'src/subdir/a.js'

...that gulp.src still fires files at gulp-coffee, but es.map is not responding.

With a little luck, you can find this lines that breaks your workflow.

from gulp.

@floatdrop I'm sure they would take a PR to change that behavior if you think it is a bug or you could fork it into a new module that supports what you want

from gulp.

I don't think this is a bug. This seems to work correctly by design since event-stream assumes node Streams are v0.8 (https://github.com/joyent/node/blob/v0.8/doc/api/stream.markdown).

It appears that es.readable has an undocumented continueOnError option as described in this blog post: http://thlorenz.com/blog/event-stream Almost what is needed.

The blog post was actually more informative than the event-stream docs.

I'm unsure how to amend map-stream.

from gulp.

Okay, I investigated the problem, and thats what i found:

First - I was wrong about map-stream and this can be fixed in gulp-coffee, or even in gulpfile.js which is nice. As I debugged it, I found this node code in Stream library (this is a pipe function):

So when stream pipe'ing to another - nasty onerror callback is applyed, which drops data handlers.

When it's clear what's happening - then you can go into gulp-coffee and place this code at the end of file instead of return es.map(modifyFile):

return es.map(modifyFile)
    .on('pipe', function(source) {
      this.removeAllListeners('error');
    });

I really feel like this is a hack, but it works. I opened pull-request for your example repo, and updated the wiki.

from gulp.

Ah interesting. On my side, I simplified to something like:

es.readArray([1,2,3])
   .pipe(es.through(function write(data) {
      data === 2 ? this.emit('error', new Error('2')) : this.emit('data', data + '\n');
   }))
      .on('error', console.log)
   .pipe(process.stdout);

and eventually came to the conclusion that the error behaviour is built-in. Unfortunately, I didn't look at node.js source code to see what was the cause.

I looked at your proposal, and nothing seems to have changed (i.e. stream still ends before c.coffee is considered). But it looks promising.

So I looked into the 'pipe' and 'newListener' events on node's EventEmitter to see what kind of effect your proposal has.

I came up with a gulpfile.js and achieved partial results:
https://github.com/Dashed/gulp-coffee-sandbox/blob/a0b019f10edc0acbfce8ea141b09eb735f159b0c/gulpfile.js

Here is a summary of my findings:

IMO, doing this.removeAllListeners('error'); on pipe event (as the first event listener) is dangerous. The reason is that one could attach their own .on('error') event listener to the plugin stream before being put into .pipe().

// 
var gulpCoffee = coffee()
    .on('pipe', function() {this.removeAllListeners('error');})
    .on('error', awesomeErrorHandling);

gulp.src(...)
// ...
.pipe(gulpCoffee); // awesomeErrorHandling is now removed

The order in which you attach event listeners matters when you're removing events in those same event listeners.

// 
var gulpCoffee = coffee()
    // swap
    .on('error', awesomeErrorHandling);
    .on('pipe', function() {this.removeAllListeners('error');})


gulp.src(...)
// ...
.pipe(gulpCoffee); // awesomeErrorHandling is still there

Since onerror is a named function, we can identify it via Function.name. So detaching it becomes easy:

if(fn.name == 'onerror') this.removeListener('error', fn);

I tried removing onerror at pipe and newListener events.

This resolves some issues I identified.

Ideally, we want to catch emitted errors in this manner:

gulp.src(...)
    .pipe(plugin())
        .on('error', gutil.log)
    // ...

But despite removing onerror at pipe and error events, it seems to get attached whenever .on('error') is called.

So I resorted to iterating the listeners and thoroughly clean it:

// clean stream of onerror
var cleaner = function(stream) {
    stream.listeners('error').forEach(function(item) {
        if(item.name == 'onerror') this.removeListener('error', item);
            // console.log('removed listener ('+ item.name +') for error');
    }, stream);
};

At this point, it looks like something that can be deferred as a gulp-plugin. I played with pipe-able and decorator versions (both in gulpfile.js).

The pipe version requires to be piped after an .on('error'), so it's not that flexible. The decorator version, however, looks very promising:

// decorator version
var continueOnError = function(stream) {
    return stream
    .on('pipe', function(src) {
        cleaner(src);
    })
    .on('newListener', function() {
        cleaner(this);
    });
};

Usage is:

gulp.src(target)
    .pipe(continueOnError(coffee({bare: true})))

As of gulp-coffee 1.2.4, this still doesn't work. However, if I modify gulp-coffee to use es.through instead of es.map, it works perfectly. I submitted a pull-request regarding this.

Note that es.map correctly receives c.coffee and transforms it. Once it's pushed through the callback, however, it's lost. This is probably a bug.

from gulp.

continueOnError won't work due to #75 (comment)

from gulp.

Seems like I overpatched map-stream somehow. Can you update wiki?

from gulp.

Updated wiki.

from gulp.

To note in this issue, the onerror function is added in this commit: nodejs/node-v0.x-archive@7c6f014#diff-f2ffb054adb0a2de0c0635929d6da4c6R78
which is quite a while ago. But the stream API is still unstable; so it's good to keep an eye on it.

Also continueOnError should attach a dummy error listener:

// decorator version
var continueOnError = function(stream) {
    return stream
    .on('error', function(){})
    .on('pipe', function(src) {
        cleaner(src);
    })
    .on('newListener', function() {
        cleaner(this);
    });
};

This is important for when an error from a gulp-plugin is not caught. So continueOnError should silently catch it for the user.

from gulp.

Well, this is bad. continueOnError will kill all errors, and I would not know about errors in coffee files. This is not a way to go.

from gulp.

Really? I'm able to catch them fine:

[gulp] Using file /Users/albertoleal/Documents/Projects/javascript/gulp-coffee-sandbox/gulpfile.js
[gulp] Working directory changed to /Users/albertoleal/Documents/Projects/javascript/gulp-coffee-sandbox
[gulp] Running 'default'...
[gulp] Finished 'default' in 7.66 ms
[gulp] [Error: /Users/albertoleal/Documents/Projects/javascript/gulp-coffee-sandbox/coffee/subdir/b.coffee:1:6: error: unexpected IDENTIFIER
b = 2sadsad
     ^^^^^^]
[gulp] Compiled 'coffee/d.coffee' to 'src/d.js'
[gulp] Compiled 'coffee/subdir/a.coffee' to 'src/subdir/a.js'
[gulp] Compiled 'coffee/subdir/c.coffee' to 'src/subdir/c.js'

from gulp.

Well, this is because you added on('error', gutil.log) after continueOnError. Why you need .on('error', function(){}) then?

from gulp.

continueOnError only works on the stream (or gulp-plugin) that you wrapped it in.

Here are various scenarios:

gulp.src(...)

    // if pluginA emits error, it's silently caught
    .pipe(continueOnError(pluginA()))

    // if pluginB emits error, it's silently caught
    // and prints to gutil.log
    .pipe(continueOnError(pluginB()))
        .on('error', gutil.log)

    // if pluginC emits error, then gulp crashes (error uncaught)
    .pipe(pluginC());

We should expect continueOnError to work as its name implies on pluginA() even if we don't add an error listener in gulpfile.js.

from gulp.

Note that continueOnError is not an inheritable behavior.

from gulp.

Well, then why not .on('error', gutil.log)? If it copy-paste snippet - then this will prevent some confusion.

from gulp.

That's what I would do. But we should consider edge-cases for when a user does not want to handle an error. It's more of 'process as much as you can' scenario.

from gulp.

Still, can be confusing, but nice work though.

from gulp.

@contra I think the way we stream files (or objects), it's sometimes useful to treat each file independently as it's piped down the stream. Error handling is largely affected by this notion. node.js's stream error handling, in its current state, doesn't support this.

When I picked up gulp the first time with little to no cursory understanding of streams, I had assumed each file was independent when being transformed in the stream. I don't doubt others may assume the same thing when picking up gulp the first time.

from gulp.

This is a bit crazy - but what if we implement "right" streams with "right" pipe? (ohmygodwhatiamsaying)

from gulp.

@floatdrop What do you mean?

from gulp.

@contra @dashed If I do not misunderstand problem - we have pipe that sends end events on error which will stop any implementation of map/through from receiving data.

from gulp.

That's right because of onerror function in https://github.com/joyent/node/blob/master/lib/stream.js#L91.

If we remove all instances of it for current stream, it looks good.

I reiterated continueOnError and came to the following that works:

var continueOnError = function(stream) {
    return stream
    .on('error', function(){})
    .on('newListener', function() {
        cleaner(this);
    });
};

Incidentally, I made a mistake of stripping the source stream of onerror. It should only strip the current stream of onerror listener functions.

Also, the reason for adding a dummy error listener is because if the current stream emits an error, node will check if there are any error listeners (onerror ins't counted). If there are none, gulp crashes.

from gulp.

I think we should reconsider error management in gulp here - https://github.com/gulpjs/gulp/wiki/Error-management-in-gulp

It is hard to reload such issue-threads after some time - so @dashed can you boil it up into wiki page?

from gulp.

@floatdrop You shouldn't put non-working code in the wiki. New users will go there and get confused when it doesn't work. That should go in a gist (or as a comment in this issue). The wiki is to help new people get ramped up on what currently exists.

from gulp.

Done - https://gist.github.com/floatdrop/8269868

from gulp.

I'd be interested in compiling a list of use cases for partial builds so we can determine if this is worth working on

from gulp.

@floatdrop Maybe the watch plugin can handle this a special way. Forcing this behavior on all plugins via monkeypatching core streams seems pretty rough

from gulp.

@contra For my use-case in particular for coffeescript, I have tasks that:

1. Compile all coffeescript files and put it into a folder (default task)
2. Watch any and all coffeescript files and compile them as necessary.

Task 2 follows after task 1.

If I run gulp and task 1 is stopped by a single file with invalid coffeescript with the error output in the terminal, I'd have to correct it. Once I corrected the file that stopped task 1, then task 2 would normally compile it.

But then, I have to stop gulp (via ctrl-c), then rerun it to check if there are any files with errors via task 1 and amend as necessary. This repetition may get annoying -- becomes a human factor consideration.

In this scenario, it's useful to see all errors in one shot in the terminal, and correct them accordingly. Partial builds in this manner becomes part of the development workflow.

I already have a work-around in my gulpfile that doesn't use the hack I proposed in this issue -- I'm just bringing this to attention because it may become a common potential misconception of working with streams.

from gulp.

@dashed Plugins where the error event is non-fatal should be able to disable the core behavior for just themselves. I don't think this belongs in gulp core other than documentation in the wiki so people don't get bitten by this

from gulp.

@contra Essentially the workaround comes to this type of pattern without hacks:

function doSomethingOnThisFile(target) {
    gulp.src(target)
        .pipe(...)
};

gulp.src(glob_target)
        .on('data', function(file) {
            doSomethingOnThisFile(file.path);
        });

This is probably the best resolution without being too extreme to achieve the result.

from gulp.

Closing this - use plumber + make sure plugins don't use map stream (or if they do, make sure they use the new error failure option)

from gulp.

I meet this problem when I use webpack-stream, just remove all default error handler.

see: https://github.com/floatdrop/gulp-plumber , and use it.

from gulp.

I'm using plumber and just wanted more human-friendly errors. Here's what I came up with:

  gulp.src(coffeeSrc)
    .pipe(plumber())
    .pipe(coffee({bare: true}).on('error', function (error) {
        console.error("Compilation error: " + error.stack);
      }))
    .pipe(gulp.dest('./css'))

Errors then render like this:

[15:27:16] Starting 'coffee:dev'...
[15:27:16] Finished 'coffee:dev' after 19 ms
Compilation error: C:\Path\theme.behaviors.coffee:3:19: error: unexpected =
    arkayoTypekit = {
                  ^

from gulp.