Fix bug where compiler is not accessible by computeContextPath

[DASPRiD]

I'm not 100% certain why this was causing a problem. I did some debugging and it turned out that computeContextPath as an arrow function had not the same this context. When changing it to a normal function, it works as expected.

Fixes #267

For me this bug was happening in conjunction with serverless-webpack and having individual packaging enabled.

[johnnyreilly]

Awesome work! Would you be able to add a test as well so we don't regress this in future please? (I've an idea we've broken this previously 😭)

[DASPRiD]

Well, the thing is, I have no idea how to reproduce the problem in an isolated test. I mean, the existing tests don't break, so I'm really not sure about the cause of this.

[johnnyreilly]

Never mind - let's get this anyways 😄

[johnnyreilly]

I'd ❤️ to merge this but it doesn't meet our commit guidelines: https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/master/CONTRIBUTING.md#examples

We're using semantic-release - can you fix up using this convention instead please?

[DASPRiD]

Sure, done.

[phryneas]

Not questioning this PR, (let's merge it as soon as the CI is through), but out of general curiousity, because I would love to understand the underlying problem:

What is your setup? Are you also using grunt-webpack like in #267?

[DASPRiD]

@phryneas No, as I wrote above, I am using serverless with serverless-webpack and individual packaging enabled.

[phryneas]

@phryneas No, as I wrote above, I am using serverless with serverless-webpack and individual packaging enabled.

Oh, sorry, somehow overread that :) Thanks 👍

So it seems this happens with multi-webpack setups.

[piotr-oles]

🎉 This PR is included in version 1.3.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[phryneas]

So what I believe happens is this:

serverless-webpack creates multiple instances of webpack and all those instances are just getting clones of the original configuration. If this is really the place I am suspecting, they are just calling cloneDeep on the config, which also creates a clone of the fork-ts-checker-webpack-plugin instance.
And then we are getting the bug: plugin.apply is called on the clone, but the method that was fixed with this PR is a bound arrow function, so this is still pointing to the original plugin, not to the clone. And that one is never initialized (and if it were, it would point at a wrong compiler).

[DASPRiD]

Wow, that actually makes sense. Good find!

[DASPRiD]

@phryneas Any idea if a test for that can be written?

[phryneas]

@DASPRiD Phew... I guess it can, at least partially.

Short analysis: this type of bug can only occur in the index.ts, in the class ForkTsCheckerWebpackPlugin itself. Everything else happens in a forked process, and that can't be incluenced by any such type of cloning.

Instantiating the plugin, deepCloning it and then just calling all methods won't do the trick though (as all these methods are highly interconnected) - and especially it will most likely not catch any new bugs introduced in the future.

The only thing I would see here is to create an integration test and in the prepareWebpackConfig callback of the createCompiler helper replace the plugin with a deepCloned copy. Then just run the plugin and see if it works or fails.

Something like this test could be a starting point, with the modifications I described above.

Would you be willing to give it a try? :)

[DASPRiD]

@phryneas uff, I'm not too much into all the guts of webpack or this plugin, so I'd rather leave that to you, as you seem to definitely have a better understanding of this :)

[johnnyreilly]

Once again I'm astounded by @phryneas ability to find, diagnose and understand an esotoric problem in the codebase... And not only that he documents it effectively thus communicating what he's learned to anyone that cares to read. Props! ♥️🥰

[phryneas]

@phryneas uff, I'm not too much into all the guts of webpack or this plugin, so I'd rather leave that to you, as you seem to definitely have a better understanding of this :)

@DASPRiD Feared so ^^.
I'll put it on my TODO list, but it can take some time until I get around to that.
If you change your mind and wanna give it a try before that, just do :) Can't really go wrong ;)

[madebyherzblut]

I am still having some trouble with this. Sometimes, this.compiler in computeContextPath is still undefined:

TypeError: Cannot read property 'compiler' of undefined
    at computeContextPath ([REDACTED]]/launchpad/node_modules/fork-ts-checker-webpack-plugin/lib/index.js:207:27)
    at Array.map (<anonymous>)
    at ForkTsCheckerWebpackPlugin.apply ([REDACTED]]/launchpad/node_modules/fork-ts-checker-webpack-plugin/lib/index.js:149:34)
    at webpack ([REDACTED]]/launchpad/node_modules/webpack/lib/webpack.js:47:13)
    at compile ([REDACTED]]/launchpad/packages/launchpad/src/scripts/start.js:72:16)
    at main ([REDACTED]]/launchpad/packages/launchpad/src/scripts/start.js:33:26)

The problem is fixed when I add a .bind(this) to this.watchPaths = this.watch.map(this.computeContextPath); (L283). The webpack configuration itself can be seen here–as far as I am concerned there is no cloning involved.

[phryneas]

@madebyherzblut you're absolutely right, that's a follow-up error that now occurs because that method is not implicitly bound any more sigh
I'm gonna look into that this evening.

[phryneas]

While I was at it writing a regression test für #300, I added one for this ticket, too :)