Fix: camelcase false positives on interface properties (fixes #177)

[weirdpattern]

Do not merge just yet.

I'm submitting this so we can discuss a few things that are bothering me...

1. The base ESLint rule is supposed to validate camelCase (emphasis on the casing), but the below code is not reported as an error...

var Test = "test";

eslint/eslint#10473 seems to discuss this issue in detail...

2. Properties in classes present the same behavior as interfaces, but I'm not sure if this should be fixed here or in the base rule as JavaScript accepts class properties via a babel transformation...

class Foo extends React.Component {
   state = { ... }
}

3. The ESLint version used by the plugin is dated, so the camelcase rule has new options that are not available to the plugin. I'm including these new options in the documentation along with a note explaining support for some of the options will depend on the ESLint version used.

Thoughts?

[bradzacher]

1)

The hard thing about explicitly differentiating camelCase vs PascalCase is that conventions states generally that classes (and react functional component variables) use PascalCase, and so do interfaces and generics.
Variables and properties use camelCase. Though class properties have the extra exception of the private convention of a leading underscore.

So if you want a rule that only allows camelCase or PascalCase, then you have to add config options for every single thing.
Doable, just a PITA to configure 😅

If that's the route we want to go, it may be better to diverge from eslint, and add one rule per case, instead of one rule with a config per case?
That might be easier for users configure... (and would keep code simpler)

2)

I think we should add support for it in our codebase for now.
Typescript support them, the parser supports them, so our users will expect support for them!

Class properties are still stage 3 (https://github.com/tc39/proposal-class-fields), so I don't think eslint will add support until they are stage 4 (aka actually released).

We'll just have to monitor the base implementation to see when we can delete code I guess.

As an aside on this point - don't forget about constructor arguments: TSParameterProperty.
Technically is a property, so should follow the property-based settings.

3)

I'm fine with bumping the eslint version. The breaking changes in v5 shouldn't affect our tests.
If you're worried about it at all, we can always do that as a separate PR.

[weirdpattern]

Well we already have a rule to validate PascalCase in classes and interfaces, might as well create new rules for the rest... if we decide to go this route, what would happen to the camelcase rule? wouldn't this be in direct conflict with the new rules?

That's what I thought, adding support for these (and any other case I can think of) tonight.

Bumping the version has no real impact on this rule, so I would prefer to do it in a separate PR...
I will create a new chore... on that note, is there a reason why eslint and the parser are dev-dependencies and not peer-dependencies?

[bradzacher]

1)

hmmm.. looking back over the rules, we have this handled in a very mish-mash way.
We have:

• class-name-casing which only allows PascalCase for classes AND interfaces.
• member-naming which uses regex on specific modifiers (only private atm).
• generic-type-naming which uses regex.
• interface-name-prefix which just allows or blocks I at the start of interface names.

So we're already going down the path of one rule per case...
For the 1.0.0 release, we should probably address this and bring them all into some semblance of order:

• standardise naming
• standardise base functionality (free regex vs fixed settings)
• one rule per type (split interface out of class-name-casing / merge it into interface-name-prefix).

I should probably open a new issue to discuss this 😅
Probably beyond the scope of this PR!

2)

👍

3)

Go for it!

I don't know why exactly.
I heard of some weird things eslint does with dependencies that aren't root dependencies, though I've not seen it in newer versions.

At the very least, they should be peer dependencies.

For the 1.0.0 release I was thinking of making the parser a direct dependency, and eslint a peer dependency (so long as that all works...!)

[weirdpattern]

I agree... this should be a separate bug...

I will add the missing cases to this rule, so we can close it... we can deal with the refactoring and new rules at a later time (maybe even after 1.0.0)...

[bradzacher]

LGTM!

[weirdpattern]

I will wait until #186 gets merged to update the branch and do the merge