Re-Evaluate supported TypeScript versions, open bugs in older versions

[phryneas]

I just executed the current test suite against all supported TypeScript versions. The results aren't too promising:
https://travis-ci.org/phryneas/fork-ts-checker-webpack-plugin/builds/522776393

Here's a collection of the failing tests:

TS 2.1

General:
1) should not mark warnings as errors when ignoreLintWarnings passed as option
2) should support custom resolution
3) should fix linting errors with tslintAutofix flag set to true
4) should not fix linting by default
5) should not find syntactic errors when checkSyntacticErrors is false
6) should find syntactic errors when checkSyntacticErrors is true
7) should find the same errors on multi-process mode
8) should only show errors matching paths specified in reportFiles when provided
9) should handle errors within the IncrementalChecker gracefully as diagnostic
Vue-Specific:
10) should get syntactic diagnostics from Vue program
11) should resolve src attribute but not report not found error
12) should be able to compile *.vue with each lang"before all" hook
13) should be able to detect errors in *.vue: "before all" hook
14) should resolve *.vue in the same way as TypeScript counter check - should report report one generic compilation error

TS 2.2

as TS 2.1

TS 2.3

General as above
Vue-Specific:
10) should resolve src attribute but not report not found error

TS 2.4

as TS 2.3

TS 2.5

General
1) should not mark warnings as errors when ignoreLintWarnings passed as option
Vue-Specific
2) should resolve src attribute but not report not found error

TS 2.6

General
1) should not mark warnings as errors when ignoreLintWarnings passed as option

TS 2.7

General (useIncrementalApi: true)
1) should not mark warnings as errors when ignoreLintWarnings passed as option
General (useIncrementalApi: false)
2) should not mark warnings as errors when ignoreLintWarnings passed as option

TS 2.8

as TS 2.7

TS 2.9

as TS 2.7

All TS 3.X versions seem to be working fine.

So the question is: how to continue from here on?
From looking at the tests, I would suggest to drop support for TS<2.5 and investigating the other two failing tests.
But of course, this might be something you know and expect and I'm just over-interpreting those results.

What do you think?

PS: please keep in mind, all these test were only run in the following configuration:
linux node@10 webpack@^4.0.0 ts-loader@^5.0.0 vue-loader@^15.2.4
Running other configurations might uncover more errors, but for now I kept it at that.

[phryneas]

So I tried hunting down this should not mark warnings as errors when ignoreLintWarnings passed as option test's error.

It boils down to the test finding an error that is not found with later TS versions:

[ { rawMessage:
     'ERROR TS2322: Type \'"1"\' is not assignable to type \'number\'.',
    message:
     '\u001b[1m\u001b[31mERROR in \u001b[39m\u001b[22m\u001b[1m\u001b[36m/home/weber/tmp/fork-ts-checker-webpack-plugin/test/integration/project/src/index.ts(2,7)\u001b[39m\u001b[22m\u001b[1m\u001b[31m:\u001b[39m\u001b[22m\n\u001b[90mTS2322: \u001b[39mType \'"1"\' is not assignable to type \'number\'.',
    location: { line: 2, character: 7 },
    file:
     '/home/weber/tmp/fork-ts-checker-webpack-plugin/test/integration/project/src/index.ts' } ]

Honestly, looking at the tests I would also expect to see this behaviour in TS >= 3.0 😩

Do you have any explanation? Did searching for source files somehow change with 3.0?

[johnnyreilly]

This is a very sad thing!

Honestly, looking at the tests I would also expect to see this behaviour in TS >= 3.0

When you mentioned that I remembered a change we made when we 1 point oh-ed: https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/master/CHANGELOG.md#v100

There are actually no breaking changes that we're aware of; users of 0.x fork-ts-checker-webpack-plugin should be be able to upgrade without any drama. Users of TypeScript 3+ may notice a performance improvement as by default the plugin now uses the incremental watch API in TypeScript. Should this prove problematic you can opt out of using it by supplying useTypescriptIncrementalApi: false.

You get different behaviour for error reporting with the incremental API: #198 (comment)

Essentially, the compiler starts reporting errors and stops checking for more of different kinds when using the incremental API. This may / may not explain the specific test you're talking about here.

With regards all tests running with useTypescriptIncrementalApi: true, I would expect near failures for TypeScript 2.9 and lower. The incremental API didn't arrive until TypeScript 2.7 (I think) and didn't really stabilise until around 2.9.

If you exclude incremental API tests running for < 3.0 how does that change the picture?

Also, arguably the should not mark warnings as errors when ignoreLintWarnings passed as option test could be improved. Instead of just checking for a number of failures, I think it would be a better test if it checked whether a specific error was not present. Kinda like this: https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/dfd8933f23da7ca72aa3a864e5811725146323d4/test/integration/index.spec.js#L363

[phryneas]

Unfortunately, this is not a useTypescriptIncrementalApi: true thing - I did not execute those tests for TS<2.7 and am getting the most errors for these versions nonetheless.

The error in questions occurs for both useTypescriptIncrementalApi: false and (in >=2.7) useTypescriptIncrementalApi: true in the same way :/

On the other hand, we have no additional failures with useTypescriptIncrementalApi: true, which is cool ;)

As for modifying the should not mark warnings as errors when ignoreLintWarnings passed as option test - yes, that could be done, and for that specific test that would be correct. But it reported something we would have missed otherwise: different behavious with different TS versions. So I want to at least understand what exactly changed here, to be able to decide if this is just "different behaviour" or a full-fledged bug that we are just seeing for the first time and that will bite us in the future..

[johnnyreilly]

So I want to at least understand what exactly changed here, to be able to decide if this is just "different behaviour" or a full-fledged bug that we are just seeing for the first time and that will bite us in the future..

Good shout.

[phryneas]

New finding:
https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/3990a135cb93e336ef0890061f6c360e7efc2f4d/test/integration/index.spec.js#L178

All tests running helpers.testLintAutoFixTest are not passing the useTypescriptIncrementalApi option at all, so for TS>=3.0, these tests are always running with the default value of useTypescriptIncrementalApi: true, even if the tests that should be running useTypescriptIncrementalApi: false.

That's the only reason these tests are not failing.

So yes, this seems to be a different behaviour between useTypescriptIncrementalApi true|false, nothing to do with TS 2 or 3.

[johnnyreilly]

All tests running helpers.testLintAutoFixTest are not passing the useTypescriptIncrementalApi option

How many are in that camp? [hands over eyes]

[phryneas]

Just four. Assuming I haven't missed anything else.

[johnnyreilly]

cool - which ones are they? are they all tslint related?

[phryneas]

• "should not print warnings when ignoreLintWarnings passed as option"
• "should not mark warnings as errors when ignoreLintWarnings passed as option"
• "should fix linting errors with tslintAutofix flag set to true"
• "should not fix linting by default"

so yes, all linting-related.

[phryneas]

But I think I found something far more concerning.

I just copied the files from the test over to a copy of https://github.com/TypeStrong/ts-loader/tree/master/examples/fork-ts-checker-webpack-plugin

In useTypescriptIncrementalApi: true mode I'm not getting any errors even though the files themselves are full of errors.

I'll make a production repo, maybe you can tell me what's going on there...

[phryneas]

You can check out this branch.
https://github.com/phryneas/fork-ts-checker-webpack-plugin/tree/reproduce-useTypescriptIncrementalApi-missing-errors

yarn start and yarn build show no errors, even though there is one right in index.ts.
Turning on checkSyntacticErrors finds errors though - but still misses the semantic error in index.ts.

[johnnyreilly]

Just saw your thing - the below was made by using the ts-loader example

[phryneas]

Yup, it's an edge case with the test suite here.

As long as that syntactic error exists there, the ApiIIncrementalChecker bails on that syntactic error. Due to a syntactic error being present, the semantic errors are never found.
And as semantic errors are not being reported per default, fork-ts-checker-webpack-plugin happily reports

No type errors found
No lint errors found