Change failOnError parameter to play nice with webpack/webpack-dev-server

[nickgcattaneo]

Hi,

I'm currently utilizing webpack-dev-server & webpack-hot-middleware to marry with my own assets/express server/build processes/etc. I recently decided to add your plugin to my build process and one of the keys I noticed right away was "failOnError". I took a look through your source and noticed that when this parameter is present, you are piping an Error() object to webpack (https://github.com/JaKXz/stylelint-webpack-plugin/blob/master/lib/run-compilation.js#L35). This seems to work nicely for running webpack in a single build on its own (as failing the webpack build outright isn't an issue), but when we want webpack to pause building (but still listen for file changes and rebuild, which is a feature of webpack-dev-server), this failOnError key will actually stop webpack entirely (e.g. the build will no longer listen to file changes/attempt to rebuild/etc and we then have to restart the entire webpack bundling process/etc).

I found this behavior strange, because I use eslint-loader and it's failOnError key doesn't throw an error necessarily; it gracefully halts the webpack build process and still allows webpack to listen for file changes/etc and rebuild accordingly. I believe the reason is due to how eslint-loader pipes its response back to webpack. In particular, it looks like rather than throwing an error, it gets the current webpack instance and utilizes the emitError function under webpack (https://github.com/MoOx/eslint-loader/blob/master/index.js#L105).

I'd be curious to hear your thoughts and if I'm following this correctly.

I would be happy to put a pull request in as well to implement the error handling similar to how eslint-loader does it.

Thanks!

[JaKXz]

@nickgcattaneo thank you for the report. I've been wondering about this for a while as well, because the only really uncovered part of the code in this plugin is the watch run calls... It's a good point that the watch runs are not exactly the same especially when errors are thrown.

A rudimentary solution in my mind would be just binding another argument to the runCompilation function on watch runs, and not use an Error constructor in that case. Obviously / however, I am not enthusiastic about that solution which is why it's not in the source code ATM...

I would love to see a PR with the eslint approach! :) And a great stretch goal would be to mock out the watch runner mode somehow, perhaps with testdouble and get some coverage on this behaviour. I look forward to seeing what you come up with 👍

[nickgcattaneo]

Great, thanks for the feedback. This is tied to a production build I'm utilizing, so I plan to look into it this week. Otherwise I will be out of town all next week for 4th of july.

[nickgcattaneo]

Sorry for the delay in getting back here - finally got some time to toy around with the source and spent some time rewriting a portion of the runCompilation method. In particular, there are two changes I'm proposing:

• Update the emitErrors and failOnError descriptions (see below).
  • emitErrors: Pipe stylelint 'error' severity messages to the error message handler in webpack's current instance. Note when this property is disabled (false) all stylelint messages are piped to webpack's warning message handler. Default: true
  • failOnError: Throw a fatal error in the global build process (e.g. kill your entire build process on any stylelint 'error' severity message). Default: false
• Update the event hooks to use either 'after-emit' or 'after-compile'. 'after-emit' should be used when emitErrors is true, and 'after-compile' when emitErrors is false.

'use strict';

var R = require('ramda');
var linter = require('./linter');
var errorMessage = require('./constants').errorMessage;

/**
 * Function bound to the plugin `apply` method to run the linter and report any
 * errors (and their source file locations)
 * @param options - from the apply method, the options passed in
 * @param compiler - the compiler object
 * @param done - webpack callback
 */
module.exports = function runCompilation (options, compiler, done) {
  var errors = [];
  var warnings = [];
  var compilerMethod = options.emitErrors ? 'after-compile' : 'after-emit';

  linter(options)
    .then(function linterSuccess (lint) {
      var results = lint.results;

      if (options.emitErrors === false) {
        warnings = results.filter(R.either(fileHasErrors, fileHasWarnings));
      } else {
        warnings = results.filter(R.both(R.complement(fileHasErrors), fileHasWarnings));
        errors = results.filter(fileHasErrors);
      }

      if (options.quiet === false) console.warn(options.formatter(results));
      if (options.failOnError && errors.length) done(new Error(errorMessage));
      else done();
    }).catch(done);

  compiler.plugin(compilerMethod, function(compilation, next) {
    if (warnings.length) {
      compilation.warnings.push(new Error(options.formatter(warnings)));
      warnings = [];
    }

    if (errors.length) {
      compilation.errors.push(new Error(options.formatter(errors)));
      errors = [];
    }

    next();
  });
};

function fileHasErrors (file) {
  return file.errored;
}

function fileHasWarnings (file) {
  return file.warnings && file.warnings.length;
}

Before I write test cases or pushed this any farther I wanted to get your buy-in/thoughts on the above.

Thanks!

edit: @JaKXz fyi :D