Don't typecheck or load files under node_modules/ unless they're imported by flow-typed files

[Macil]

Most of the files under node_modules aren't the responsibility of my project, errors in them are probably due to them being written for a different Flow version, and the only reason to parse them would be if my project has flow-typed files referring to types in them.

It seems like a common issue is that Flow takes longer to start up than it ought to because it reads all of the javascript files under node_modules/ when the user isn't interested in typechecking most of them (#863). Tons of files under node_modules/ are sub-dependencies of other non-Flow-typed dependencies. People can manually ignore individual dependencies to speed things up (example), but this loses its effectiveness with the newly-released npm 3 because now dependencies aren't usually nested. (Browserify's many dependencies are now placed directly under node_modules/ instead of node_modules/browserify/node_modules/, so ignoring .*node_modules/browserify.* is no longer nearly as effective.)

It makes sense to check the flow types of files under node_modules/ if they're imported by flow-typed files in the project itself, and maybe the types of the files those ones include if it's necessary to fully figure out their types, but I think most/all users would prefer that files under node_modules/ would not be even loaded for typechecking unless they're imported by a flow-typed file in the project proper. Maybe this could be a (default-on?) setting in .flowconfig.

[GGAlanSmithee]

Also #719 and #835

[samwgoldman]

This seems like a good idea to me, but it also sounds really hard, given how things currently work.

[danvk]

Also #863 for an easy way to make this problem more user-visible.

[jamiter]

When I test my own code with Flow I'm not interested at all in type errors in the node_modules directory. I can't change anything about it anyway. But I do like to be able to import a module without having to declare the module interface for every NPM module I use. This is suggested in #719. I find it hard to accept this as a workaround... That means a lot of manual work for anybody using Flow.

Shouldn't node_modules be treated as a library in some way? As in: do not type check them or report on errors in them, but do find them and use them when imported.

I think most/all users would prefer that files under node_modules/ would not be even loaded for typechecking unless they're imported by a flow-typed file

I agree, but do you expect Flow to show you errors in those imported files? Personally I only want errors from my own code. Flow should be smart enough to understand that code in node_modules isn't something you have control over.

[samwgoldman]

We made a big change here in the last release. Only @flow files will be parsed, which I suspect will improve this situation considerably.

[jamiter]

@samwgoldman, thanks for the update! Just tested it and t does check a lot less files which is good! An option to totally switch it off for node modules would be great though.

[Macil]

@jamiter It's not (yet) very common, but files under node_modules can have type definitions that are used by your project. For example, my react-save-refs and ud modules both do, so by default you get type-checking on calls to them without any configuration needed. (babel-plugin-transform-flow-comments can be used to do this, though it has issues like T7054. Instead, I just include the babel-transpiled code in the NPM package, and then include the original source with the ".flow" suffix to the filename, which Flow reads instead. This is set up by the "prepublish" commands in the scripts section of the modules' package.json.)

[jamiter]

@agentme, thank you for the clarification. I'm new to FlowType and still getting to know all implementation details. I understand your use case, but it could lead to issues if some module is running another (out of date) flow version, right? In your case you can update your own module and push it to NPM, but it won't always be that easy.

So I still think that Flow should understand the types defined in node modules, but not show any errors in the command line about them. Only when your own code doesn't match any defined types (in node modules or your own code). How can you use Flow in CI when it throws errors about code you do not own?

[Macil]

If a newer version of Flow caused it to not understand the types in one of my modules, the error message would be more useful than silently failing so I knew there was something wrong. I can always add the module to my .flowconfig's [ignore] section. (I currently have to do that with the fbjs module and some babel modules which seem to have some invalid flow-typed code.)

[jamiter]

Just to get this right: You can add it to your ignore section, but then you will get Required module not found errors. So you not only have to ignore them but also create a definition file where you declare the ignored module, correct?

The new version seems to ignore most node_module files now, which means none of them are found anymore. Which means a zillion declarations have to be written... And it seems like this is by design. So apologies for highjacking this thread, I'm just a little surprised that this is the case.

[toddw]

I'm very interested in using flow but I'm running into this issue as well. My project is reporting errors about files I can't really do much about. I feel like there should be an option to ignore errors in the "node_modules" folder. At first I thought [ignore] was exactly what I needed until I ran into "Required module not found" errors all over. I realize that flow needs to have knowledge of the files included in a project, but it would be nice if there was an option to silence errors found in the node_modules files.

If I submitted a PR for this, would it be considered?

[samwgoldman]

@toddw What errors are you running into specifically?

[toddw]

node_modules/fbjs/lib/PromiseMap.js.flow:15
 15: var Deferred = require('Deferred');
                    ^^^^^^^^^^^^^^^^^^^ Deferred. Required module not found

node_modules/fbjs/lib/fetchWithRetries.js.flow:16
 16: var ExecutionEnvironment = require('ExecutionEnvironment');
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ExecutionEnvironment. Required module not found

node_modules/fbjs/lib/fetchWithRetries.js.flow:19
 19: var sprintf = require('sprintf');
                   ^^^^^^^^^^^^^^^^^^ sprintf. Required module not found

node_modules/fbjs/lib/fetchWithRetries.js.flow:20
 20: var fetch = require('fetch');
                 ^^^^^^^^^^^^^^^^ fetch. Required module not found

node_modules/config-chain/test/broken.json:11
 11:   "dependencies": {
       ^^^^^^^^^^^^^^ Unexpected string

node_modules/npmconf/test/fixtures/package.json:1
  1: 
     ^ Unexpected end of input

node_modules/y18n/test/locales/bad-locale.json:1
  1: {"hello": "worl}
     ^ Unexpected token ILLEGAL


Found 7 errors

[samwgoldman]

@toddw Thanks! The solution in this case is to ignore the specific files that are problematic:

I haven't tested it, but this should work:

[ignore]
.*/node_modules/fbjs/.*
.*/node_modules/config-chain/test/broken.json
.*/node_modules/npmconf/test/.*

I've also sent an upstream change to fix fbjs, so the next version should no longer cause this issue. We're also working to fix the invalid JSON errors, so that shouldn't be an issue soon.

Ignoring everything in node_modules is problematic, because we look in there to a) ensure you've actually installed your dependencies and b) find Flow types for packages which might have included them.