fork-ts-checker-webpack-plugin: Re-Evaluate supported TypeScript versions, open bugs in older versions

@phryneas Invitation sent - welcome aboard 🚢

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 😃

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…

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 ⛵️ 😉

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 https://github.com/Realytics/fork-ts-checker-webpack-plugin/pull/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: https://github.com/Realytics/fork-ts-checker-webpack-plugin/issues/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.

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?

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.

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…