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

crash the build process on error #295

Closed
louy opened this issue Feb 29, 2016 · 28 comments
Closed

crash the build process on error #295

louy opened this issue Feb 29, 2016 · 28 comments

Comments

@louy
Copy link

louy commented Feb 29, 2016

Expected behavior:
When a typescript error is detected, gulp shouldn't exit with code 0, an error should be reported instead.

Actual behavior:
errors are displayed but gulp exists with code 0.

Your gulpfile:

Include your gulpfile, or only the related task (with ts.createProject).

var tsProject = ts.createProject('tsconfig.json', {
  typescript: require('typescript'),
});
gulp.task('ts', function() {
  return gulp.src([
    'typings/**/*.d.ts',
    'server*.ts',
    'webpack.config.ts',
    'app/**/*.ts',
    'app/**/*.tsx',
  ], { base: __dirname })
    .pipe(sourcemaps.init({
      loadMaps: true,
    }))
    .pipe(ts(tsProject))
    .pipe(sourcemaps.write('.'))
    .pipe(gulp.dest('.'));
});

tsconfig.json

Include your tsconfig, if related to this issue.

{
  "compilerOptions": {
    "target": "es2015",
    "module": "commonjs",
    "jsx": "preserve",
    "inlineSourceMap": true,
    "moduleResolution": "node",
    "experimentalDecorators": true
  },
  "exclude": [
    "node_modules"
  ]
}
@ivogabe
Copy link
Owner

ivogabe commented Mar 1, 2016

Most users don't want this, since it would break their watch task. However, you can use something like this to exit with a specific code:

    ...
    .pipe(ts(tsProject))
    .on('error', function() { process.exit(1) })

@louy
Copy link
Author

louy commented Mar 1, 2016

I was hoping this would be a flag or something. This is a very common use case as it makes no sense to ignore errors in production builds/CI environment.

@ivogabe
Copy link
Owner

ivogabe commented Mar 2, 2016

I think that's more suitable to be implemented directly in gulp instead of gulp-typescript. Can you report it there? Other plugins could also benefit from this.

@ivogabe
Copy link
Owner

ivogabe commented Mar 2, 2016

This might be already implemented in gulp 4.

@louy
Copy link
Author

louy commented Mar 2, 2016

AFAIK gulp (current version) stops the task and crashes when an error occurs. This is the default behaviour and it was very annoying in watch tasks, but it's what should happen in production builds. for some reason gulp-typescript behaves differently.

@kruncher
Copy link

I added the following to my gulpfile.js and it seemed to work:

let failed = false;
...
  .pipe(ts(tsProject))
  .on("error", function () { failed = true; })
  .on("finish", function () { failed && process.exit(1); });

In my opinion this should not be integrated directly into gulp-typescript; but I do think that it would be a good idea to include a demonstration of this use case in the documentation.

@falsandtru
Copy link

simplified

  .once("error", function () {
    this.once("finish", () => process.exit(1));
  })

@kruncher
Copy link

@falsandtru does this simplified solution occur after ALL type errors have been listed?

When I tried something similar it aborted on the very first type error.

@falsandtru
Copy link

I got all errors before exit with errors.

...
[21:31:30] TypeScript: 5 semantic errors
[21:31:30] TypeScript: emit succeeded (with errors)

@kruncher
Copy link

@falsandtru awesome, thanks!

@brphelps
Copy link

The fix on package consumer side is really concise, which I'm happy about, but I will say that a lot of other linters and gulp plugins will come with an optional way to fail the task if "error conditions" were detected (like warnings / errors in a linter), which can then be opted into as the task implementer sees fit in their CI builds.

@wfortin
Copy link

wfortin commented Jun 27, 2016

+1 please add a way to integrate this directly in the plugin. I've made the fix suggested by @falsandtru in my projects but I would prefer to use a simple flag in the configuration of the gulp task. Thanks!

@navels
Copy link

navels commented Aug 31, 2016

gulp-jshint has a pretty good solution to this: a 'fail' reporter

stuff
  .pipe(jshint())
  .pipe(jshint.reporter('jshint-stylish'))
  .pipe(jshint.reporter('fail'))

@markstos
Copy link

I'm just adding that gulp-less has the same problem, so I agree it should be fixed upstream in the main gulp project. Because our less build step failed, the site was broken, but gulp still "succeeded", so the deploy succeeded.

I think developers would agree that while "breaking watch" is annoying, breaking production deploys because broken build steps "succeed" is a worse default behavior!

@danieloprado
Copy link

+1

@markstos
Copy link

markstos commented Nov 1, 2016

@danielmoore please use Github's "thumbs up" reaction feature instead of "+1" comments. Reactions are designed to be a better alternative for expressing support without the visual clutter and additional email notifications that "+1" comments bring.

@ulrichb
Copy link

ulrichb commented Dec 11, 2016

@ivogabe If we really want to keep the current behavior (opt-in non-zero exit code on errors), I think there should be a note about that on the README page (+ a hint for a workaround, e.g. this one).

That said, I would still prefer opt-out (for watch tasks).

@tsctao
Copy link

tsctao commented Mar 6, 2017

Here is my solution. It works fine for me, and it don't break watch!

Use async task because I want to end this task conditionally. I put error handler on tsResult to count the number of errors and finish handler after dest pipe to delete created files if there is any error and send a error to the callback to break gulp tasks, otherwise I call the callback without any argument to continue remaining tasks.

const gulp = require("gulp");
const del = require("del");
const ts = require("gulp-typescript");

gulp.task('compile', function (cb) {
    let errorCount = 0;
    const tsProject = ts.createProject("./tsconfig.json");
    const tsResult = gulp
        .src("./src/**/*.ts")
        .pipe(tsProject())
        .on("error", () => {
            errorCount += 1;
        });

    tsResult.js
        .pipe(gulp.dest("./build"))
        .on("finish", () => {
            if (errorCount > 0) {
                let error = new Error(`Failed to compile: ${errorCount} error(s).`);
                error['showStack'] = false;

                del("./build/**").then(() => {
                    cb(error);
                });
            } else {
                cb();
            }
        });

    return gulp.src('src/**/*.ts')
        .pipe(ts({
            noImplicitAny: true,
            out: 'output.js'
        }))
        .pipe(gulp.dest('built/local'));
});

@Madd0g
Copy link

Madd0g commented Jul 2, 2017

I'm using gulp-typescript in a node-server project, so every change runs a gulp-nodemon task. However an error in the typescript task still allows the nodemon task to run.

The solution posted above by @tsctao is the only solution I could find that worked.
But It only works because of the side-effect, the deletion of the build directory, so I get 2 errors instead of only the typescript error.

It seems to me like the conversation above is focused on exiting or not-exiting with an error exit-code, but in the nodemon case I don't want the process to quit, just for it to wait until a non-error result from typescript.

Is there a cleaner workaround?

EDIT: never mind, I switched out nodemon for this solution and it solved all my problems.

@SimonNodel-AI
Copy link

We recently had to deal with this issue as well. We chose to invoke a callback function of the task with an error message. This would look something like this:

gulp.task('ts', function(next) {
  return gulp.src([
    'typings/**/*.d.ts',
    'server*.ts',
    'webpack.config.ts',
    'app/**/*.ts',
    'app/**/*.tsx',
  ], { base: __dirname })
    .pipe(sourcemaps.init({
      loadMaps: true,
    }))
    .pipe(ts(tsProject))
    .on('error', next('Failed during typescript compilation'))
    .pipe(sourcemaps.write('.'))
    .pipe(gulp.dest('.'));
});

@raky291
Copy link

raky291 commented Feb 1, 2018

Most users don't want this, since it would break their watch task.

Hi @ivogabe If you don't want to break the watch task, then use this package (gulp-plumber), but please don't do it by default.

@hgezim
Copy link

hgezim commented Feb 1, 2018

@SimonNodel-AI Thank you for that. It works with node-mon as well.

@ivogabe
Copy link
Owner

ivogabe commented Feb 7, 2018

I still don't want to break this, especially given that the philosophy of TypeScript is that all errors are warnings, and there are no fatal errors. Though to make easier to crash the build, I'd suggest to add a new reporter (see readme) which will crash in case of errors. That way there is less code needed to configure this. Do you agree on this solution?

@gauntface
Copy link

cc @phated in case he as any points to add on how best to support failing builds while working with watch tasks.

@thedillonb
Copy link

Most users don't want this, since it would break their watch task.

@ivogabe I think you should reconsider breaking this for watch tasks. The default invocation of gulp-typescript should be conservative rather than permissive for a use-case (watch) that many people may not even utilize. Rather than new users failing to notice that errors are not failing gulp ("you don't know what you don't know") I would think it would be easier for users attempting to setup a watch task to fail and go looking for an answer.

@phated
Copy link

phated commented May 13, 2018

I believe gulp 4 no longer crashes if a watch task errors. We landed a fix for it at gulpjs/glob-watcher@ad96e3f (glob-watcher 5.0.1) so you should properly emit errors from your streams.

@ivogabe
Copy link
Owner

ivogabe commented Jun 5, 2018

@phated Great, I'll take a look at that!

@ivogabe
Copy link
Owner

ivogabe commented Jun 11, 2018

The crashing behavior will be part of our release 5 🎉! Thanks for everyones opinions, I tested the behavior of gulp 4 and the watch scenario works there. The changes with the current behavior are as follows:

  • gulp-typescript does not add a noop event listener to the error event. Errors will thus crash the build.
  • gulp-typescript emits only a single error. It appeared that the new watch behavior of gulp 4 only catches the first error. We thus emit a single error at the end of the compilation. To get notified of each separate error, one should use a reporter.
  • As pointed by @phated, gulp does not crash in watch mode.

To prevent crashing the build, even outside of watch mode, you can add .on('error', () => {}) to your stream, just after .pipe(tsProject()).

You can install the new version with npm install [email protected]

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

No branches or pull requests