Combine define & use-flowType rules

[danez]

I'm currently trying to figure out what is the best solution to go forward with babel-eslint 7.x and this plugin.

We want to make the next babel-eslint version not to define or use any flow type anymore. When I pull in the dev version of babel-eslint into this plugin the whole test-suite collapses for the define & use rule. It is pretty straight forward to fix most of the cases, but I stumbled upon cases where it is now hard to test, because the two plugins are split.

Assume this example:

function x<A>() {}; x()

Previously this was always failing (A is defined but never used) without any rule. Now babel-eslint is not doing anything to A so this does not trigger any error by default.

What the tests should cover now is

• if define-flow-type is enabled then it should fail
• if define-flow-type and use-flow-type is enabled it should still fail

Now the first is easy, but for the second assertion there is currently no location to test that.
This could be solved by either adding a new testfile that tests both rules together, includes the define-rule in the use-test or by combining both rules into one.

The two rules were usually always used together afaik, but at least with the new babel-eslint version it will be nearly always necessary to use both of them when using flowtype in your code and especially when linting of flowtype is desired. So that brought up the question in my head if it makes sense to have two rules at all. Although both rules have a single responsibility right now, I think it would maybe still make sense to combine them and the underlying code could still be split into use/define.

But before I start creating a PR I wanted to get feedback and hear what you think about it @gajus
?

[gajus]

Hi @danez Thank you for raising the issue.

The authority on this subject is @zertosh. He has written the implementation of both rules (#36).

@zertosh what are your thoughts about this?

[zertosh]

The reason for keeping them separate was because use-flow-type only makes sense if you use library definitions. If you don't use library definitions, then enabling use-flow-type would hide legitimate cases of unused vars.

If babel-eslint 7.x isn't going to update the scope anymore, then it makes total sense to merge the rules, since, yeah, you'll always want them together. I like that babel-eslint is slimming down, but there's a very real need for scope information on flow types. I can see how a plugin could easily either update the scope, or maintain its own scope information. @danez do you know if the eslint team has any opposition against exposing estraverse on the context object?

[danez]

I'm not part of eslint so I'm not sure what the think about exposing estraverse, maybe @hzoo knows.
But I can't really follow: why would you want to have estraverse in the context? Isn't the scope managed by escope? As far as I see getScope() returns the current scope, which is perfect for some cases. But there is probably also the need to create own scopes, right? In the case like declare module {} which is it's own scope I think, but not sure.

[zertosh]

why would you want to have estraverse in the context?

So I can do my own traversal before the rule visitors are called and augment the scope. I can install my own, but it'd be nice to use the same machinery that eslint already has.

[migueloller]

Would it be possible to expand define-flow-type to check for type declarations in flow-typed directories and add all the Flow built-in types to avoid false-positives?

Maybe a better option would be to make an ESLint env that includes all Flow built-ins as globals and then a way to define directories or files that contain type declarations (similar to how import/resolver works).

[gajus]

Closing as stale.