extend genericSpacing to cover funtion annotations

[hsimah]

I put this together - it's ended up being fairly complex, so if I missed the mark let me know.

It works if I call eslint . --fix on my code, but the tests are failing saying the expected rule is not throwing. If I deploy this branch to my project's package.json I get all the warnings and fixers.

Fixes #24

[Brianzchen]

Thanks, looks like some good effort here initially. Some input here would be,

• I thought we were fixing the rule genericSpacing instead of creating a new rule. I find the name typeAnnotationSpacing confusing because what is a type annotation, generics are actually what you're covering
• You'll need to add to readme (if it's really meant to be a new rule) https://github.com/flow-typed/eslint-plugin-ft-flow/blob/master/CONTRIBUTING.md#adding-documentation
• The tests should all pass and not be commented out, especially if you're adding a new rule they should be commented from the very beginning.
• The fact that it works in your build but not project but not in testing is so dodgy... Can you give some insight into what eslint version you're running for my own reference 😅

[hsimah]

• I thought we were fixing the rule genericSpacing instead of creating a new rule. I find the name typeAnnotationSpacing confusing because what is a type annotation, generics are actually what you're covering

Yeah, agreed. I will merge the rules.

• You'll need to add to readme (if it's really meant to be a new rule) https://github.com/flow-typed/eslint-plugin-ft-flow/blob/master/CONTRIBUTING.md#adding-documentation

Can do.

• The tests should all pass and not be commented out, especially if you're adding a new rule they should be commented from the very beginning.

Ah, forgot to mention there seems to be an issue in the underlying test runner. When I add <?string> it crashes. Interestingly enough the array long form tests (Array<?string>) don't have the same issue. I will fix that and uncomment those tests before we ship this. We can still hone the rule code itself while I work on that.

• The fact that it works in your build but not project but not in testing is so dodgy... Can you give some insight into what eslint version you're running for my own reference

Yeah, scratching my head at that too. I will check my linting version, mostly I use CRA + Relay + Flow outside of work (where all this stuff magically works for me).

[Brianzchen]

When I add <?string> it crashes

Amazing, that's so interesting that this use case was never tested. I'll try look into a fix too but if you find one feel free to contribute it

[Brianzchen]

Also you can remove package-lock.json and why idea why there was a large yarn.lock diff? Not a biggie we can cleanup later

[Brianzchen]

We should remove this now?

[hsimah]

yep, forgot to stage that change

[hsimah]

I am still seeing failures when the lint tester tries to parse the ?. it's weird because the array rule contains question marks, though those are type declarations and not function calls. I'll keep fiddling with it.

[Brianzchen]

Yeah I've just spent the better part of my day trying to figure this one out. I'm a bit confused.. I dug into the code and in the end RuleTester calls @babel/eslint-parser which is also what is used when in production but syntax like useState<?string>() doesn't cause eslint to crash in those instances.

At this rate if we can't solve it but your improvement works i'm ok to merge it as an alpha release initially whenever you think your change is ready

[Brianzchen]

This should say end of type annotations

[Brianzchen]

Suggested change

	errors: [{ message: 'There must be no space at start of type annotations' }],
	errors: [{ message: 'There must be no space at end of type annotations' }],

[Brianzchen]

Suggested change

	errors: [{ message: 'There must be no space at start of type annotations' }],
	errors: [{ message: 'There must be no space at end of type annotations' }],

[Brianzchen]

Could you add a test that's something like the following if it doesn't already exist elsewhere I'm interested to know if it strips all spaces blindly or only when it's between maybe types ? and arrow brackets </>

    {
      code: 'useState< string | number >(null)',
      errors: [{ message: 'There must be no space at start of type annotations' }],
      output: 'useState<string | number>(null)',
    },

[Brianzchen]

Ok found something. I believe this is unrelated to the test runner itself. If you go to any file in src and add useState<?string>(null) it will throw the same unexpected token error. So it must be the way the eslint/babel is configured on this project. At least there's a way to easily test and fix now

[Brianzchen]

Right as it turns out, it's because it's not parsing the token correctly unless you have // @flow\n at the start of your code snippet, can you try that and let me know?

[Brianzchen]

This is a guess but I'm imagining it has something to do with how TS doesn't have maybe types so flow token parsing doesn't actually run with babel unless the flow pragma is first detected.

[hsimah]

I was looking at the TS side of things, so I think you are right. I will pull out my laptop tonight after work and action your comments here. Thanks for the time - it feels nice to use Flow at home :)