Allow Sass compilation to restart after gulp dev errors

[colinrotherham]

This PR fixes an issue with our Sass compilation compile:scss Gulp task

1. Begin npm start
2. Make any Sass *.scss invalid, save it
3. Wait for gulp.watch() to kick in
4. Sass compilation compile:scss Gulp task shows error + hangs 🔥
5. Uh oh, have to cancel and run npm start again 😭

Development: we now .emit('end') for the Sass compiler to resume on next save
Production: we still .emit('error') for the Sass compiler to crash and break the build

Merge this PR and you can save, error, save, error, save, fix—all day long 😊

[colinrotherham]

Probably also comes under #2243

[romaricpascal]

Nice one, makes more sense to only build the docs if we could actually build the stylesheet 🙌🏻

[romaricpascal]

nitpick I see what this does, but it feels a bit narrow. If we'd introduce a new plugin or custom transform that'd emit error, we'd be back to square one.

Wondering if something like adding a final handling of error on the watch stream (found via this StackOverflow comment) could get us not to worry about task errors in dev long term? It may be an outdated solution though 😬

[colinrotherham]

Ah sorry @romaricpascal I've only taken this from the docs:
https://github.com/dlmanning/gulp-sass#render-your-css

Their approach suggests we swallow all errors with this.emit('end') inside the logger:

.pipe(sass().on('error', sass.logError))

This is perfect for development and doesn't stop gulp.watch()
This is awful for production as we no longer "break the build"

So I've gone one step better and emitted an error when not developing

Wondering if something like adding a final handling of error on the watch stream (found via this StackOverflow comment) could get us not to worry about task errors in dev long term? It may be an outdated solution though 😬

We'd lose Gulp's PluginError() flagging gulp-sass as the culprit (with fancy error output). Plus we'd lose per-plugin control of error.showProperties = false where we know the log formatter already includes this

Also if you use stream.on('error') to trigger stream.emit('error', error) we get an infinite loop

[romaricpascal]

Thanks for the pointer to the gulp-sass recommendation 😄

I think what I was looking for is a more systematic approach to handling transforms breaking, as for now it happens with Sass, but could happen on other transforms and the watch task would break again. It felt very gulp-sass-specific, hence my link to the example that turns error into end (not error so no looping) globally on the watch streams (so the build still breaks when not watching).

Similarly, the PluginError bit should ideally be part of any piping, imho. This way it won't just be Sass letting us know that it's gulp-sass breaking (though the error and stacktrace will likely already provide plenty to pinpoint things, no?)

If we're happy to revisit if a newer plugin breaks, then fine going with it.

[colinrotherham]

No worries, yeah I understand, it's good feedback

I've pushed again with the error handler moved into gulp-plumber so it listens for PostCSS errors too

Similarly, the PluginError bit should ideally be part of any piping, imho. This way it won't just be Sass letting us know that it's gulp-sass breaking (though the error and stacktrace will likely already provide plenty to pinpoint things, no?)

Sounds good, I'll nominate you for taking this further 😆

Hopefully we're good to get this first improvement PR in though?

[romaricpascal]

Neat! Yeah, using plumber to handle makes things more generic 🙌🏻 Shame Gulp doesn't have that built in 😢