Strange "Cannot find module" error when using with TSLint and the no-unused-variable rule

[pelotom]

This is a very weird bug which took me hours to whittle down to a minimal example, so I hope it's of use. Steps to reproduce:

1.  git clone https://github.com/pelotom/fork-ts-checker-bug
    cd fork-ts-checker-bug
    npm install
    npm start
   

2. Everything should compile fine on the first run. Now open the file foo.ts, add a newline or some other innocuous change, and save it.
3. Now there is an error in a completely different file:

   ERROR in /Users/tom/code/fork-ts-checker-bug/index.ts
   (1,25): Cannot find module 'moment'.
   

What the hey!?

Some observations:

• Undoing the change has no effect; the error remains. However modifying index.ts and saving it fixes the problem somehow.
• Killing the watch process and restarting fixes the problem; first builds always work
• The problem only occurs with the no-unused-variable TSLint rule enabled
• If another library besidesmoment is imported in index.ts, it may or may not exhibit the problem. For example, typestyle exhibits the problem but rxjs doesn't. I'm not sure what the commonality is among packages that run afoul of this.

[piotr-oles]

Thank you for reporting this issue with reproduction repo :) I will check it, but I'm busy so it would be probably in the next week. But maybe someone else could help? PR's are welcome :)

[univerio]

I stepped through the code and it looks like it happens for packages that specify typings in their package.json. Because no-unused-variable pollutes the module resolution cache in SourceFile instances, if they are reused after linting, and if the set of changed files does not cause typescript to re-resolve these packages, it fails to resolve them altogether.

A quick workaround is to do this:

diff --git src/IncrementalChecker.ts src/IncrementalChecker.ts
index 56fdb70..ca69bda 100644
--- src/IncrementalChecker.ts
+++ src/IncrementalChecker.ts
@@ -115,6 +115,11 @@ class IncrementalChecker {
       return files.getData(filePath).source;
     };
 
+    // after linting, cached `SourceFile` instances may not be safely reused
+    files.keys().forEach(filePath => {
+      files.remove(filePath);
+    });
+
     return ts.createProgram(
       programConfig.fileNames,
       programConfig.options,

But obviously this has some perf impacts. I'm not sure how this can be properly fixed.

[univerio]

Related tslint issue: palantir/tslint#2763

[johnnyreilly]

Thanks for the information @univerio - as I understand it this only occurs when the no-unused-variable is in play.

It's cool there's a workaround. Given this problem only occurs for the one rule I'm not that keen to use the workaround in all cases. I'm kinda reticent to use it at all given it's working around a problem in another package which might be fixed in future.

That said, I'm wondering if it would be useful to put it behind a flag so you could "opt-in" to that behaviour. It could be driven directly from the tslint.json of course too. But if the problem was fixed upstream in the future then we'd need to ship another version the remove it.

What do you think @piotr-oles ?

[pelotom]

Thanks for looking into this and filing a bug with tslint, @univerio. Speaking for myself, due to this issue and the fact that lint rules which require type checking don’t show up in VSCode, I’m currently opting instead for tsc’s noUnusedLocals and noUnusedParameters. I really hope tslint fixes this rule someday because I want it to be autofixable, but in the meantime there are too many reasons why it’s not worth it.

[johnnyreilly]

@pelotom makes an excellent point; the various tsc noUnused... compiler options make the no-unused-variable less useful. If memory serves, tslint do retire rules when the Typescript compiler offers an "in-the-box" alternative. I wonder if this rule will be retired as a result?

[univerio]

@johnnyreilly It will not. palantir/tslint#1481

I actually dug around a little more last night, and it seems that the proper fix would be in tslint. The reason I say this is that I've noticed similar behavior in tslint-language-service. I have filed an issue with them with a (seemingly okay) patch. This fix works for tslint-language-service as well.

[pelotom]

As I commented on that issue, I don't want this rule to go away. I just want them to fix it!

[clentfort]

Is there any way a temporary fix could be applied in fork-ts-checker-webpack-plugin? This package is now used as the webpack-loader by react-script-ts which might surface this problem to many, especially new, users.

[mbrowne]

@clentfort You're right - I was just affected by this on a new create-react-app project using the new beta version of material-ui (https://material-ui-next.com/). It took me quite a while to troubleshoot it and find this thread.

[andrewbranch]

Oh, I'm so glad I finally found this—maybe a maintainer would like to add “Cannot find module” to the issue title, because I gave up on this plugin before coming back to it weeks later to discover this.

[pelotom]

@andrewbranch I changed the title

[kelly-tock]

so what is the actual fix for this? just ran into it, and am glad to have found the core issue at least.

[pelotom]

@kelly-tock my fix has been to abandon the no-unused-variable lint rule and instead use TypeScript's noUnusedLocals and noUnusedParameters options instead. I still believe this is the kind of thing a linter ought to handle, but at the moment the happy path seems to be letting the compiler do it. This lint rule is just buggy and problematic in a number of different ways and I've seen no indications that it will be fixed soon.

[univerio]

@kelly-tock The actual problem exists in tslint itself, and, specifically, the way the no-unused-variable rule is implemented. I've filed palantir/tslint#3671, but heard no response.

In the meantime I've resorted to using a local fork of that rule with the patch in the linked issue.

I've also come across the no-unused rule from ajafff/tslint-consistent-codestyle, which is advertised to be immune from this problem, but I have not tested it myself.

[kelly-tock]

hmmmm...i'm still getting this issue actually.

• from tslint.json I took out the no-unused-variable
• from tsconfig.json I added "noUnusedLocals": true,

I could not turn on noUnusedParameters at this time.

new ForkTsCheckerWebpackPlugin({
      formatter: 'default',
      logger: {
        error: console.error,
        warn: console.warn,
        info: function() {},
      },
      checkSyntacticErrors: true,
      tslint: path.resolve(__dirname, '../tslint.json'),
      memoryLimit: 3072,
    }),

on tslint 5.10.0.

and after I run webpack, make an obvious change to cause lint error, I get

ERROR in /...GoogleMap.tsx(3,64):
TS2307: Cannot find module 'react-google-maps'.
ERROR in /....GoogleMap.tsx(4,23):
TS2307: Cannot find module 'react-google-maps/lib/components/places/SearchBox'.

any thoughts?

[kelly-tock]

realized the project I was working with extended a few different configs, so I put no-unused-variable in as false and its working.

[mbrowne]

I've noticed that this is only an issue when changing certain top-level files (at least in my current project). When changing most React components it doesn't occur.