New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report diagnostics only on certain files [WIP] #701

Merged
merged 9 commits into from Jan 20, 2018

Conversation

Projects
None yet
2 participants
@freeman
Contributor

freeman commented Dec 23, 2017

Fixes #700

This is a first implementation heavily borrowed from at-loader.

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 ?

Also currently the pattern is applied against the fully qualified path to the file. I think we should match against the context relative path. Unfortunately it is not available in the formatErrors function.

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)

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Dec 23, 2017

Member

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.

Member

johnnyreilly commented Dec 23, 2017

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.

Show outdated Hide outdated src/utils.ts Outdated
@freeman

This comment has been minimized.

Show comment
Hide comment
@freeman

freeman Dec 23, 2017

Contributor

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).

Contributor

freeman commented Dec 23, 2017

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).

@freeman

This comment has been minimized.

Show comment
Hide comment
@freeman

freeman Dec 23, 2017

Contributor

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.

Contributor

freeman commented Dec 23, 2017

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.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Dec 23, 2017

Member

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.

Member

johnnyreilly commented Dec 23, 2017

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.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Dec 24, 2017

Member

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

Member

johnnyreilly commented Dec 24, 2017

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

@@ -245,6 +245,10 @@ messages are emitted via webpack which is not affected by this flag.
You can squelch certain TypeScript errors by specifying an array of diagnostic
codes to ignore.
#### reportFiles *(string[]) (default=[])*
Only report errors on files matching these glob patterns.

This comment has been minimized.

@johnnyreilly

johnnyreilly Dec 24, 2017

Member

Could you add an example here please? Examples rule the world when it comes to docs in my experience.

@johnnyreilly

johnnyreilly Dec 24, 2017

Member

Could you add an example here please? Examples rule the world when it comes to docs in my experience.

This comment has been minimized.

@freeman

freeman Dec 29, 2017

Contributor

I have added an example

@freeman

freeman Dec 29, 2017

Contributor

I have added an example

Show outdated Hide outdated src/utils.ts Outdated
@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Dec 24, 2017

Member

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)

Member

johnnyreilly commented Dec 24, 2017

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)

@freeman

This comment has been minimized.

Show comment
Hide comment
@freeman

freeman Dec 28, 2017

Contributor

I will try the execution test approach

Contributor

freeman commented Dec 28, 2017

I will try the execution test approach

@freeman

This comment has been minimized.

Show comment
Hide comment
@freeman

freeman Dec 28, 2017

Contributor

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 ...

Contributor

freeman commented Dec 28, 2017

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 ...

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Dec 29, 2017

Member

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.

Member

johnnyreilly commented Dec 29, 2017

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.

freeman added some commits Dec 29, 2017

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 1, 2018

Member

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!

Member

johnnyreilly commented Jan 1, 2018

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!

johnnyreilly added some commits Jan 20, 2018

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 20, 2018

Member

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

Member

johnnyreilly commented Jan 20, 2018

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

@johnnyreilly johnnyreilly merged commit ea8dfd7 into TypeStrong:master Jan 20, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 20, 2018

Member

Will look to release this with 3.3

Member

johnnyreilly commented Jan 20, 2018

Will look to release this with 3.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment