Initially this looks good to me. I'm on my mobile and so it's hard to do a proper review. Couple of things:

I had to add an explicit micromatch dependency (even though we have it indirectly, I felt it was cleaner to have a dependency on it if we rely on it). Is that ok ?

Yeah that's probably fine and if we're using something I definitely want to be explicit. How did we have it indirectly in the first place?

I think it is reachable in every call site. Is adding a new argument to formatErrors OK ? (note that I already took a first step by making the last argument non optional as it was provided at every call site)

Yup absolutely fine.

Yeah that's probably fine and if we're using something I definitely want to be explicit. How did we have it indirectly in the first place?

It is karma that is installing it (so only a dev dependency which makes it even more important to add it to the direct dependencies).

Seems complete. But I am having second thoughts about the option name.

reportFiles is nice because it is the same as at-loader (so moving between loader is easier). But I feel like ignoreFiles would be more coherent with ignoreDiagnostics. I am happy to change it if you prefer the ignoreFiles list approach.

There's a long standing plan (which may or may not happen) to merge ts-loader and ATL. As part of that we'd discussed standardising on the same API as we moved towards merging. For that reason I think going with the same option name as ATL is probably a good idea.

I've fixed (I think) the broken validateLoaderOptionNames test with this: freeman#1

Could we remove the comparison test you've implemented and replace it with an execution test please? https://github.com/TypeStrong/ts-loader/blob/master/test/execution-tests/README.md

Execution tests are run against all versions of TypeScript that are supported and tend to be less flaky than comparison tests (which we only run against the latest supported version of TypeScript)

My guess is you're going to want to implement broken code which will be ignored by use of the new reportFiles option. noEmitOnError in our tsconfig.json will allow us to detect if we're ignoring files correctly.

A good basis for your test might be this one: https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests/2.0.3_typesResolution - it follows the same principles

See also: https://github.com/TypeStrong/ts-loader/blob/master/test/execution-tests/2.0.3_typesResolution/test/app.tests.ts

So in your case the test need be no more complex than expect(1).toBe(1);. In order that the test pack passes it needs to contain a single passing test. In your scenario the actual test is arbitrary - the real test is than we compile without erroring with reportFiles in place. (in a situation where it would error without reportFiles filtering)

I will try the execution test approach

I don't think the execution tests approach can work because reportFiles (and ignoreDiagnostics) do not work at the typescript compiler level.

They only filter errors from webpack reporting but if noEmitOnError is true, nothing will be emitted anyway. Not sure if it is possible to go one level deeper ...

Yup - I think you're right. We could only use an execution test if we changed the approach so that the files being supplied to the Typescript compiler were varied by reportFiles. I think that would likely make things more complicated than they need be (and might introduce other issues) so a comparison test is fine.

This all looks good - thanks for your work @freeman! I'm currently shepherding through a very complex PR from one of the TypeScript team: #685

That's a biggie so I'm holding off merging other PRs until that's in. Once that's in I'll look to merge and release. Watch this space!

Tests aren't passing after other PRs were merged; I'm going to merge this and fix up afterwards

Will look to release this with 3.3