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

webpack exit code is equal to 0, if typescript build fails #108

Closed
marcelkottmann opened this issue Nov 18, 2015 · 46 comments
Closed

webpack exit code is equal to 0, if typescript build fails #108

marcelkottmann opened this issue Nov 18, 2015 · 46 comments
Labels

Comments

@marcelkottmann
Copy link

When the build fails (i.e. at least one typescript compile error occured), the ts-loader emits an incomplete javascript file and does not propagate the build failure to webpack. When using webpack with the --bail command line option an compile error, should result in an webpack exit code not equal to zero.

Is there another way or option to check if a compile error occured.

@mpseidel
Copy link

+1 - this would be really helpful

@PePe79 theoretically you could use the --noEmitOnError typescript option to get an exit code > 0 but then you only get something like this as an error message ModuleBuildError: Module build failed: Error: Typescript emitted no output for .../file.ts so none of the helpful ts compiler errors.

@marcelkottmann
Copy link
Author

yes, i tried this already before, but this error message is not really useful.

https://github.com/denvned/webpack-typescript does fail with exit code > 0 on compile error with webpack command line option --bail

But this loader has other issues, so that's not an alternative for us and i would like to go on with ts-loader. But this bug is a blocker for our automated builds.

@jbrantly
Copy link
Member

Yea, definitely know the need is there. Have you tried noEmitOnError (even though there are possibly issues with that - #91)?

@jbrantly
Copy link
Member

Wow. I didn't get a lot of sleep and missed your comment on noEmitOnError. Sorry.

@denvned
Copy link

denvned commented Nov 18, 2015

@PePe79, I would be happy to resolve any issues you have with webpack-typescript: https://github.com/denvned/webpack-typescript/issues

@MarkusKramer
Copy link

+1 would be really great if this was fixed. as incomplete error messages are obviously a big problem.

@EvAlex
Copy link

EvAlex commented Jan 21, 2016

👍 unbeleivable! That's a blocker issue for using with build server!

@EvAlex
Copy link

EvAlex commented Jan 21, 2016

@denvned, your loader has the same issue

@EvAlex
Copy link

EvAlex commented Jan 21, 2016

Oh yeah, I've managed to fix it. I will create Pull Request and attach explaination there in 6-8 hours.

@raybooysen
Copy link

or @EvAlex @denvned @jbrantly combine to have a single loader, so we don't have all these disparate loaders. :)

@jbrantly
Copy link
Member

Always open to PRs. FWIW, I think the correct solution to this is to fix #91 to be more useful and then combine that with --bail for the webpack exit code, but since I haven't actually dug in and tried it yet I don't know if there are problems with that approach.

EvAlex added a commit to EvAlex/ts-loader that referenced this issue Jan 21, 2016
@vladimir-rovensky
Copy link

FWIW until this is fixed you can use something like webpack-fail-plugin to make your build fail on error.

@jbrantly
Copy link
Member

OK, I've looked at this in more detail and here is where I've arrived:

According to this, webpack2 should fix the exit code. They are not making the change to webpack1 for backward-compatibility reasons.

I could make a change to ts-loader that would change the exit code in the case of failure. However, I feel like there are some issues with that, namely that a single loader would be taking on the responsibility of changing the exit code when there could be a lot of moving parts in the webpack config. For that reason it would at the very least need to be behind a flag. I could default the flag to "on" which would be a breaking change, and possibly could cause confusion when someone is trying to figure out why their build is all of a sudden failing. Or I could default to "off", which means you fine people looking for this functionality would have to configure it to "on" anyways.

It seems to me a better solution is to recommend the webpack-fail-plugin suggested by @vladimir-rovensky. The source for that plugin seems completely reasonable, and I just tried it myself and it gets the job done. It's not that much more work to use that plugin than it would be to turn on a flag in ts-loader. All TypeScript errors are still printed without any changes to ts-loader. And when webpack2 hits the problem just goes away.

If this seems reasonable I'll add some documentation pointing people to this solution.

@jbrantly
Copy link
Member

FWIW, it was as simple as adding this to my webpack.config.js

plugins: [require('webpack-fail-plugin')]

@marcelkottmann
Copy link
Author

Works like a charm!

@jbrantly
Copy link
Member

This has been added to the docs. Thanks @johnnyreilly!

@seansfkelley
Copy link

I'm a little confused by the reasoning here. Specifically:

However, I feel like there are some issues with that, namely that a single loader would be taking on the responsibility of changing the exit code when there could be a lot of moving parts in the webpack config.

If I make a mistake that causes a compilation error, I would expect the build failure. I don't understand why it would be inappropriate for the build system to report this error via the standard mechanism.

I could default the flag to "on" which would be a breaking change, and possibly could cause confusion when someone is trying to figure out why their build is all of a sudden failing.

If anyone is actually surprised by this, something is very, very wrong. Why was it okay that their build used to be failing but now that it's reported as such it's bad? Build failures should be loud. If I want to hard-fail the Webpack process, it provides a --bail option for me to specify it. It's not the job of a loader to decide how failures should be interpreted, just that failures occurred.

webpack-fail-plugin may work, but it's kind of a sketchy solution, given that (1) it already has some pretty obvious flaws and (2) uses an API for something it's not really intended for.

In short, I think this is the responsibility of ts-loader to report errors like this, and it's the responsibility of webpack and the user to decide what to do with the errors. The two should not be conflated.

@jbrantly
Copy link
Member

If I want to hard-fail the Webpack process, it provides a --bail option for me to specify it. It's not the job of a loader to decide how failures should be interpreted, just that failures occurred.

I agree, which is why I didn't want to add it to the loader. I think you might be misunderstanding the issue here, which is that ts-loader is reporting errors just fine to webpack, but that webpack does not set the exit code if there are errors. You literally linked to the issue for this. If I were to attempt fixing this I would be putting process.exit(1) somewhere in the code.

There are two ways to report errors to webpack in a loader. There is the emitError API and there is "throw an exception". Right now ts-loader only throws an exception if TypeScript literally could not produce any output. For TypeScript diagnostics ts-loader uses emitError. I believe this is the appropriate usage of the API that is provided to loaders.

In webpack 1.x, if you use the "throw an exception" route and you use --bail, then webpack will report a non-zero exit code. Otherwise it doesn't. I think this is an issue, but it's on webpack, not on ts-loader.

ts-loader could potentially take the "throw an exception" route for all errors, but that has its own problems I believe (I haven't tried that in awhile so my memory is a little fuzzy). I don't recall if errors using emitError would still be reported. You'd also still have to use --bail which means you'd only get the error(s) from the first file, but that might not actually be the source of the issue.

@jbrantly
Copy link
Member

I would also like to point out that I don't believe --bail should equal "report error code". I'll link to the relevant comment on this. Error codes should be reported regardless of whether or not --bail was used, and unfortunately I don't know if that's going to be the case in webpack2.

@jbrantly
Copy link
Member

Actually, sorry, one last bit on this. I decided to go see if maybe webpack2 did change this and it looks like it may have. I haven't actually tested webpack2 but this code seems to cover the case above:

        if(!options.doWatch && stats.hasErrors()) {
            process.on("exit", function() {
                process.exit(2); // eslint-disable-line
            });
        }

@seansfkelley
Copy link

Point taken on --bail. However, the readme is still confusing. It seems to state that ts-loader purposefully doesn't propagate errors, but as you explain here it's actually Webpack that is mishandling the errors that ts-loader is reporting via emitError (aside: I don't see any references to emitError anywhere). Perhaps rephrasing it and linking to that issue would help clarify what's actually going on here.

@jbrantly
Copy link
Member

I can see what you mean:

ts-loader does not propagate the build failure to webpack

That should be updated.

Re: emitError, it's because ts-loader doesn't call that API directly but instead goes to the internals of webpack to add the errors (the same as if you were to call emitError). The reason is that ts-loader doesn't add the errors until after the loader has ran for all files, at which point you no longer actually have access to the emitError API directly.

@vidartf
Copy link

vidartf commented Nov 16, 2016

ts-loader does not propagate the build failure to webpack

That should be updated.

I see this text is still in the readme. Does anyone have a suggestion for a replacement text?

@johnnyreilly
Copy link
Member

Sorry I don't understand the question. There is detail on how to make the build failure propogate here:

https://github.com/TypeStrong/ts-loader#failing-the-build-on-typescript-compilation-error

@vidartf
Copy link

vidartf commented Nov 16, 2016

As far as I understood the discussion above, ts-loader does propagate the build failure to webpack, but webpack doesn't actually fail when it receives an error ?

@vidartf
Copy link

vidartf commented Nov 16, 2016

Other than that, since there has been a recent push to cooperate with ATL, it might be worth mentioning that I really enjoy ATL's printing of the error text on a failed compilation.

@jaybekster
Copy link

Hello guys!
Could you please tell me if the problem is solved?
I've tried many ways to provide ts-loader errors to webpack error callback, but it still doesnt work.
I want to provide ts-loader readable error toerror in compiler.run(function(err, stats) { argument

@johnnyreilly
Copy link
Member

Have you tried the instructions in the readme?

@jaybekster
Copy link

jaybekster commented Nov 18, 2016

Yes. I've done following actions:

  1. added "emitError": true to tsconfig.
  2. added bail: true to webpack.config
  3. added require('webpack-fail-plugin') into plugins to webpack.config

But when build is broken by ts-loader, it shows me unreadable error.

I wanna see an error like if emitError is disabled.

@scorchio
Copy link

scorchio commented Dec 9, 2016

I would be interested in this as well, having a readable error message would be much better.

@roryprimrose
Copy link

roryprimrose commented Dec 15, 2016

I can get the build to fail which is what this issue addresses, but like the others, I can't get meaningful information from the failure.

@johnnyreilly
Copy link
Member

Take a look at our example: https://github.com/TypeStrong/ts-loader/tree/master/examples/webpack1-gulp-react-flux-babel-karma

Production mode uses fail mode

giogonzo added a commit to buildo/scriptoni that referenced this issue Jul 8, 2017
reading docs and some issue discussions, this should be the default
behavior with webpack >= 2, but it doesn't seem to work without adding
webpack-fail-plugin
See e.g. TypeStrong/ts-loader#108
@gabro
Copy link

gabro commented Jul 10, 2017

Hey @johnnyreilly is this issue still actual for modern versions of TypeScript?

I tried to use the --noEmitOnError option of tsc (2.3+) and it appears to exit with code 1 and show meaningful errors.

@johnnyreilly
Copy link
Member

I haven't tested lately and so I'm not sure.... Perhaps it's changed?

@gabro
Copy link

gabro commented Jul 10, 2017

It seems like an option, this was discussed more than a year ago.
What would be a quick way of testing it? Despite the readme, I've been recently surprised by this and accidentally deployed a broken bundle (in dev, thankfully)

@johnnyreilly
Copy link
Member

I've just noticed that you said:

I tried to use the --noEmitOnError option of tsc (2.3+) and it appears to exit with code 1 and show meaningful errors.

You probably don't want to use this as it massively slows down compilation (can't emit until there are no errors at all)

My guess is you still need to use the fail plugin to fail the build. I still am...

@gabro
Copy link

gabro commented Jul 10, 2017

You're talking about incremental builds, right?

@johnnyreilly
Copy link
Member

johnnyreilly commented Jul 10, 2017

No - production builds. Errors show up just fine in incremental

@gabro
Copy link

gabro commented Jul 10, 2017

Then I'm not sure I follow. Can you elaborate on how not emitting on errors slows down the build?

@johnnyreilly
Copy link
Member

Sorry - I had my wires crossed there. Here's my workflow:

  • I develop in watch mode. Here I should notice errors either in the console or as a result of the error toast notification I have set up.
  • Our CI server runs the full build. Here the fail plugin will fail the build if there are any errors.

What are you trying to do?

@gabro
Copy link

gabro commented Jul 10, 2017

Same setup. However I initially missed the fail plugin note on the readme and the CI deployed a broken bundle that didn't make the build fail.

I read this thread trying to understand why ts-loader doesn't propagate TS errors to webpack, and I stumbled upon the issue about --noEmitError not providing useful messages.
That doesn't seem to be the case with recent versions of TS.

I couldn't quite understand if there's other reasons of why errors are not propagated.

@johnnyreilly
Copy link
Member

Well if you have the fail plugin in place now then you should be good. I can't actually remember anymore the details of why I'm afraid.... @jbrantly may be able to advise.

@gabro
Copy link

gabro commented Jul 10, 2017

Yes, my CI is happy now.

However, I suspect I'm not and I won't be the only one being bitten by this.

Having the loader to propagate the errors to webpack is a reasonable expectation to have, so I'd like to understand what is the blocking issue.

@johnnyreilly
Copy link
Member

I think this is more of a webpack rather than a ts-loader issue. I agree it's a reasonable expectation to have. There's more context here: webpack/webpack#708

@gabro
Copy link

gabro commented Jul 10, 2017

Ah I see, thanks for the pointer.

gabro added a commit to gabro/webpack-fail-plugin that referenced this issue Jul 10, 2017
Despite the changes in webpack 2, this plugin is still needed in some instances (see for example the discussion here: TypeStrong/ts-loader#108).
@Lazarencjusz
Copy link

Lazarencjusz commented Feb 16, 2018

Unfortunately, in my case, when I want to use thread-loader or happypack the webpack-fail-plugin not works (even I use new ForkTsCheckerWebpackPlugin({ checkSyntacticErrors: true }) plugin).

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

No branches or pull requests