Plugin doesn't fail build

[SBoudrias]

Even if JSHint returns error, the plugins continue without errors.

It'd be best if it returned an error when JSHint fail in order to break a build.

[yocontra]

Agreed - will have this done soon

[yocontra]

Just waiting to finish gulp-util PrettyError to make stuff like this look a little better. This will cause peoples stuff to start exiting on error (unless we add a --force)

[robrich]

I agree it should fail the build, but if it just piped an error into the stream, the build would fail before the reporter got a crack at it. Should it fail on('end',.. instead?

[yocontra]

@robrich What do you guys think about this behavior being off by default (since a jshint error is not really a fatal error) and having a flag that enables this?

[robrich]

I'd say it's in the eyes of the beholder how fatal the error is, and I'd lean more towards on by default. If your npm test is mocha && jshint then you wanted to die on lint errors. Switch to gulp-jshint and you'd expect the same.

[yocontra]

What should the error message look like? It won't have access to any reporters

[jakobwesthoff]

In my opinion jshint.reporter should have the corresponding fail configuration, which allows the stream to fail after all errors have been reported. Maybe on end?. Otherwise the whole linting is kind of useless, as one wants to know which is broken in order to fix it. It isn't enough to know something is broken somewhere.

Furthermore, there might be good reasons to habe jslint process all the files in the stream and after that apply some other custom stream to it, which simply uses the generated .jshint information on every file for further processing.

While I am thinking about it. Why not have a jshint.fail() stream, Which simply fails the build if there is at least one unsuccessfully linted file in the stream?

gulp.src("src/**/*.js")
  .pipe(jshint())
  .pipe(jshint.reporter("default"))
  .pipe(jshint.failOnError());

Something like the above would provide a maximum flexibility, while still adhering to the separation of concerns idea, which are one of the reasons streams are used. Right?

[robrich]

I really like the separation of three tasks: running the linting, reporting on the linting, and failing the build. It seems like a lot of ceremony for typical users though. Adding "fail" to the reporter is a wonderful idea (as I really do want the reporter to run before I fail the build), but all built-in reporters don't do this. (They also log not to events or to the logger of my choice but directly to stdout.) I think because of this, bundling fail with reporting would ultimately not gain traction.

[SBoudrias]

But, failing a build might be a task Gulp should handle.

For example, Gulp-JSHint run linting on each task. Once all files have passed through (es.through), it log the reports. If any linting fails, it just notify Gulp process (Gulp.failBuild() or anything).

So process continue and other task can be runned like unit tests and such.

Once all the streams are done, Gulp either exit 0 with success, or then exit 1 logging all tasks who've failed, e.g.

Gulp: Build failed (0/3)
  jshint: Linting errors
  mocha: Tests error
  uglifyjs: Unexpected error

[jakobwesthoff]

Even though it might be a little confusing for the typical user at first, the three different streams would reflect the whole architecture of Gulp best, I think. Once users understand the architectural idea behind the system, they should easily understand what's going on there. For convenience reasons jshint.reporter could provide a fail option, which automatically chains the proposed jshint.failOnError stream. This way both ways would be possible.

I don't see a problem with the reporters not having the capability to fail the build, as it is the reporter-stream here, which should take care of doing this.

[jakobwesthoff]

@SBoudrias I agree with you maybe failing builds should be handled in a way, which better reflects the stream based architecture of gulp. But I think this belongs into another more general Issue on the Gulp repository itself, as it concerns more than only this plugin.

[robrich]

That's intriguing: like a "fail the build" flag in gulp that tasks can set that doesn't crash out immediately but rather waits for all orchestrations to finish then crashes. I agree, pitch this as an idea on gulp, and we'll implement it in orchestrator.

[SBoudrias]

Opened an issue on Gulp about this: gulpjs/gulp#113

[khilnani]

Also see sindresorhus/jshint-stylish#6 for a temporary fix

[yocontra]

Working on this now. This is where I'm going with it

gulp.src("src/**/*.js")
  .pipe(jshint())
  .pipe(jshint.reporter("default"))
  .pipe(jshint.reporter("fail"));

I'm just going to include a custom reporter that fails on error

[yocontra]

This is out in 1.4 - let me know if this doesn't work out for everyone

[SBoudrias]

Works for me, but it throws as soon as an error is encountered. I feel it'd be better if it only throwed once every files passed through; this way we get a full report.

[laurelnaiad]

+1 for getting a full report and then erroring as another option

[yeehaa123]

Still does not work for me. This returns a status code 0.

gulp.task('lintJS', function(){
  return gulp.src('./src/app/**/*.js')
                   .pipe(jshint('.jshintrc'))
                   .pipe(jshint.reporter(stylish))
                   .pipe(jshint.reporter("fail"));
});

It works when I leave off the return, but that does not seem right to me...

Can anyone help me out?

Cheers,

JH

[chmontgomery]

+1 for full report and erroring at the end.

[nelsonpecora]

As of 1.5.4 it works if I have a return statement, but doesn't work if I leave it off (the opposite of @yeehaa123's issue). Also, by "works" I mean "it stops the other things from executing" but it displays the entire error stack.

[chmontgomery]

I noticed similar behavior to @yeehaa123's issue. The way I've ultimately solved this problem is to watch for errors manually, then process.exit(0):

  var exitCode = 0,
    totalLintErrors = 0;

  process.on('exit', function () {
    process.nextTick(function () {
      var msg = "gulp '" + gulp.seq + "' failed";
      console.log(gutil.colors.red(msg));
      process.exit(exitCode);
    });
  });

  function lintOnEnd() {
    var errString = totalLintErrors + '';
    if (exitCode) {
      console.log(gutil.colors.magenta(errString), 'errors\n');
      gutil.beep();
    }
  }

  gulp.task('lint', function () {
    return gulp.src([
      './*.js',
      './lib/**/*.js',
      './test/**/*.js'
    ])
      .pipe(jshint())
      .pipe(jshint.reporter(stylish))
      .pipe(map(function (file, cb) {
        if (!file.jshint.success) {
          totalLintErrors += file.jshint.results.length;
          exitCode = 1;
        }
        cb(null, file);
      }))
      .on('end', function () {
        lintOnEnd();
        if (exitCode) {
          process.emit('exit');
        }
      });
  });

It feels a bit brute force but I get the advantage of tracking all jshint errors, across all files, in one complete report before exiting:

[~/Projects/tesla.lib.core]$ gulp lint
[gulp] Using gulpfile /Users/montgomeryc/Projects/tesla.lib.core/gulpfile.js
[gulp] Starting 'lint'...

/Users/montgomeryc/Projects/tesla.lib.core/lib/service/core/property_service.js
  line 135  col 5  The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
  line 125  col 3  The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.

✖ 2 problems


/Users/montgomeryc/Projects/tesla.lib.core/test/service/inject/dependency-tree-test.js
  line 17  col 5  The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.

✖ 1 problem

3 errors

[gulp] Finished 'lint' after 1.04 s
gulp 'lint' failed

I also created a slight variation, lint-watch, used by my watch task that does not process.exit and just spits out errors to the console and continues to watch for file changes.

[Aaronius]

As of 1.6.1 I'm still getting an exit code of 0. My config is as follows:

gulp.task('lint', function() {
  return gulp.src(['*.js', 'client/scripts/**/*.js', 'client/instruments/**/*.js', 'server/**/*.js'])
    .pipe(jshint())
    .pipe(jshint.reporter('jshint-stylish'))
    .pipe(jshint.reporter('fail'));
});

Output:

bash-3.2$ gulp lint; echo $?
[gulp] Using gulpfile ~/dev/totem/gulpfile.js
[gulp] Starting 'lint'...

/Users/aahardy/dev/totem/client/scripts/dashboard/controllers/dashboard-edit-ctrl.js
  line 12  col 44  This function has too many parameters. (9)

✖ 1 problem

0

[analog-nico]

FYI @Aaronius: I have just about the same code as you do. With 1.6.4 it now returns 1.

[yocontra]

Exit codes should work fine now with the latest gulp (global and local) and the latest gulp-jshint

[Aaronius]

Confirmed. Thank you!

[luckylooke]

@chmontgomery I have used your code, but it says: map is not defined, missing something? Thanks ;)

Finally, I have found better fail reporting here:
https://github.com/mikaelbr/gulp-notify#as-jshint-reporter

[chmontgomery]

@luckylooke I'm missing the require statements in my example. map refers to dominictarr's map-stream

[luckylooke]

@chmontgomery Thank you, could you edit the post for others, just put

var map = require('map-stream'); 

at the begining. It can save someones time I think :)

[yocontra]

@luckylooke You don't have to use his example code because the bug was fixed