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

Provide a suppression option (to mute noisy 3rd-party node_modules) #3208

Closed
mosesoak opened this Issue Jan 18, 2017 · 10 comments

Comments

Projects
None yet
9 participants
@mosesoak

mosesoak commented Jan 18, 2017

Opening a new thread following the discussion at #869

There are certain cases where 3rd-party modules generate Flow errors, for example when a React Native library has baked their types and uses an outdated version of Flow. The reality is that although it's cyclical, there will probably always be cases where external modules bring their own errors.

Developers need a way to retain all of Flow's functionality but not be strapped with errors that fall outside their direct responsibility. The clearest path to doing this, suggested in the issue linked above, is to provide a simple suppression mechanism.

Here's a proposal for a way to easily limit the noise while leaving type-checking enabled, but get a clear indication of which modules contain breakage:

  1. Leave [ignore] as is, continue to type-check everything else.

  2. Add a suppress block to the config, maybe [quiet]. Folks can either list specific problem modules here, or node_modules/* depending on the current noise level.

  3. Make [include] override quiet. So even if you use * in quiet you can whitelist the ones you contribute to or trust to stay current.

  4. And finally, a concise but informative status summary in the output like,

    No errors.

    34 errors hidden by [quiet]: @exponent/ex-navigation (31), other (3).

This would provide a simple 'mute' option for the masses, that still provides enough information to prompt developers to solve the problems in the wider community over time.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jan 18, 2017

One data point to make the case for this feature is that React itself (indirectly) causes unavoidable Flow errors due to its fbjs dependency:

$ flow
node_modules/fbjs/lib/partitionObjectByKey.js.flow:20
 20:   return partitionObject(source, (_, key) => whitelist.has(key));
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tuple type. This type is incompatible with the expected return type of
 19: function partitionObjectByKey(source: Object, whitelist: Set<string>): Array<Object> {
                                                                            ^^^^^^^^^^^^^ array type


Found 1 error
$ yarn why fbjs
yarn why v0.20.0-0
[1/4] 🤔  Why do we have the module "fbjs"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
info Reasons this module exists
   - "react" depends on it
   - "react-dom" depends on it
info Disk size without dependencies: "5.36MB"
info Disk size with unique dependencies: "12.76MB"
info Disk size with transitive dependencies: "13.46MB"
info Amount of shared dependencies: 11
✨  Done in 0.89s.

The ideal outcome I think is for Flow to tell me that fbjs has internal Flow errors and then I can bug the maintainers of fbjs if I care about that, and in the meantime still have flow check pass and get an exit code of 0.

ide commented Jan 18, 2017

One data point to make the case for this feature is that React itself (indirectly) causes unavoidable Flow errors due to its fbjs dependency:

$ flow
node_modules/fbjs/lib/partitionObjectByKey.js.flow:20
 20:   return partitionObject(source, (_, key) => whitelist.has(key));
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tuple type. This type is incompatible with the expected return type of
 19: function partitionObjectByKey(source: Object, whitelist: Set<string>): Array<Object> {
                                                                            ^^^^^^^^^^^^^ array type


Found 1 error
$ yarn why fbjs
yarn why v0.20.0-0
[1/4] 🤔  Why do we have the module "fbjs"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
info Reasons this module exists
   - "react" depends on it
   - "react-dom" depends on it
info Disk size without dependencies: "5.36MB"
info Disk size with unique dependencies: "12.76MB"
info Disk size with transitive dependencies: "13.46MB"
info Amount of shared dependencies: 11
✨  Done in 0.89s.

The ideal outcome I think is for Flow to tell me that fbjs has internal Flow errors and then I can bug the maintainers of fbjs if I care about that, and in the meantime still have flow check pass and get an exit code of 0.

@peduarte

This comment has been minimized.

Show comment
Hide comment
@peduarte

peduarte Jan 19, 2017

Contributor

As a temp fix, I added .*/node_modules/fbjs/* to .flowconfig ignores.

Contributor

peduarte commented Jan 19, 2017

As a temp fix, I added .*/node_modules/fbjs/* to .flowconfig ignores.

@cpmech

This comment has been minimized.

Show comment
Hide comment
@cpmech

cpmech Jan 24, 2017

Same problem here:

$ create-react-app hello-world
$ cd hello-world
$ touch .flowconfig
$ flow
Launching Flow server for /home/me/temp/hello-world
Spawned flow server (pid=6302)
Logs will go to /tmp/flow/zShomezSmezStempzShello-world.log
node_modules/fbjs/lib/partitionObjectByKey.js.flow:20
 20:   return partitionObject(source, (_, key) => whitelist.has(key));
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tuple type. This type is incompatible with the expected return type of
 19: function partitionObjectByKey(source: Object, whitelist: Set<string>): Array<Object> {
                                                                            ^^^^^^^^^^^^^ array type


Found 1 error

cpmech commented Jan 24, 2017

Same problem here:

$ create-react-app hello-world
$ cd hello-world
$ touch .flowconfig
$ flow
Launching Flow server for /home/me/temp/hello-world
Spawned flow server (pid=6302)
Logs will go to /tmp/flow/zShomezSmezStempzShello-world.log
node_modules/fbjs/lib/partitionObjectByKey.js.flow:20
 20:   return partitionObject(source, (_, key) => whitelist.has(key));
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tuple type. This type is incompatible with the expected return type of
 19: function partitionObjectByKey(source: Object, whitelist: Set<string>): Array<Object> {
                                                                            ^^^^^^^^^^^^^ array type


Found 1 error
@mosesoak

This comment has been minimized.

Show comment
Hide comment
@mosesoak

mosesoak Jan 24, 2017

@ide Yes. That yarn example is a great illustration of how good software can surface information in a way that is helpful, not inherently frustrating.

Flow is a nice piece of engineering from a small core team, and the reality is that Facebook hasn't prioritized the developer experience since it's being used more as an internal build tool.

Coming from a team that has tried to use it despite this, we have found that it's valuable but frustrating due to its insistence on surfacing the details of problems we're not directly responsible for. From inside Facebook perhaps it makes sense since you're all sharing the responsibilities of the larger React ecosystem, but from out here it makes Flow pretty unusable.

We are looking into switching to TypeScript since that does have the developer-oriented tooling we are after, and it doesn't seem to be nearly as slow as it used to be in React Native.

mosesoak commented Jan 24, 2017

@ide Yes. That yarn example is a great illustration of how good software can surface information in a way that is helpful, not inherently frustrating.

Flow is a nice piece of engineering from a small core team, and the reality is that Facebook hasn't prioritized the developer experience since it's being used more as an internal build tool.

Coming from a team that has tried to use it despite this, we have found that it's valuable but frustrating due to its insistence on surfacing the details of problems we're not directly responsible for. From inside Facebook perhaps it makes sense since you're all sharing the responsibilities of the larger React ecosystem, but from out here it makes Flow pretty unusable.

We are looking into switching to TypeScript since that does have the developer-oriented tooling we are after, and it doesn't seem to be nearly as slow as it used to be in React Native.

@ericmasiello

This comment has been minimized.

Show comment
Hide comment
@ericmasiello

ericmasiello Jan 28, 2017

Contributor

Same problem here

Contributor

ericmasiello commented Jan 28, 2017

Same problem here

ericmasiello added a commit to ericmasiello/synbydesign that referenced this issue Jan 29, 2017

@crazy4groovy

This comment has been minimized.

Show comment
Hide comment
@crazy4groovy

crazy4groovy Feb 2, 2017

This is the best work-around solution I've found, for now:
#869 (comment)

crazy4groovy commented Feb 2, 2017

This is the best work-around solution I've found, for now:
#869 (comment)

@miracle2k

This comment has been minimized.

Show comment
Hide comment
@miracle2k

miracle2k Feb 10, 2017

This is a good proposal. I would add that the files from the quiet section should optionally not be checked at all for performance reasons, unless included by checked code.

miracle2k commented Feb 10, 2017

This is a good proposal. I would add that the files from the quiet section should optionally not be checked at all for performance reasons, unless included by checked code.

@gunar

This comment has been minimized.

Show comment
Hide comment
@gunar

gunar Nov 21, 2017

@facebook-github-bot This issue is still relevant and shouldn't have been closed.

gunar commented Nov 21, 2017

@facebook-github-bot This issue is still relevant and shouldn't have been closed.

@crazy4groovy

This comment has been minimized.

Show comment
Hide comment
@crazy4groovy

crazy4groovy commented Nov 24, 2017

Agreed

@haggholm

This comment has been minimized.

Show comment
Hide comment
@haggholm

haggholm Nov 24, 2017

The merged solution doesn’t solve the issue the way @LegNeato’s [silence] section did, does it? E.g., with [silence], Flow would check if my code comports with the types in imported modules (e.g., passing arrays, not strings, to lodash array functions), while suppressing any errors arising strictly within the libraries. With the accepted PR, it suppresses those errors, but also fails to check if my code is calling correctly into libraries.

haggholm commented Nov 24, 2017

The merged solution doesn’t solve the issue the way @LegNeato’s [silence] section did, does it? E.g., with [silence], Flow would check if my code comports with the types in imported modules (e.g., passing arrays, not strings, to lodash array functions), while suppressing any errors arising strictly within the libraries. With the accepted PR, it suppresses those errors, but also fails to check if my code is calling correctly into libraries.

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