Add 'failOnTypeErrors' option. Improve build result handling.

[chexxor]

Yo - I was quite confused using grunt-ts when first starting on adding TypeScript to my project. Why? I did grunt ts:build, and it said it failed due to type errors. This is odd because every JavaScript file should be able to pass through the TypeScript compiler. So, why was grunt-ts saying my JavaScript can't pass through the tsc compiler?

I did some looking, and it appears that the grunt-ts plugin has non-existent interpretation of the results of the tsc app.

 if (!result || result.code) {
    grunt.log.error('Compilation failed'.red);
 }

So, I decided to throw my hand at a slightly better interpretation of those tsc results. Maybe I'm totally wrong here, but I tested this change in my program, and I like what I see.

With this change, a syntax error will always fail the build, but only type errors will not. If we do want the build to fail on type errors, we can set the failGruntOnTypeErrors option to true in our Gruntfile.

What do you think?

Btw - you've got some nice code here! Really makes it a pleasure to dive in and help out.

[basarat]

What do you think?

Thanks! Please see comments inline. Cheers.

[Bartvds]

Good stuff.

+1 for failing on type errors by default. We use TypeScript to check types so if they have errors it should fail by default.

[jeffmay]

I'm not sure about the color green here. It implies that type errors are ok, when it can be the cause of the build failure if options.failGruntOnTypeErrors is false.

[jeffmay]

This is awesome! Thanks. I do share @basarat's concern about the default being to let type errors pass. I would rather the user consciously turn it off, as allowing the user to amass type errors early on is going to make adoption of TypeScript less beneficial.

[chexxor]

Thanks for the review!

It seems we need to clarify expected functionality of a compile tool.

Here's the issue I see: grunt-ts says the build failed, but it still created the JS files, and those JS files appear to be fine. If it created the JS files, then the compile is a success, in my eyes. Everything else is just a warning.

You already use grunt-ts in your workflow, and you are resistant to change, which I understand. But, in my eyes, grunt-ts is inconsistent in reporting compile failure, and so should be fixed. If a compile fails, this implies the resulting code should not work at all. If a build fails, that means something other than compilation went wrong.

Am I odd in this thinking? If so, can you link me to some reading material?

[Arnavion]

I cannot find any reference that the presence of "only level 2" errors counts as type errors. As I discussed with @chexxor on IRC, it might be better to look at the output files (target.out or target.outDir) to see if there's any output. If there is, then any errors must've only been type errors.

For reference, the error codes are defined in this file. From a cursory glance it does looks like everything under 3-9 is only type errors, but I'm not sure it's safe to rely on that unless the TS team says it is.

@chexxor 's use case is to ignore errors if he gets an output file (and it just so happens that type errors don't prevent an output file from being written).

I also agree that failGruntOnTypeErrors should be true by default. The practical reason is that setting it to false will "break" everyone's build scripts in the sense that what used to be build breaks will now not be, till they get around to modifying their grunt config to set it explicitly to true. The logical reason is that compile is a step of the build process, and if a step fails the whole process should fail.

[Bartvds]

I agree with @Arnavion on that if a build step fails the whole build is considered a failure. This is how grunt works (it bails on first error). Overrides are negotiable as option but certainly not a default.

Grunt will exit with a non-zero exit code and that indicates failure of the whole build. Any files that may be created are considered just cruft, similar as when your compilation would complete but your tests break.

Or when you generate some site (or whatever) and halfway something goes wrong. You may have some partial build products (that may even be runnable), but ultimately the process went red and you should consider the results faulty unless you explicitly allow it.

[chexxor]

Very good explanation. I totally see where you're coming from, and I agree with you. I'd like the compiler to demote type errors from error to warning, which isn't an option of the tsc command, it appears, so I'll have to do it in a build tool.

I'll make the changes that were suggested and let you know when that's done.

• Change option to failOnTypeErrors
• Default option to true
• Change success interpretation color to info color, not green
• Look at the output files, such as target.out or target.outDir, as a better indicator of compile success/failure. If they exist/have content/were updated, this implies that any errors must have been minor, perhaps only type errors.

[jeffmay]

• Change success interpretation color to info color, not green

Could we check the flag? Maybe we can log info if you have failOnTypeErrors set as false and log warning or error if the flag is set to true?

[chexxor]

I asked the TypeScript guys about the meanings behind the TSxxxx error codes. Here's their answer.

"The 1000s are syntax errors which will stop JavaScript emit from happening. 2000s are semantic errors that will still cause emit to occur. I believe 1000s and 5000s (compiler flag mis-use) should be the only ones stopping emit from occurring."

Maybe we should rename the property failIfNoEmit or something like that, as his answer is about emit/non-emit of JS code. Or we could add that as a separate property.

What do you think? Is it ok to base this on error codes for now?

[jeffmay]

I'm ok with basing it on error codes until there is a better way.

My only concern is that the default should not be to ignore type errors. Not everyone is migrating a code base. For people starting from scratch, type checking can prevent unperformant javascript code as well as other unforseen type mismatches from occuring. Making a clear comment about this feature in the readme about how it is useful for migration from js to ts would make it easy enough for adoption.

[basarat]

I'll make the changes that were suggested and let you know when that's done.

looking forward to it 👍

[chexxor]

I just committed a few more changes.

It appears I broke the build, but I don't know what the issue is - can anyone give me a tip?

/home/travis/build/grunt-ts/grunt-ts/test/fail/ts/deep/fail.ts(7,1): error TS2095: Could not find symbol 'Simplebad'.
/home/travis/build/grunt-ts/grunt-ts/test/fail/ts/deep/work.ts(11,21): error TS2000: Duplicate identifier 'main'. Additional locations:
/home/travis/build/grunt-ts/grunt-ts/test/fail/ts/deep/fail.ts(2,5)

[basarat]

@chexxor just a travis error (node0.8 needs to be retired ... let me push a fix so you can fetch from upstream). The fail task (/home/travis/build/grunt-ts/grunt-ts/test/fail) is designed to test a failure but wrapped in a continue so doesn't fail the build. https://github.com/grunt-ts/grunt-ts/blob/master/Gruntfile.js#L285 and not related to this build failure

[basarat]

Pushed 9693735 and build succeeded https://travis-ci.org/grunt-ts/grunt-ts/builds/26112209 (https://travis-ci.org/grunt-ts/grunt-ts/builds/26112209#L653)

[chexxor]

I rebased onto master, and the build seems to be passing now. thanks for the help @basarat !

[basarat]

👍 LGTM