feat(eslint-plugin): add class-literal-property-style rule

[G-Rath]

Closes #1556

While I see the value in this rule, I think it'll be misleading to have a default value as I think neither style is Strictly Better.

For now I've typed the rule as having a secret any style that just does nothing, which I've omitted from the schema.

I originally had it as an error to force you to provide a style, but that's not a convention I've seen around - Very much open to ideas on how to handle the configuration of this rule.

Also open to docs suggestions - I aimed for passable, but the wording is a little hamfisted (in particularly at the start) 😬

---

I've decided to leave arrays, objects, and functions off the docket as theres some interesting implications & subtle differences in the behaviours of ways you could refactor them to follow this rule; For example:

class Mx {
  private readonly arr = [];
}

While the property is readonly, the array isn't, and so could be mutated; getters style would have the class written like so:

class Mx {
  private get arr() {
    return [];
  }
}

Which means a new array will be created every time, effectively making the array itself readonly, which might not be desired.

An off-the-cuff fix that could be suggested might be to use a variable:

const arr = [];

class Mx {
  private get arr() {
    return arr;
  }
}

The gotcha with this is that in the original example a new array was initialised along with the class - now the array is initialised upon definition, and the field is assigned a reference to the array.

This means that all instances will share - and mutate - the same array.

This was a rabbit hole I decided not to go down for now, given that I think the primary reasons behind why this rule was requested is met, and that this can be easily added to down the line if it comes up.

Including them is just a matter of removing the isSupportedLiteral check; I believe that readonly types should be fine (i.e readonly string[]) but that would require type checking.

[bradzacher]

I haven't reviewed the code yet, but

While I see the value in this rule, I think it'll be misleading to have a default value as I think neither style is Strictly Better.

IMO readonly prop = value is strictly better, because it means that whenever you access the value, JS does not need to setup and teardown a function closure etc.

Instead of creating a special any type, I would suggest just making it an optional tuple element.

type Options = [('fields' | 'getters')?];

[G-Rath]

@c32hedge cheers, those were good catches 😅

@bradzacher

IMO readonly prop = value is strictly better, because

I agree with you, except that as @sindresorhus said in the original issue, if you're making a library for JS it's possible for people to override such literals.

Thinking about it some more I think I'd be happy to make fields the default, as while getters has valid use for when writing libraries, I think it's more common to be writing applications (or generally, unpublished/private code) and so code that remains generally in the TypeScript domain where the readonly can be enforced.

Instead of creating a special any type, I would suggest just making it an optional tuple element.

Very clever - that's much better than having a special type 😄

I'll use the optional tuple, but given the above I'm happy to set a default if you'd like 🙂

---

Also, the build failed due to missing lerna 😬

/bin/bash --noprofile --norc /home/vsts/work/_temp/4d64e216-0340-4469-a907-e1ae8d3d78a2.sh
yarn run v1.21.1
$ lerna run typecheck
/bin/sh: 1: lerna: not found
error Command failed with exit code 127.

[bradzacher]

a few comments, otherwise LGTM.
Thanks for working on this!

[bradzacher]

no support for TaggedTemplateLiteral?

Would probably good to support them for things like graphql tagged strings:

readonly query = graphql`
  query {
    # ...
  }
`;

[G-Rath]

Very good point! I've added support, with the same bailout on if they've got any expressions since we'd have to scope-check them which is probably overkill (unless I've missed the magic method that checks exactly that?).

I've also not had the fixer bother trying to make them look nicer by having them multi-line, as that feels like more overkill as we'd have to try to calculate indentation.

Let me know what you think :)

[bradzacher]

stylistically, do you think more people enforce spaces inside function body?

get foo() {return 1}
// or
get foo() { return 1; }

I think more people do the latter, so it might be good to do it to save the majority having to edit it afterward?

I use prettier in everything, so it's no skin off my nose. I'm happy with whatever you want to do.

[c32hedge]

I don't know how interactions between rules work, but how would this interact with the block-spacing eslint rule? Same question about the semi rule for the return statement.

Sorry @bradzacher, I just wasn't clear from your comment if these other rules would fix this if they are set based on your remark about "having to edit it afterward".

[bradzacher]

The docs explain this better than I can:

https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

After applying fixes, ESLint will run all of the enabled rules again on the fixed code, potentially applying more fixes. This process will repeat up to 10 times, or until no more fixable problems are found. Afterwards, any remaining problems will be reported as usual.

Best practices for fixes:
\4. Since all rules are run again after the initial round of fixes is applied, it's not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule.

One exception to this is fixers applied via an IDE - fixing a single error via the IDE will run the fixer exactly once.

Which is why when applying larger fixes like this, I still like to try and go with what I believe to be the "most common" styling, so there's less chance of the fix causing a lint error, and so people are less likely to want to fix it up manually afterward.

[G-Rath]

One exception to this is fixers applied via an IDE - fixing a single error via the IDE will run the fixer exactly once.

That's a good point - I'm not particularly fussed, and do prefer the nicer output in the tests anyway, so +1

[bradzacher]

Considering you're pretty much replacing the entire node, I would just simplify this and actually replace the entire node:

let text = printNodeModifiers(node, 'readonly');
const name = sourceCode.getText(node.key);
if (node.computed) {
  text += `[${name}]`;
} else {
  text += name;
}
text += ` = ${sourceCode.getText(argument)};`;
return fixer.replace(node, text);

[G-Rath]

Happy to do so, but note that replacing the entire node means that any comments would be lost:

readonly /* public */ p1 = 'hello world'

[bradzacher]

tbf, you're nuking the majority of the comments anyways 😄

/* */ public /* */ static /* */ readonly /* */ p1 /* */ = /* */ 'hello world' /* */;

I think your current code only keeps the outer two comments.
Replacing the node would also only keep the outer two comments.

If someone comes later on to complain about missing comments, we can reassess the fixer - I'm not fussed about killing comments in really weird places.

[G-Rath]

I'm not fussed about killing comments in really weird places.

That's my feelings too :)

[bradzacher]

same comment as the above fixer - might be better to just replace the entire node.

[bradzacher]

LGTM - thanks for doing this.

One question before merging:
Do you think this rule would be better named as one of:

• class-literal-property-style
• class-property-literal-style
• literal-class-property-style
• class-readonly-property-style
• readonly-class-property-style

I'm wondering if just "literals" by itself is too vague to convey intention?

[G-Rath]

Yeah I was wondering that too.

I don't think that the rule name should include readonly, since I feel that has different connotations vs what this rule actually does.

I think class-literal-property-style is probably the way to go. I'll do a rename.

On an aside: do you have any opinions on plural vs singular rule names? it's another one of those things I've hit a couple of times where neither seems to strongly win out vs the other to me :/

[bradzacher]

do you have any opinions on plural vs singular rule names

I think that as long as the name makes sense it doesn't really matter.
I probably lean toward the singular where possible, but sometimes there's just this gut "it feels better" one way or the other 😅
Making it read like a sentence is always a good way to go.
Using the context of what the rule does is another good option.

Examples from eyeballing our rule list:

• space-before-function-paren - makes more sense as singular, because it's about the space before a single parenthesis.
• quotes - makes more sense as plural, because you have quotes in pairs, and it's about both quotes.
• no-magic-numbers / no-unused-expressions / no-unused-vars all could make sense as singular or plural, but IMO make more sense plural, because it reads more like a sentence.
  • In the same vein
    • no-empty-function would probably be better as plural.
    • no-extra-semi is okay as single, because semi is an abbreviation, and semis sounds weird.
• unified-signatures makes more sense plural because it's only going to operate on multiple signatures, never on a single.