Skip to content
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

Build passes if some import has filename with different case #289

Closed
Reeywhaar opened this issue May 15, 2019 · 12 comments · Fixed by #293
Closed

Build passes if some import has filename with different case #289

Reeywhaar opened this issue May 15, 2019 · 12 comments · Fixed by #293

Comments

@Reeywhaar
Copy link

Reeywhaar commented May 15, 2019

Current behavior

Consider we have src/SomeLib.ts and import it as import {something} from "src/someLib".
If SomeLib has type errors fork-ts-checker will give no hint about this. Even more, it completely misses all type errors in project and always builds with "no type errors found". Standard ts compiler will give errors and will warn that import filename case differs from original.

Expected behavior

Fork-ts-checker should give same errors as ts compiler

Steps to reproduce the issue

Create index.ts and lib.ts, put content to index.ts: import {Something} from "Lib.ts", put content to lib.ts

type Obj = {something: number};
export const Something: Obj = {something: "notnumber"};

build with webpack.

Test Repository

https://github.com/Reeywhaar/fork-ts-checker-289

Environment

  • fork-ts-checker-webpack-plugin: 1.3.1
  • typescript: 3.4.5
  • webpack: 4.30.0
  • os: MacOS 10.14.4

Additional

tsconfig.json
{
  "compilerOptions": {
    "target": "es2017",
    "module": "esnext",
    "lib": ["es2017", "dom"],
    "incremental": true,
    "jsx": "preserve",
    "jsxFactory": "React.createElement",
    "strict": true,
    "esModuleInterop": true,
    "moduleResolution": "node",
    "alwaysStrict": true,
    "sourceMap": true,
    "noImplicitAny": false,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "allowSyntheticDefaultImports": true,
    "skipLibCheck": true,
    "allowJs": true,
    "baseUrl": "./src",
    "outDir": "./build",
    "paths": {
      "@app/*": ["./*"]
    }
  },
  "include": ["./src/**/*"],
  "exclude": ["node_modules", "public"],
}
@Reeywhaar Reeywhaar added the bug label May 15, 2019
@phryneas
Copy link
Contributor

Does this behave differently when you change useTypescriptIncrementalApi to true or false?

@Reeywhaar
Copy link
Author

Nope, behaviour is same, whether it true or false

@Reeywhaar
Copy link
Author

Reeywhaar commented May 15, 2019

Oh, no, sorry, I've tested wrongfully. Setting useTypescriptIncrementalApi to false actually makes checker behave normal, except it doesn't give warning about case mismatch, but at least build fails.

@phryneas
Copy link
Contributor

Ah. What happens if you combine checkSyntacticErrors: true with useTypescriptIncrementalApi: true?

@Reeywhaar
Copy link
Author

Aha, build fails!

ERROR in undefined(undefined,undefined):
TS1149: File name 'removed/DispatchBinder.ts' differs from already included file name 'removed/dispatchBinder.ts' only in casing.

@phryneas
Copy link
Contributor

So this is an interesting edge case.

Just to give you some background on this:
We have two different types modes we can run the type check in. One of them is the old mode - incremental: false. This has file discovery & watching written mostly by hand. The new mode is the mode that typescript uses internally with their file watcher.

Their version does an early exit if a syntactic error is found and will not emit any other kind of error. But as usually ts-loader already reports syntactic errors, we don't emit those by default so that you don't see this error twice. So you neither get an error nor an indication that an error might have been seen, but has never been looked for.

With #268, the situation there was improved (syntactic errors now should not prevent the emission of semantic errors), but your error is apparently neither a syntactic or semantic error, but a third category - I guess a global error.

I had not seen one of these in the wild until now :/

I'll try to get a PR for this running in the next days - if possible. But it might also be necessary to change the default behaviour altogether. I'm not sure for now.

Anyways: it would be great if you could do some testing when I come back to this in a few days :)

@johnnyreilly - do you have any thoughts on this?

@Reeywhaar
Copy link
Author

At your service, of course. Guess I should make test repo, since bug is legit.

@johnnyreilly
Copy link
Member

I too had not seen a global error in the wild either! 😁

But you can see the behaviour in the TypeScript incremental API here:

https://github.com/Microsoft/TypeScript/blob/abc861862ab7c08b07216ba2c4ede7ccefc1cc60/src/compiler/watch.ts#L134

To quote the comment:

// If we didn't have any syntactic errors, then also try getting the global and
// semantic errors.

So yeah, it looks like we should be catering for global errors in the same way we do for semantic errors.

Oh and a test to cover this would be tremendous 🤗

@Reeywhaar
Copy link
Author

Reeywhaar commented May 15, 2019

So, made test repo https://github.com/Reeywhaar/fork-ts-checker-289

Funny enough, now I see that ts-loader behaves differently from project where I've found bug, In my project it reports all errors (type error, casing error). However, here it reports duplicated error.

@johnnyreilly
Copy link
Member

Yeah I'd expect ts-loader to be fine here. It generally doesn't use the incremental API. It has support through an (undocumented) option called experimentalWatchApi. But problems were reported with it and so I've never felt comfortable promoting it to be the default.

https://github.com/TypeStrong/ts-loader/blob/cc7b2b18fb30bb6b3a6b0e17fb2f2ee95ca55ac6/src/index.ts#L252

@phryneas
Copy link
Contributor

Oh, great, this is a double-edge-case: It only fails on operating systems with case-insensitive file systems like OSX or Windows. On my Linux machine, this doesn't even get to the global error, because it already fails before that step. Gotta find a way to test that :D

@piotr-oles
Copy link
Collaborator

🎉 This issue has been resolved in version 1.3.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants