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

Prevent reading all node_modules when using [untyped] #7431

Closed
vjpr opened this issue Feb 1, 2019 · 6 comments
Closed

Prevent reading all node_modules when using [untyped] #7431

vjpr opened this issue Feb 1, 2019 · 6 comments

Comments

@vjpr
Copy link

vjpr commented Feb 1, 2019

I want import 'foo/bar' to not throw "cannot find module" errors, but want to prevent the server looking at all my node_modules.

With the current options there is no way to get this to work.

a.

[untyped]
.*/node_modules/.*

Problem: Dramatically slower server startup.

This makes the server scan all my node_modules. I have ~3000 project files, and 200K files in node_modules.

This means the server takes 1 minute to start versus instantly.

Instead of needing to read and watch every node_module file to ensure import 'foo/bar' is valid, just check whether the files exist during type checking process. But I would be fine with just ignoring the check of whether my imports are valid for node_modules too.

b. ignore

[ignore]
.*/node_modules/.*

Problem: "Cannot find module" errors for any import foo from 'foo'.

c. ignore all node_modules except first level of dirs

(Ignore all node_modules dirs except the first)

[ignore]
.*/node_modules/.*/.*/.*

# Needed for pnpm because Flow tests regexes on realpaths - see #7429 
.*/node_modules/\.registry\.npmjs\.org/.*/.*/node_modules/.*/.*/.*

Problem: import 'foo/bar' doesn't work.

d. declarations

[declarations]
.*/node_modules/.*

Problem: Still slow server startup - scans every file.

Problem: Errors are shown for all 3rd party modules with type annotations of which there are usually hundreds broken (e.g. graphql@0.13.2). This will happen perpetually because there will always be deprecations and new features, and flow doesn't have a way to version type annotations.

e. name_mapper for all non-relative requires

[ignore]
.*/node_modules/.*

[options]
# Match `import 'foo'` but not `import './foo'`.
module.name_mapper='^[A-Za-z]+.*' -> '<PROJECT_ROOT>/any.js.flow'

any.js.flow

// @flow
declare module.exports: any;

Doesn't scan or watch any node_modules. All package imports will be replaced with any type - just liked [untyped] does.

Problem: This doesn't work with a monorepo where import foo from 'foo' could refer to a symlinked package.

Problem: It's not flexible.


Solution

Add an option module.system.node.silence_module_not_found_errors_in_ignore_paths or something like that.

Related

a lot of us want type processing in node modules but NO errors shown in any node modules. We want errors to be shown in our own code when we missuse a module, but we don't want to see that any node modules did something wrong internally. We want to be able to maintain code that is error free and see that on a PR there are no flow errors, but we can't do that when node modules reports hundreds of errors for dependencies I can't control.

@vjpr vjpr added the discussion label Feb 1, 2019
@vjpr vjpr changed the title ignore and untyped Prevent reading all node_modules Feb 1, 2019
@vjpr vjpr changed the title Prevent reading all node_modules Prevent reading all node_modules when using [untyped] Feb 1, 2019
@GunnarAK
Copy link

GunnarAK commented Oct 22, 2019

One could do the following;

  1. add the library you want to ignore to [ignore]

  2. then create a JS-file with similar name to library in the flow-typed folder.
    with the content:

// @flow
declare module "<LIBRARY_NAME>" {
  declare module.exports: any;
}

@Brianzchen
Copy link
Contributor

Standard is just to use flow-typed which can help you stub any definitions that don't exist in flow-typed. Then you can safely ignore all of node_modules which solves performance issue and module resolution.

@murrayju
Copy link

@Brianzchen I don't understand why that would be the standard recommendation. This would prevent library authors from including useful Flow type definitions in their npm packages. Surely this could only hurt flow's adoption?

In my experience, flow-typed is useful but not adequately complete.

@Brianzchen
Copy link
Contributor

flow-typed will solve the immediately issue of performance and not being able to resolve anything.

I should have been more thorough in explaining though. You're right this would stop library developers from shipping flow types, which is why flow also comes with exclusions for ignores which you can read about here, https://flow.org/en/docs/config/ignore/ (at the bottom of the page)

So your flow config would look something like

[ignore]
<PROJECT_ROOT>/node_modules/.*
!<PROJECT_ROOT>/node_modules/not-excluded-package-A/.*
!<PROJECT_ROOT>/node_modules/not-excluded-package-B/.*

# other stuff...

To my knowledge flow has no way of knowing if a package ships with flow types, so it has to be a manual process. My recommendation is that if flow-typed stubs the library, you should check if it ships with flow types.

Hope this was an appropriate explanation.

@murrayju
Copy link

Thank you, that is helpful.

Still, though, this seems like a workaround, not a solution. Manually declaring how to handle the types for every dependency is tedious, if nothing else. Again, this feels like a barrier to adoption.

I have found that using [declarations] to scan my node_modules works exactly how I would like, except that it is very slow.

It seems that there should be some solution that flow could implement to improve the performance. Perhaps this needs to be a separate ticket, but it seems very important to me.

@evbo
Copy link

evbo commented Feb 28, 2021

@vjpr @murrayju it's too bad this issue was closed as I agree with you both there should be just a single line of config to address this issue.

Here's my workaround, 2 lines of config per node_module you need to import:

[ignore]
<PROJECT_ROOT>/node/.*
<PROJECT_ROOT>/node_modules/.*
<PROJECT_ROOT>/src/__tests__/.*
<PROJECT_ROOT>/webpack.config.js
<PROJECT_ROOT>/pom.xml
<PROJECT_ROOT>/.babelrc
<PROJECT_ROOT>/frontend.iml
# to avoid import errors, parse only necessary modules per: https://github.com/facebook/flow/issues/7431
!<PROJECT_ROOT>/node_modules/react/index.js
!<PROJECT_ROOT>/node_modules/react-dom/index.js

[options]
# no need to add // @flow to every file, by default it parses all with flow
all=true

[declarations]
<PROJECT_ROOT>/node_modules/react/index.js
<PROJECT_ROOT>/node_modules/react-dom/index.js

[untyped]

[include]

[libs]

[lints]

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

No branches or pull requests

5 participants