-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Continue gulp on error? #75
Comments
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. |
gulp.src('./**/*.coffee')
.pipe(coffee({bare: true})
.on('error', gutil.log)
.on('error', gutil.beep)
)
.pipe(gulp.dest(destDir)); should work |
Nesting I think it makes more sense to create new individual streams for each coffee file; much what I have. |
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. |
@dashed probably event-stream behavior. Will check it out |
I did a little introspection on gulp-coffee. It seems that for Not sure how to get around this. I think this would be critical for something like #13. |
@dashed where do you see that? I looked at it earlier and it seemed to keep going |
@contra I put together a testbed at: https://github.com/Dashed/gulp-coffee-sandbox to demonstrate the issue. Just There are four coffeescript files:
The file |
This is not issue with 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 With a little luck, you can find this lines that breaks your workflow. |
@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 |
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 The blog post was actually more informative than the event-stream docs. I'm unsure how to amend map-stream. |
Okay, I investigated the problem, and thats what i found: First - I was wrong about So when stream pipe'ing to another - nasty When it's clear what's happening - then you can go into 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. |
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: Here is a summary of my findings: IMO, doing //
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 if(fn.name == 'onerror') this.removeListener('error', fn); I tried removing 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 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 // 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 Note that |
continueOnError won't work due to #75 (comment) |
Seems like I overpatched map-stream somehow. Can you update wiki? |
Updated wiki. |
To note in this issue, the 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. |
Well, this is bad. |
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' |
Well, this is because you added |
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 |
Note that |
Well, then why not |
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. |
Still, can be confusing, but nice work though. |
@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. |
This is a bit crazy - but what if we implement "right" streams with "right" pipe? (ohmygodwhatiamsaying) |
@floatdrop What do you mean? |
That's right because of 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 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. |
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? |
@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. |
I'd be interested in compiling a list of use cases for partial builds so we can determine if this is worth working on |
@floatdrop Maybe the watch plugin can handle this a special way. Forcing this behavior on all plugins via monkeypatching core streams seems pretty rough |
@contra For my use-case in particular for coffeescript, I have tasks that:
Task 2 follows after task 1. If I run 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. |
@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 |
@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. |
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) |
I meet this problem when I use see: https://github.com/floatdrop/gulp-plumber , and use it. |
I'm using 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:
|
Isn't |
I have the following gulp js code using the gulp-coffee plugin:
Whenever gulp-coffee throws an error (e.g. invalid coffee syntax), it stops the rest of valid coffee files from 'piping' through; even though the error is caught.
To clarify this, here is a sample file queued via
gulp.src
:Is there a way to 'continue' the stream?
The text was updated successfully, but these errors were encountered: