Suggested improvements to no-flow-fix-me-in-strict-files rule

[mrtnzlml]

I think this rule is a really good idea. I have a suggestion for improvements though before I can enable it in our codebase. For example, this should be an error (I actually thought this is what the rule is supposed to catch):

// @flow strict

export const test: $FlowFixMe = 42;

Flow is happy, Eslint rule is happy (it should not be). Another example which I think is wrong is this:

// @flow strict

// $FlowExpectedError[incompatible-type]
export const test: string = 42;

It's wrong because it's a Flow test (__flowtests__/test.js) and the error is expected to happen ($FlowExpectedError). In other words, it's correct that Flow complains here (I marked it as expected) but it doesn't mean the file cannot be strict. This case is actually very common in our codebase where we have many strictly typed flow test files. It would be correct if the suppression was for example $FlowFixMe or $FlowIssue.

What do you think about this?

[Brianzchen]

So for the first one, you're naming a type variable as $FlowFixMe? What use case do you have to do that? I believe this rule is made to handle suppression errors (aka flowfixme) in comments.

For your second case, you've made this file strict though you want to have suppression errors which is an anti pattern as per the docs. Any chance you'd consider removing strict mode on these flow tests you run, maybe it doesn't make sense there?

Or maybe the rule just doesn't make sense for your codebase which is fine since it's not part of recommended config 🙂

Tagging original author in case he has any ideas @nvie since tbh I don't use strict mode.

[mrtnzlml]

So for the first one, you're naming a type variable as $FlowFixMe? What use case do you have to do that? I believe this rule is made to handle suppression errors (aka flowfixme) in comments.

In this case $FlowFixMe is a special type and it basically means any. See: https://flow.org/en/docs/config/options/#toc-suppress-type-string

I didn't realize it's not built-in like other suppressions so I think this should be configurable.

For your second case, you've made this file strict though you want to have suppression errors which is an anti pattern as per the docs. Any chance you'd consider removing strict mode on these flow tests you run, maybe it doesn't make sense there?

I agree with the statement for normal files. But the docs are also saying "just add @flow strict once all issues have been resolved" which is never the case in flowtests. The whole point of flowtests is to throw errors and mark them as expected via $FlowExpectedError. That doesn't mean the file cannot follow strict lint rules though. Flowtests are a special case in my opinion.

[Brianzchen]

oh wow learning something new everyday, suppress_type is a cool idea!

On the point of this rule it should not handle usage of suppress_type that goes beyond the specs of the rule and we should be careful it doesn't become a catchall. In fact I would think because it's an alias to any it can be an option that's added to no-weak-types where it can accept an array of types that your project has configured reflecting .flowconfig.

For your suppression errors in flow strict files we can add an option to this rule that takes an array of ignored dirs?

Let me know what you think of these ideas.

[mrtnzlml]

I have another improvement to this rule before I address your comments: I think the rule name no-flow-fix-me-in-strict-files is not great. Something like no-flow-suppressions-in-strict-files would be more accurate (it's not only about "flow fix me").

I would think because it's an alias to any it can be an option that's added to no-weak-types where it can accept an array of types that your project has configured reflecting .flowconfig.

Here is my point of view: no-weak-types is a great rule and we are using it to avoid usage of Object and Function types. However, we allow any because sometimes you simply want anything or you want to optimize. No need to go into whether it's good or bad, that's how we do it. For the same reason, I am not planning on disabling suppress_type=$FlowFixMe because it has its use-cases.

On the other hand, Flow doesn't allow any in strictly typed files and I want to follow the same logic with types defined in suppress_type. That's why I think it would be a good addition for this rule (in other words, take into account not only suppress comments but also suppress types). At this moment there is no way how to prevent this.

For your suppression errors in flow strict files we can add an option to this rule that takes an array of ignored dirs?

I thought that ignoring $FlowExpectedError should be enough because, well, it's "expected". But yeah, it should be fine as long as I can ignore all flowtests (https://github.com/facebook/flow/blob/36eca27dab527f4f300c1a725e39aad5a04dd41e/packages/flow-dev-tools/src/comment/remove-commentsRunner.js#L84-L92) in the whole codebase easily. 🙂

[Brianzchen]

I have another improvement to this rule before I address your comments: I think the rule name no-flow-fix-me-in-strict-files is not great. Something like no-flow-suppressions-in-strict-files would be more accurate (it's not only about "flow fix me").

yes I had the same thought.. Maybe worth opening another issue for this I'm not sure of the impacts of changing it, it would be a breaking change for anyone using it. Though could be a now or never update since this lib isn't that hugely used yet...

I thought that ignoring $FlowExpectedError should be enough because, well, it's "expected". But yeah, it should be fine as long as I can ignore all flowtests

The alternative is to follow the pattern of no-weak-types and by default all suppression errors will error but you can turn them off like [2, { '$FlowExpectedError': false }]. I like this idea more because basically it gives more value to the various suppression error types where right now they all have the same effect.

I'm happy to implement this one if it suits you?

[Brianzchen]

Additionally to this option object we could add flowTests property which defaults to being false which is fair since it's for testing but if a user wants to change it to true, they can do that also.

Overall the rule structure would be used like

'ft-flow/no-flow-fix-me-in-strict-files': [2, {
  $FlowExpectedError: false, // you'd disable this
  flowTests: false, // this is the default value but could be changed to true which means to throw error
}]

[mrtnzlml]

Maybe worth opening another issue for this...

Done: #17

The alternative is to follow the pattern of no-weak-types and by default all suppression errors will error but you can turn them off like [2, { '$FlowExpectedError': false }]. I like this idea more because basically it gives more value to the various suppression error types where right now they all have the same effect.

Sounds good to me. 👍

Additionally to this option object we could add flowTests property...

This might not be necessary once we can do [2, { '$FlowExpectedError': false }]. 🤔