Add incremental compilation.

[0xorial]

#196

[johnnyreilly]

Thanks @0xorial - this is amazing work!

We've got some failing tests:

• should not fix linting by default x 2
• should find the same errors on multi-process mode

Could you look into these please?

Also, are we running the test pack with incremental watch API both on and with it off? Ideally we want to ensure we do this so there's no regressions to either mode going forwards.

[johnnyreilly]

Very exciting by the way ♥️🌻🤗 - really appreciate your hard work!

[johnnyreilly]

I had a quick look at the failing tests:

should not fix linting by default - it appears to be the same failure twice:

expect(stats.compilation.warnings.length).to.be.eq(7);

Both are failing as they're getting a 8 warnings and not 7. I'll admit this is not the most helpful error to debug. Could be worth finding out what the expect warnings are when running the testpack prior to your PR. Maybe then we can find out if this is a meaningful deviation and what to remedy. The test as is makes it sure hard to fix 😄

should find the same errors on multi-process mode

This one sounds more curious; it seems like a legitimate failure; 😢

[johnnyreilly]

Actually - I think all the failures came down to changes made in the https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/master/test/integration/project/src/index.ts file.

We now have passing tests! I'll to give this a proper review soon. Thanks again for your work!

[johnnyreilly]

is this significant?

[0xorial]

It looks so; from a quick glance at usages of that function inside TypeScript I imagine it may cause some weird bugs on Linux. Glad that you noticed!

[johnnyreilly]

I tried running the standard integration test pack with incremental by amending createCompiler to the following:

  function createCompiler(
    options,
    happyPackMode,
    entryPoint = './src/index.ts'
  ) {
    options = options || {};
    options.useTypescriptIncrementalApi = true;
    var compiler = helpers.createCompiler(options, happyPackMode, entryPoint);
    plugin = compiler.plugin;
    return compiler.webpack;
  }

and the following tests failed:

    1) should work without configuration
    2) should fix linting errors with tslintAutofix flag set to true
    3) should find the same errors on multi-process mode
    4) should not find syntactic errors when checkSyntacticErrors is false
    5) should find syntactic errors when checkSyntacticErrors is true
    6) should only show errors matching paths specified in reportFiles when provided

Ideally we should look to get to a point where these would pass regardless of the mode. Obviously there's some nuance in this; should find the same errors on multi-process mode for instance is a test we don't need in incremental mode

[johnnyreilly]

A puzzle. Looking at the index.spec.js there is:

  it('should find syntactic errors when checkSyntacticErrors is true', function(callback) {
    var compiler = createCompiler({ checkSyntacticErrors: true }, true);

    compiler.run(function(error, stats) {
      expect(stats.compilation.errors.length).to.equal(2); // here we're expecting 2 errors
      callback();
    });
  });

The equivalent test in incrementalApi.spec.js is:

  it('should find syntactic errors when checkSyntacticErrors is true', function(callback) {
    var compiler = createCompiler({ checkSyntacticErrors: true }, true);

    compiler.run(function(error, stats) {
      expect(stats.compilation.errors.length).to.equal(1); // here we're expecting 1 error
      expect(stats.compilation.errors[0].rawMessage).to.contain('TS1005');
      callback();
    });
  });

This is surprising - don't we want to expect the same number of errors regardless of mode?

[0xorial]

Thank you so much for your help with that PR!

Just some of my story behind the tests....
Initially I my idea was indeed to somehow reuse all existing tests and switch mode incremental/normal. However, I had 3 issues with that, two more technical and one more stylistic:

1. I did not manage to find an easy way to reuse the tests. I was looking for something analogical to this. (by 'easy' I mean that it would not break existing tooling - in particular I use WebStorm and it is able recognize individual test cases in code and run/debug them separately).
2. There were some differences regarding which errors are produced - one that I remember - incremental compilation would not tell semantic errors if it found syntactic. Therefore this would also require some adjustments inside tests.
3. There are some cases which are indeed not applicable to incremental compilation.

Because of that I ended up extracting some helper code and creating a completely separate file, which only has cases which are relevant to the incremental compilation.

Regarding the failing cases that you got, I am going to have a look at them, but at least some of those are probably due to differences in how incremental compilation works currently.

[0xorial]

This is surprising - don't we want to expect the same number of errors regardless of mode?

Yep, that is the case that I was talking about.

[johnnyreilly]

incremental compilation would not tell semantic errors if it found syntactic. Therefore this would also require some adjustments inside tests.

Cool - so this is actually the behaviour of the watch API I guess? In which case the difference makes sense.

Regarding the failing cases that you got, I am going to have a look at them, but at least some of those are probably due to differences in how incremental compilation works currently.

Awesome - could we document the expected different behaviour in the tests somehow too please? Could be in comments, could be in the test name. I don't really mind but I just want it to be clear to anyone looking at the tests that there are expected differences in behaviour between the two approaches.

[johnnyreilly]

Hey @0xorial!

You're either going to love or hate my last commit. I've identified common tests and parameterised them to run with useTypescriptIncrementalApi: true and useTypescriptIncrementalApi: false. I'm glad that this covers off running common tests in both modes but I'm mindful about what you said about the debuggability of tests.

Does doing this torch your ability to debug?

I'm open to changing this (or you changing this) to keep these tests debuggable. I place high value on debuggability.

What say you?

Also, question: the should work without configuration test feels like it should work with useTypescriptIncrementalApi: true - any ideas why it wouldn't?

[normano64]

This dependency doesn't seems to be used, was it maybe included for the linked list at one point?

[0xorial]

you are right. removed.

[normano64]

Thanks for your awesome work, I am looking forward to trying this feature out when it is merged :)

[0xorial]

@johnnyreilly If the trade-off is between nicer code and simpler build/run/ide configuration, I would probably lean slightly to nicer code side. The main reason I didn't do anything like this myself was because I was not sure that changes so significant would be accepted :)

But yeah, WebStorm does not recognize the cases as individual tests anymore, so there is no UI button to run them.

On the other hand, I am a bit disappointed that mocha does not have any API to support tests with variable parameters - it seems like a rather essential feature. Personally, I would consider using a runner that does that (I don't know much about js test runners choices though :) )

[johnnyreilly]

Cool - I'll leave it as is for now then. It's pretty straightforward to unpick what I've done to make a test debuggable so I figure it's probably a good choice. It's pretty rare that you actually need to do that. (Though obviously super useful when the time comes 😁)

Any thoughts on this:

question: the should work without configuration test feels like it should work with useTypescriptIncrementalApi: true - any ideas why it wouldn't?

[0xorial]

Ah, sorry, missed that. I've quickly checked what is happening, and the checker finds syntactic error in 'syntacticError.ts', but it is ignored because checkSyntacticErrors is false.
While writing this comment, I checked inside TypeScript code and found the reason for this behavior.
Now I would like to try to get this working, but also wondering if this is not a requirement for performance :) I am going to investigate that a bit and post results here.

BTW, earlier I noticed that with {checkSyntacticErrors: false, tsLint: true}, syntactic errors are still shown because in that case linter starts producing them :) Or did I miss something?

[johnnyreilly]

BTW, earlier I noticed that with {checkSyntacticErrors: false, tsLint: true}, syntactic errors are still shown because in that case linter starts producing them :) Or did I miss something?

Really? 😄 No - haven't noticed that; maybe something to look into though!

Ah, sorry, missed that. I've quickly checked what is happening, and the checker finds syntactic error in 'syntacticError.ts', but it is ignored because checkSyntacticErrors is false.

I didn't quite follow what you meant here. Whilst checkSyntacticErrors is false the semantic error in index.ts should still be found presumably?:

// Semantic error
const x: number = '1';

So I'm puzzled this is not erroring at all. Or did I misunderstand your meaning?

At the moment it looks like the incremental API is not surfacing semantic errors at all - which is obviously a big problem! 😄

[johnnyreilly]

Actually - I've dug into this myself. I follow what you meant. The project context includes a semantic error (index.ts) and a syntactic error (syntacticError.ts). The test simply checks that there is any error surfaced. There is different behaviour for useTypescriptIncrementalApi: true and useTypescriptIncrementalApi: false but that is OKAY. 😄

In fact even good.

useTypescriptIncrementalApi: true

The compiler detects errors in both index.ts and syntacticError.ts. The compiler has found a syntactic error and so doesn't bother to go on to surface the semantic error (for whatever reason - as you say likely performance). But because we're not reporting syntactic errors we never get to hear about it; and our test fails.

useTypescriptIncrementalApi: false

Same story as with true but there's no short circuiting (that we're aware of) happening in the compiler in this case so we hear about our errors and our test passes.

[injitools]

vue projects supported?

[johnnyreilly]

@injitools

vue projects supported?

Yes - neither myself not @0xorial are vue users though, so all feedback on usage with vue will be appreciated!

[johnnyreilly]

I've written another test @0xorial - this one is called should find semantic errors. It covers the use case of ensuring that when semantic errors alone are present they are surfaced. I've added it to the common tests.

Assuming CI passes I think we might be ready to merge!

[johnnyreilly]

Okay let's merge this!

I'll push this out as 1.0.0-alpha.2. Anyone who wants to use this should be able to do so by targeting that or @next in their package.json and add useTypescriptIncrementalApi: true to the plugin when they initialise it in their webpack.config.js.

Please note that there's nothing to be scared about using fork-ts-checker-webpack-plugin@next - fork-ts-checker-webpack-plugin has merely been updated for webpack 5 (which is in alpha) and the @next reflects that if webpack 5 changes then we will make the necessary changes to the plugin without doing a major version increment.

To be clear, the @next version of the plugin still supports (remarkably!) webpack 2, 3 and 4. Users of those versions of webpack should feel safe using this; usage is unlikely to change. webpack 5 users should expect potential changes to align with webpack 5.

I'll try and write a blog post this week to publicise this.

[johnnyreilly]

Blogged about this here:

https://blog.johnnyreilly.com/2019/01/typescript-and-webpack-watch-it.html