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

G-Rath · web-flow · commit b2dbd890a5be · 2020-03-22T22:10:17.000-07:00
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
@@ -99,6 +99,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
 | [`@typescript-eslint/await-thenable`](./docs/rules/await-thenable.md)                                                 | Disallows awaiting a value that is not a Thenable                                                                       | :heavy_check_mark: |          | :thought_balloon: |
 | [`@typescript-eslint/ban-ts-comment`](./docs/rules/ban-ts-comment.md)                                                 | Bans `// @ts-<directive>` comments from being used                                                                      |                    |          |                   |
 | [`@typescript-eslint/ban-types`](./docs/rules/ban-types.md)                                                           | Bans specific types from being used                                                                                     | :heavy_check_mark: | :wrench: |                   |
+| [`@typescript-eslint/class-literal-property-style`](./docs/rules/class-literal-property-style.md)                     | Ensures that literals on classes are exposed in a consistent style                                                      |                    | :wrench: |                   |
 | [`@typescript-eslint/consistent-type-assertions`](./docs/rules/consistent-type-assertions.md)                         | Enforces consistent usage of type assertions                                                                            | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/consistent-type-definitions`](./docs/rules/consistent-type-definitions.md)                       | Consistent with type definition either `interface` or `type`                                                            |                    | :wrench: |                   |
 | [`@typescript-eslint/explicit-function-return-type`](./docs/rules/explicit-function-return-type.md)                   | Require explicit return types on functions and class methods                                                            | :heavy_check_mark: |          |                   |
diff --git a/packages/eslint-plugin/docs/rules/class-literal-property-style.md b/packages/eslint-plugin/docs/rules/class-literal-property-style.md
@@ -0,0 +1,96 @@
+# Ensures that literals on classes are exposed in a consistent style (`class-literal-property-style`)
+
+When writing TypeScript applications, it's typically safe to store literal values on classes using fields with the `readonly` modifier to prevent them from being reassigned.
+When writing TypeScript libraries that could be used by Javascript users however, it's typically safer to expose these literals using `getter`s, since the `readonly` modifier is enforced at compile type.
+
+## Rule Details
+
+This rule aims to ensure that literals exposed by classes are done so consistently, in one of the two style described above.
+By default this rule prefers the `fields` style as it means JS doesn't have to setup & teardown a function closure.
+
+Note that this rule only checks for constant _literal_ values (string, template string, number, bigint, boolean, regexp, null). It does not check objects or arrays, because a readonly field behaves differently to a getter in those cases. It also does not check functions, as it is a common pattern to use readonly fields with arrow function values as auto-bound methods.
+This is because these types can be mutated and carry with them more complex implications about their usage.
+
+#### The `fields` style
+
+This style checks for any getter methods that return literal values, and requires them to be defined using fields with the `readonly` modifier instead.
+
+Examples of **correct** code with the `fields` style:
+
+```ts
+/* eslint @typescript-eslint/class-literal-property-style: ["error", "fields"] */
+
+class Mx {
+  public readonly myField1 = 1;
+
+  // not a literal
+  public readonly myField2 = [1, 2, 3];
+
+  private readonly ['myField3'] = 'hello world';
+
+  public get myField4() {
+    return `hello from ${window.location.href}`;
+  }
+}
+```
+
+Examples of **incorrect** code with the `fields` style:
+
+```ts
+/* eslint @typescript-eslint/class-literal-property-style: ["error", "fields"] */
+
+class Mx {
+  public static get myField1() {
+    return 1;
+  }
+
+  private get ['myField2']() {
+    return 'hello world';
+  }
+}
+```
+
+#### The `getters` style
+
+This style checks for any `readonly` fields that are assigned literal values, and requires them to be defined as getters instead.
+This style pairs well with the [`@typescript-eslint/prefer-readonly`](prefer-readonly.md) rule,
+as it will identify fields that can be `readonly`, and thus should be made into getters.
+
+Examples of **correct** code with the `getters` style:
+
+```ts
+/* eslint @typescript-eslint/class-literal-property-style: ["error", "getters"] */
+
+class Mx {
+  // no readonly modifier
+  public myField1 = 'hello';
+
+  // not a literal
+  public readonly myField2 = [1, 2, 3];
+
+  public static get myField3() {
+    return 1;
+  }
+
+  private get ['myField4']() {
+    return 'hello world';
+  }
+}
+```
+
+Examples of **incorrect** code with the `getters` style:
+
+```ts
+/* eslint @typescript-eslint/class-literal-property-style: ["error", "getters"] */
+
+class Mx {
+  readonly myField1 = 1;
+  readonly myField2 = `hello world`;
+  private readonly myField3 = 'hello world';
+}
+```
+
+## When Not To Use It
+
+When you have no strong preference, or do not wish to enforce a particular style
+for how literal values are exposed by your classes.
diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json
@@ -8,6 +8,7 @@
     "@typescript-eslint/ban-types": "error",
     "brace-style": "off",
     "@typescript-eslint/brace-style": "error",
+    "@typescript-eslint/class-literal-property-style": "error",
     "comma-spacing": "off",
     "@typescript-eslint/comma-spacing": "error",
     "@typescript-eslint/consistent-type-assertions": "error",
diff --git a/packages/eslint-plugin/src/rules/class-literal-property-style.ts b/packages/eslint-plugin/src/rules/class-literal-property-style.ts
@@ -0,0 +1,136 @@
+import {
+  AST_NODE_TYPES,
+  TSESTree,
+} from '@typescript-eslint/experimental-utils';
+import * as util from '../util';
+
+type Options = ['fields' | 'getters'];
+type MessageIds = 'preferFieldStyle' | 'preferGetterStyle';
+
+interface NodeWithModifiers {
+  accessibility?: TSESTree.Accessibility;
+  static: boolean;
+}
+
+const printNodeModifiers = (
+  node: NodeWithModifiers,
+  final: 'readonly' | 'get',
+): string =>
+  `${node.accessibility ?? ''}${
+    node.static ? ' static' : ''
+  } ${final} `.trimLeft();
+
+const isSupportedLiteral = (
+  node: TSESTree.Node,
+): node is TSESTree.LiteralExpression => {
+  if (
+    node.type === AST_NODE_TYPES.Literal ||
+    node.type === AST_NODE_TYPES.BigIntLiteral
+  ) {
+    return true;
+  }
+
+  if (
+    node.type === AST_NODE_TYPES.TaggedTemplateExpression ||
+    node.type === AST_NODE_TYPES.TemplateLiteral
+  ) {
+    return ('quasi' in node ? node.quasi.quasis : node.quasis).length === 1;
+  }
+
+  return false;
+};
+
+export default util.createRule<Options, MessageIds>({
+  name: 'class-literal-property-style',
+  meta: {
+    type: 'problem',
+    docs: {
+      description:
+        'Ensures that literals on classes are exposed in a consistent style',
+      category: 'Best Practices',
+      recommended: false,
+    },
+    fixable: 'code',
+    messages: {
+      preferFieldStyle: 'Literals should be exposed using readonly fields.',
+      preferGetterStyle: 'Literals should be exposed using getters.',
+    },
+    schema: [{ enum: ['fields', 'getters'] }],
+  },
+  defaultOptions: ['fields'],
+  create(context, [style]) {
+    if (style === 'fields') {
+      return {
+        MethodDefinition(node: TSESTree.MethodDefinition): void {
+          if (
+            node.kind !== 'get' ||
+            !node.value.body ||
+            !node.value.body.body.length
+          ) {
+            return;
+          }
+
+          const [statement] = node.value.body.body;
+
+          if (statement.type !== AST_NODE_TYPES.ReturnStatement) {
+            return;
+          }
+
+          const { argument } = statement;
+
+          if (!argument || !isSupportedLiteral(argument)) {
+            return;
+          }
+
+          context.report({
+            node: node.key,
+            messageId: 'preferFieldStyle',
+            fix(fixer) {
+              const sourceCode = context.getSourceCode();
+              const name = sourceCode.getText(node.key);
+
+              let text = '';
+
+              text += printNodeModifiers(node, 'readonly');
+              text += node.computed ? `[${name}]` : name;
+              text += ` = ${sourceCode.getText(argument)};`;
+
+              return fixer.replaceText(node, text);
+            },
+          });
+        },
+      };
+    }
+
+    return {
+      ClassProperty(node: TSESTree.ClassProperty): void {
+        if (!node.readonly || node.declare) {
+          return;
+        }
+
+        const { value } = node;
+
+        if (!value || !isSupportedLiteral(value)) {
+          return;
+        }
+
+        context.report({
+          node: node.key,
+          messageId: 'preferGetterStyle',
+          fix(fixer) {
+            const sourceCode = context.getSourceCode();
+            const name = sourceCode.getText(node.key);
+
+            let text = '';
+
+            text += printNodeModifiers(node, 'get');
+            text += node.computed ? `[${name}]` : name;
+            text += `() { return ${sourceCode.getText(value)}; }`;
+
+            return fixer.replaceText(node, text);
+          },
+        });
+      },
+    };
+  },
+});
diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -7,6 +7,7 @@ import banTypes from './ban-types';
 import braceStyle from './brace-style';
 import camelcase from './camelcase';
 import classNameCasing from './class-name-casing';
+import classLiteralPropertyStyle from './class-literal-property-style';
 import commaSpacing from './comma-spacing';
 import consistentTypeAssertions from './consistent-type-assertions';
 import consistentTypeDefinitions from './consistent-type-definitions';
@@ -102,6 +103,7 @@ export default {
   'brace-style': braceStyle,
   camelcase: camelcase,
   'class-name-casing': classNameCasing,
+  'class-literal-property-style': classLiteralPropertyStyle,
   'comma-spacing': commaSpacing,
   'consistent-type-assertions': consistentTypeAssertions,
   'consistent-type-definitions': consistentTypeDefinitions,
diff --git a/packages/eslint-plugin/tests/rules/class-literal-property-style.test.ts b/packages/eslint-plugin/tests/rules/class-literal-property-style.test.ts
