Skip to content
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

Fix gulp watch stopping on error. #293

Merged
merged 2 commits into from
Jun 25, 2015
Merged

Conversation

mariusc23
Copy link
Contributor

Fixes #90. Related to: gulpjs/gulp#259

I think recommending people to not return the gulp stream is not a good idea, because gulp will not know when the task is complete, therefore any tasks that depend on sass being done will not be guaranteed that (think race conditions).

@Keats
Copy link
Collaborator

Keats commented Jun 9, 2015

Wouldn't it be more consistent with the other plugins I've seen to use gutil.PluginError instead of log ? It should take care of that

@mariusc23
Copy link
Contributor Author

That's a good point. I haven't been using gulp that long so I forgot about gutil. I updated the pull request. Let me know what you think.

gutil.log(gutil.colors.red('[' + PLUGIN_NAME + '] ') + error.messageFormatted);
var message = new gutil.PluginError('sass', error.messageFormatted).toString();
process.stderr.write(message + '\n');
this.emit('end');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can just return a new gutil.PluginError looking at other gulp plugins using cb like https://github.com/wearefractal/gulp-coffee/blob/master/index.js#L38

Snugug added a commit that referenced this pull request Jun 25, 2015
Fix gulp watch stopping on error.
@Snugug Snugug merged commit 273e0a3 into dlmanning:master Jun 25, 2015
@Snugug
Copy link
Collaborator

Snugug commented Jun 25, 2015

@Keats Just tested, this.emit('end') is needed.

@ne0-cz
Copy link

ne0-cz commented Jul 5, 2015

This breaks my gulp setup - I'm not sure if I should open new issue, continue with discussion here or if I'm just using gulp-sass the wrong way.

The stopping of watch on error was a big issue for me, so I found a workaround:

.pipe(sass().on('error', function(err) {
    sass.logError(err);
    this.emit('end');
}))

This permitted me to have visual bell in console (console.log('\u0007');) on error which also greatly helps my current workflow.

My approach is broken by this commit, since this.emit('end'); in gulpSass.logError function has different this context depending on the way it's called (my way is not working, the usual .pipe(sass().on('error', sass.logError)) works).

I don't really understand javascript that much but since this line is the only usage of magic keyword this in the plugin I feel like this is not way to go - but then again I can't think of any better way since I'm new to the whole gulp/npm scene.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants