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.

[toddw]

Thanks @samwgoldman, if I use fbjs will I be getting the Required module not found error?

I've never written anything in OCaml so this is taking me longer than I hoped but I was thinking like the following:

(sorry for the screenshot and not a legit PR but I'm just throwing around ideas)

[samwgoldman]

Well, using fbjs isn't really advisable in general, but if you do want to I'd recommend waiting for the new release, which fixes the issue you ran into.

Regarding suppressing, I'm not sure how I feel about it. On one hand, we want to fix those issues, not suppress them. On the other hand, lots of new users run into issues and making their path easier is important.

I don't want to discourage you from trying, but I'm not confident that a PR adding this feature would be merged.

[toddw]

Fair enough. fbjs is probably not the best example. I'm imagining a scenario where some other 3rd party component has flow errors. As flow gets wider adoption, as I'm sure it will, it seems inevitable that people will run into more issues. I didn't think [ignore] worked the way it does. I expected it to work as [supress] which I think should be considered.

Either way, I appreciate the time you took to address my issue and I should be unblocked by your suggestion.

[toddw]

Update, I've been using flow for the past 20 days and this continues to be an annoying problem.

node_modules/react-motion/src/spring.js:7
  7:   config = presets.noWobble): SpringConfig {
       ^^^^^^ parameter `config`. Missing annotation

I do use spring from react-motion but I don't want warnings from packages that I'm not maintaining. Has there been any more thought about this?

[taylorlapeyre]

Just adding my 2¢ - this is a big oversight. It seems to me I currently have two options:

1. Go with the default settings so that imported npm modules get detected correctly, but see lots of flow errors from other packages that I do not maintain.
2. Add .*/node_modules/.* to the [ignore] section of my flow config so that I no longer see those errors, but instead get a different flow error that it can't detect any npm packages I import.

Both of these seem pretty unacceptable to me. Is there any workaround?

[skeet70]

This is a pretty big pain point. Flow seemed like the nicest way to get some sort of static typechecking incrementally added to our JS codebase, but both "ignore all the module not found errors" and "ignore all the errors coming out of node_modules" aren't really things I want to say when trying to sell the team on Flow.

[ffxsam]

It's not just fbjs that's an issue:

node_modules/cjson/test/fixtures/templates/conf11tmpl.json:2
  2:     "subobject": {{subobject}},
                       ^ Unexpected token {

node_modules/cjson/test/fixtures/templates/conf12tmpl.json:2
  2:     "num": {{num}},
                 ^ Unexpected token {

I don't know if it's practical to ignore individual npm packages as they identify themselves as being problematic. I don't see any issue with ignoring the node_modules folder. I've done it and run flow and it works just fine.

[iansinnott]

In my projects I've had success ignoring problematic modules on a case-by-case basis. For example, fbjs, react-motion, etc. For those modules that do expose flow types and do not throw errors it is really nice to not have to write one's own type definitions for them. Simply use the modules as you would normally and everything will type check.

I'm not saying the current situation is ideal, but for larger projects I haven't had issues selectively ignoring node modules.

[nkrambo]

It's by no way ideal, but as an intermediate solution @iansinnott's approach has worked for me too. I'd also like to see this better resolved however. Hitting immediate hurdles like this one are not a great first impression, even if Flow is a really great tool.

[kylebebak]

I'm all for adding .*/node_modules/*. under the [ignore] directive, but I don't like seeing all the Required module not found errors. As @toddw and others have mentioned, this adds a lot of noise to Flow's output.

One solution would be to implement a syntax for ignoring certain classes of errors in the config file, like with eslint. For example, you could just mark Required module not found errors with a priority of 0, so that they don't get printed when running flow.

[ffxsam]

I take back what I said. Ignoring the whole node_modules folder actually did wind up causing an issue for me.

[pdf]

@samwgoldman

Regarding suppressing, I'm not sure how I feel about it. On one hand, we want to fix those issues, not suppress them. On the other hand, lots of new users run into issues and making their path easier is important.

I don't want to discourage you from trying, but I'm not confident that a PR adding this feature would be merged.

Who is the we in this statement? Because the manifestation of this issue is that every consumer of a library with type errors is shouted at by flow, and it does not seem at all reasonable to suggest that all those users be expected to take responsibility for fixing upstream issues.

It's really just noise that distracts people from writing their own code - library maintainers will get their own errors if they use flow in their projects - it doesn't make sense to push those errors out to everyone.

[ffxsam]

I think I'm going to pull Flow from both of the projects I started to use it on. I'm just running into too many issues that wind up slowing me down rather than help me get my work done. I'll revisit later though, for sure.

IMO, I don't think there should be a separate project (flowtyped) to handle 3rd party libraries' typedefs. I think Flow should have some innate ability for library maintainers to export types and indicate to Flow (in a project using said libraries) how to find the types and incorporate them into the dev's project.

[Macil]

@ffxsam Libraries can include their own Flow types. It's not well documented but there's some information about how to do it here: #1996 (comment). (The flowtyped project is for libraries that don't bundle their own types.)

[mrkev]

@Diokuz, currently only @flow'd files are checked. Files can be ignored with [ignore]. If you want to still include node_modules in the type check but not see it's errors, you'll be able to use [declarations] once #4916 lands. I'll attempt the merge tomorrow.

[HunderlineK]

@mrkev Thanks for the followup. FYI, I have unexpected token , error in 'bower.json' of bcrypt, which is not a @flow'd file as far as I am concerned, and I do not even require it in a js file (it is required in a typescript file); nonetheless, I do not understand why this issue is closed if it is being addressed, even if partially, without at least proper label?

[mrkev]

@HunderlineK Idk man, I just hopped into this issue14 hours ago ¯\(ツ)/¯. I'm sure somewhere in the thread there's discussion about why it was closed.

You can [ignore] that broken json file btw.

[LegNeato]

For those still following this issue, in the next release of Flow I added a new [declarations] section. Here are the docs about it:

Often third-party libraries have broken type definitions or have type
definitions only compatible with a certain version of Flow. In those cases it
may be useful to use type information from the third-party libraries without
typechecking their contents.

The [declarations] section in a .flowconfig file tells Flow to parse files
matching the specified regular expressions in declaration mode. In declaration
mode the code is not typechecked. However, the signatures of functions, classes,
etc are extracted and used by the typechecker when checking other code.

Conceptually one can think of declaration mode as if Flow still typechecks the
files but acts as if there is a comment that matches
suppress_comment on every line.

I believe by leveraging [untyped], [ignore], and the new [declarations] section all problems mentioned in this issue can be resolved (once the release happens of course). 🎉

[levenleven]

all problems mentioned in this issue can be resolved

@LegNeato except the fact that flow will still walk through not relevant files (even just to check if marked with// @flow) and for big repos with multiple packages this still a problem. Ignoring every single dependency and dependencies of dependencies is not an option. The question is still not answered - why to parse not imported/ not relevant files?

[Macil]

#869 (comment)

currently only @flow'd files are checked

This isn't quite true: .json files are always checked for syntax correctness, which is annoying because many dependencies intentionally publish tests that contain broken .json files.

[mrkev]

why to parse not imported/ not relevant files?

@levenleven say you have 3 JS files: A, B and C. A has an import for B, and B has one for C.

• If you open B, it's easy to see that it depends on C and you should check that file-- information about A is not relevant.
• If you edit however B, you don't really care about C, but you do care about re-checking A, since whatever it's importing from B might have changed.

The only way to know what files should be re-checked is if you form a full dependency graph at the beginning of the type-checking process. Flow goes the safe route in including all files in this graph so you could change any file and all files that depend on it would be automatically re-checked.

Perhaps there's optimizations that could be made, sure, but I'd say that's a separate issue.

[LegNeato]

#869 (comment)

.json files are always checked for syntax correctness, which is annoying because many dependencies intentionally publish tests that contain broken .json files.

@agentme so ignore all test json and/or treat json as declarations:

[ignore]
**/(__)?tests?(__)?/*.json

[declarations]
 **/*.json

#869 (comment)

except the fact that flow will still walk through not relevant files (even just to check if marked with// @flow) and for big repos with multiple packages this still a problem. Ignoring every single dependency and dependencies of dependencies is not an option

@levenleven I assure you Flow has been used on bigger codebases than yours in many large monorepos. Also, the things to check vs not check are usually project dependent...Flow's default behavior + config option customization is pretty much the only good method to make sure no files are missed in the general case as @mrkev demonstrates. FWIW you might carve huge chunks off for your use-case by doing creative things in your config like:

[ignore]
**
[include]
**/*.js(on)?

---

So with the next Flow version we have:

• [ignore] - Have flow not parse
• [untyped] - Have flow parse but throw away types
• [declarations] - Have flow parse and use types but throw away errors

There are no known use-cases that can't be fixed with a combination of these.

The Flow defaults may not work out of the box for your project but that doesn't mean there is an issue here. Before [declarations] some fixes for use-cases were impossible. Please open other tasks with specific default behavior changes.

[levenleven]

@LegNeato Nice suggestion, but once you [ignore] ** nothing can be included. Seems that ignore beats the include. Limited regex options (no way to ignore all but 'foo') makes "ignoring" really annoying.

[kylebebak]

[declarations], although it's now a supported config section, doesn't actually work.

I'm running flow 0.76.0. 16 files in node modules don't pass the flow check. One of these files is /Users/kylebebak/Code/Work/919/web/node_modules/npm/node_modules/config-chain/test/broken.json.

None of the following patterns silences flow errors in this file (or any of the other 15 files, for that matter):

• /Users/kylebebak/Code/Work/919/web/node_modules/npm/node_modules/config-chain/test/broken\.json
• <PROJECT_ROOT>/node_modules/.*
• .*/node_modules/.*

So either the docs are wrong, or the feature was shipped but it's broken.

EDIT: So, as @LegNeato mentioned in #4916 , this isn't a problem with flow. I didn't understand the difference between ignore and declarations. Thank you!

[LegNeato]

I'll take a look and see what is up...in my testing it worked and it landed with tests. Maybe something is wonky. Thanks for trying it!

[kylebebak]

Thanks @LetNeato, I'm really looking forward to using this!

[rostislav-simonik]

exactly as @levenleven noticed. [ignore] section has the highest precedence. Would be great if every sections [include], [ignore], [libs], [declarations] has also option for exclusive rule like .gitignore ! prefix.

[Diokuz]

I think there is more problems with Cannot resolve module error) #5183

[ajitsen]

Most of my errors are gone by adding following to .flowconfig

[untyped]
.*/node_modules/**/.*

[FanchenBao]

Use [declarations] as mentioned in #869 (comment) and the document

[declarations]
<PROJECT_ROOT>/node_modules/.*

[untyped] treats extracts modules as any, instead of their actual types. This seems to negate the purpose of type checking in the first place.

[joan-saum]

Why is this issue closed? The problem has not been answered and has not been marked as won't fix.
Even though there is a new section to ignore errors from dependencies, flow is checking a lot of files which are not in the flow-typed, so it is unnecessary and takes some time.

[evbo]

Can we be given a simple option to just ignore all cannot-resolve-module errors? Like @joan-saum said, by using the new [untyped] and [declarations] sections it still causes potentially thousands of files to be checked unless you know precisely which .js files are needed, which is not always obvious.

cannot-resolve-module is not an especially useful check anyway since webpack will tell me that. So being able to just turn off those error messages completely is good enough work around for most.

[mondaychen]

Use [declarations] as mentioned in #869 (comment) and the document

[declarations]
<PROJECT_ROOT>/node_modules/.*

[untyped] treats extracts modules as any, instead of their actual types. This seems to negate the purpose of type checking in the first place.

Thanks @FanchenBao. This should be a best practice of all flow projects. I'm surprised it's not often mentioned in other places. I think at least the flow doc should just say .*/node_modules/.* instead of "./third_party/." since node_modules is most third party libs live