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

[johnnyreilly]

okay cool. Heart attack over 😄

[phryneas]

So having the syntactic error there essentially nullifies all other tests.

Also, this might be highly confusing in production. In this specific case, ts-loader never reports the error in syntacticError.ts because it never is processed by webpack (it is not referenced anywhere), but fork-ts-checker-webpack-plugin thinks ts-loader reported an error and outputs No type errors found.

[phryneas]

okay cool. Heart attack over smile

Yup, had the same heart attack until I re-stumbled over this by accident by reading the README xD.

But this is still concerning - the case I described above might really impact real-life projects out there.

Just go to your project, create a rogue error.ts with the contents const array = [{} {}];.
As long as that file is not imported from another file, you will not get any more error reports.

[johnnyreilly]

Yes - granted. All suggestions to help fix this are welcome 🤗

I'm not certain what the best way forward here is.

[phryneas]

Hmm. Off the top of my head, I can think of three things:

• even when checkSyntacticErrors === false, instead of emitting No type errors found, emit something like There are semantic errors that block type checking. Please correct the errors in ${fileName} to get type checking again
• override typescript.emitFilesAndReportErrors to continue checking after syntactic errors were found. Seeing that this method has been edited multiple times in the past year, that might end up in a cat-and-mouse game and not be a feasible option.
• Override program.getSyntacticDiagnostics() to always return an empty array if checkSyntacticErrors === false. I think this might be the way to go.

[johnnyreilly]

In the scenario you discuss above I think you're suggesting that ts-loader won't report the syntactic error and fail the build. Is that right? If so that's disappointing.

Options 1 and 3 sound interesting. Simplicity of use remains a strong motivation of the plugin. I'd be curious to hear you talk through the developer experience of using each.

(Bearing in mind that the typical user of the plugin won't know that there is a distinction between semantic / syntactic errors I'm keen any solution is easy to consume.)

Another thing to add into the mix is the default way people use the plugin. At present, and this is a result of the way the plugin evolved, error checking is curiously split across ts-loader and fork-ts-checker-webpack-plugin. ts-loader does syntactic errors by default, fork-ts-checker-webpack-plugin does semantic by default.

A possible other option is changing the default behaviour so the plugin checks for both types of error. And change the advice in ts-loader to not report errors at all when working with the fork-ts-checker-webpack-plugin. Aka "happypack" mode: /~https://github.com/TypeStrong/ts-loader/blob/master/README.md#happypackmode-boolean-defaultfalse

If we follow that route we might want to give some more meaningful names for options. 😁

All things to consider. Thanks for your efforts - I dig it 🤗

[phryneas]

No, ts-loader will not fail the build, as it is a syntactic error that does not affect the build at all. (It is not directly referenced in webpack and not imported by any file directly referenced by webpack). The build will succeed without any errors.

fork-ts-checker-webpack-plugin on the other hand sees a syntactic error and does not report errors, assuming that the build has failed. After all, fork-ts-checker-webpack-plugin has no way of knowing which files are being processed by webpack and which files aren't.

Leaving the user will a successful compilation and reports of no errors, even though there might be any number of errors actually present :/

From the user's perspective:

1. The build succeeds, but there is a report of an error that blocks further type checking.
2. (let's skip that one, that is actually impossible from looking at the TS code)
3. Same behaviour as in useTypescriptIncrementalApi: false - build succeeds, but errors are reported.

From looking into it, 3. might be much more difficult that I thought. That TS code is a f**ing mess :/ But I'll keep investigating.

[johnnyreilly]

Cool - what do you think the option of generally steering people to do all type checking with the fork-ts-checker-webpack-plugin? Codewise there's technically nothing to do. They can achieve the same goal right now by using ts-loader with happyPackMode: true and fork-ts-checker-webpack-plugin with checkSyntacticErrors: true.

Obviously having to supply those two options is rubbish. But if we're agreed that it works then a possibility is to do a breaking changes release of fork-ts-checker-webpack-plugin which sets checkSyntacticErrors: true by default, and introduce a friendlier alias for happyPackMode to ts-loader. (eg noTypeCheck: true)

The options you suggest may well be the better choice, I just want to establish whether this is a potential other option.

[phryneas]

I think that's a good option too, but it would need an update to ts-loader and fork-ts-checker-webpack-plugin at the same time through the user, or a change to configuration. I can kind of see that being error-prone :/

As for option 3., I have got that one working.

In service.ts, directly after const typescript: typeof ts = require(process.env.TYPESCRIPT_PATH!);:

const origCreateEmitAndSemanticDiagnosticsBuilderProgram =
  typescript.createEmitAndSemanticDiagnosticsBuilderProgram;

typescript.createEmitAndSemanticDiagnosticsBuilderProgram = (
  ...args: any[]
) => {
  const program = (origCreateEmitAndSemanticDiagnosticsBuilderProgram as any)(
    ...args
  );
  if (process.env.CHECK_SYNTACTIC_ERRORS !== 'true') {
    program.getSyntacticDiagnostics = () => [];
  }
  return program;
};

This makes the ApiIncrementalChecker emit errors even if there is a syntactic error.

PS: passing this wrapping method to typescript.createWatchCompilerHost instead of typescript.createEmitAndSemanticDiagnosticsBuilderProgram does not have the same effect. That's what got me in the first place to think that wasn't possible at all...

[phryneas]

Question is: how would you like to go on from here?

While option 3) feels a bit hacky at first sight, it is essentially how #231 would work internally, so this could be a first step in that direction.
If you would be comfortable with that, I could create a PR based of #253 that fixes the tests and implements this workaround.

That should clear all errors for >=2.6. From then, I could investigate the additional vue error we have with 2.5.

Also, I would suggest visiting the travis config.
We should definitely test for older TS versions there. Maybe we could go from the current approach testing every possible combination of webpack/ts-lint/vue-loader/os/node/typescript to a few more picky tests?

[johnnyreilly]

While option 3) feels a bit hacky at first sight, it is essentially how #231 would work internally,

And yet you don't want to use proxies? 😉

Question is: how would you like to go on from here?

Great question...

Your plans sound good. I say go for it! Any thoughts @piotr-oles?

[phryneas]

While option 3) feels a bit hacky at first sight, it is essentially how #231 would work internally,

And yet you don't want to use proxies? wink

Question is: how would you like to go on from here?

Nah, the proxies are out of there for a while. Actually, while those things are interesting, it's really easy to shoot yourself in the foot with them. Make a change somewhere down the road and you got an infinite loop that's really difficult to debug (especially for future contributors). All that remains is the name of that PR 😁

[piotr-oles]

The option 3 looks reasonable - as far as I understand we don't make breaking change with that option.

Speaking about moving forward - in my opinion this plugin became hard to maintain. We have to take care about:

• IncrementalChecker
• ApiIncrementalChecker
• TsLint
• VueProgram

keeping compatibility with many typescript, webpack and tslint versions. We need to revise the architecture of the plugin. I think version 2 of this plugin should give up IncrementalChecker in favor for ApiIncrementalChecker. Also vue part should be handled in more generic way - that will allow to handle mdx using the same API (and possibly other formats in the future). That's what came to my mind - I saw promising PRs here but I think we need plan/roadmap 😃

[johnnyreilly]

I think we need plan/roadmap

Agreed - if you'd like to open an issue that's probably a good start. Here's my initial thoughts:

I think version 2 of this plugin should give up IncrementalChecker in favor for ApiIncrementalChecker

When we get to the point where ApiIncrementalChecker can do all of the things IncrementalChecker can then I'm totally sold! That's not where things are now; but hopefully that will happen...

Also vue part should be handled in more generic way - that will allow to handle mdx using the same API (and possibly other formats in the future).

Yup - that's what #231 is ideally going to lead us to.

TsLint

I'm hoping we'll drop TSLint in the future (given it is deprecated) but I'd like us to add support for ESLint if we can. See more here: #203

keeping compatibility with many typescript, webpack and tslint version

I'd like to think about dropping support for older TypeScript versions. That's a given anyway if we move to the ApiIncrementalChecker anyway. If we move to have just that and not the IncrementalChecker then we start with a minimum TypeScript version of 2.9.

All things to consider!

Also, @piotr-oles would you be okay with making @phryneas a collaborator on the plugin? He's been doing some great work which has really helped us.

[phryneas]

Also, @piotr-oles would you be okay with making @phryneas a collaborator on the plugin? He's been doing some great work which has really helped us.

@johnnyreilly I'm honored that you consider that. One thing though, just to get this off my chest: I can't make any guarantees how much time I can/will spend with this project in the future - but if that is okay, I'd be happy to continue contributing and try to stick around for a while. This is very much fun :)

[johnnyreilly]

I can't make any guarantees how much time I can/will spend with this project in the future - but if that is okay, I'd be happy to continue contributing and try to stick around for a while.

It's fine with me - people come and go. That's to be expected. But I'm always keen to make the best of people when they have time and energy to spare. Doing open source well is very much about catching the wind when it blows your way ⛵️ 😉

[piotr-oles]

@phryneas Invitation sent - welcome aboard 🚢

[phryneas]

I'm just running the tests again for the current 1.2.0-beta.4. Looks promising: https://travis-ci.org/phryneas/fork-ts-checker-webpack-plugin/builds/523703738

From what I see:

• from 2.1 to 2.4, there is only one non-vue test left failing: "should support custom resolution" - as this was just added recently with Enables custom resolver only if they're used in incremental mode #260, maybe @arcanis could take a look if it is possible to get that to work with older TS versions?
• vue-specific tests are failing as before (up to 2.5). But as we saw already, from version >= 2.3, there is only one of those failing: "should resolve src attribute but not report not found error" - maybe I'll take a look at that one in the next few days.

Fixing these two tests (or potentially, bugs) we would be at "fork-ts-checker-webpack-plugin is compatible with TS 2.1, if you want vue support you need TS 2.3".

I think we could live with that?

[arcanis]

I can take a look but not before next week at the earliest. In the meantime maybe they can be skipped? 2.5 is almost two years old 😅

[phryneas]

@arcanis no worries, those are not the regular tests 😀
I just took a moment and executed the test suite against all TS versions we officialy support at the moment, so it would be great to get that going again - otherwise we'd have to drop support for older TS versions - but I don't think this is too urgent ;)

[johnnyreilly]

Frankly I'm happy with us just not supporting < 2.5

We should technically do a breaking change for that (and we likely will in future). But given how old that is, and that no-one is really knocking down our door about this, the pragmatist in me is fine with this. 😁

Interestingly, in ts-loader we have tests that are only executed when we are above a certain version of TypeScript: /~https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests (mainly as certain features aren't in older versions of TypeScript)

That's also a strategy I am fine with. 😁

[piotr-oles]

Support was re-evaluated in the new alpha release (TypeScript >= 2.7.0 - because of the Watch API). The E2E tests check different versions of TypeScript :)