From b2dbd890a5bef81aa6978d68c166457838ee04a1 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Mon, 23 Mar 2020 18:10:17 +1300 Subject: [PATCH] feat(eslint-plugin): add `class-literal-property-style` rule (#1582) --- packages/eslint-plugin/README.md | 1 + .../rules/class-literal-property-style.md | 96 +++++ packages/eslint-plugin/src/configs/all.json | 1 + .../src/rules/class-literal-property-style.ts | 136 +++++++ packages/eslint-plugin/src/rules/index.ts | 2 + .../class-literal-property-style.test.ts | 378 ++++++++++++++++++ 6 files changed, 614 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/class-literal-property-style.md create mode 100644 packages/eslint-plugin/src/rules/class-literal-property-style.ts create mode 100644 packages/eslint-plugin/tests/rules/class-literal-property-style.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index b7927757924..fa049b2cfb2 100644 --- 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-` 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 new file mode 100644 index 00000000000..903a695dd76 --- /dev/null +++ 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 index 2626767b84c..c5575d4970d 100644 --- 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 new file mode 100644 index 00000000000..a4500cdfdbf --- /dev/null +++ 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({ + 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 index 486b8a97945..29db58b681a 100644 --- 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 new file mode 100644 index 00000000000..f0c5f2afc05 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/class-literal-property-style.test.ts @@ -0,0 +1,378 @@ +import rule from '../../src/rules/class-literal-property-style'; +import { RuleTester } from '../RuleTester'; + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('class-literal-property-style', rule, { + valid: [ + 'class Mx { declare readonly p1 = 1; }', + 'class Mx { readonly p1 = "hello world"; }', + 'class Mx { p1 = "hello world"; }', + 'class Mx { static p1 = "hello world"; }', + 'class Mx { p1: string; }', + 'class Mx { get p1(); }', + 'class Mx { get p1() {} }', + 'abstract class Mx { abstract get p1(): string }', + ` + class Mx { + get mySetting() { + if(this._aValue) { + return 'on'; + } + + return 'off'; + } + } + `, + ` + class Mx { + get mySetting() { + return \`build-\${process.env.build}\` + } + } + `, + ` + class Mx { + getMySetting() { + if(this._aValue) { + return 'on'; + } + + return 'off'; + } + } + `, + ` + class Mx { + public readonly myButton = styled.button\` + color: \${props => (props.primary ? 'hotpink' : 'turquoise')}; + \`; + } + `, + { + code: ` + class Mx { + public get myButton() { + return styled.button\` + color: \${props => (props.primary ? 'hotpink' : 'turquoise')}; + \`; + } + } + `, + options: ['fields'], + }, + { + code: 'class Mx { declare public readonly foo = 1; }', + options: ['getters'], + }, + { + code: 'class Mx { get p1() { return "hello world"; } }', + options: ['getters'], + }, + { + code: 'class Mx { p1 = "hello world"; }', + options: ['getters'], + }, + { + code: 'class Mx { p1: string; }', + options: ['getters'], + }, + { + code: 'class Mx { readonly p1 = [1, 2, 3]; }', + options: ['getters'], + }, + { + code: 'class Mx { static p1: string; }', + options: ['getters'], + }, + { + code: 'class Mx { static get p1() { return "hello world"; } }', + options: ['getters'], + }, + { + code: ` + class Mx { + public readonly myButton = styled.button\` + color: \${props => (props.primary ? 'hotpink' : 'turquoise')}; + \`; + } + `, + options: ['getters'], + }, + { + code: ` + class Mx { + public get myButton() { + return styled.button\` + color: \${props => (props.primary ? 'hotpink' : 'turquoise')}; + \`; + } + } + `, + options: ['getters'], + }, + ], + invalid: [ + { + code: 'class Mx { get p1() { return "hello world"; } }', + output: 'class Mx { readonly p1 = "hello world"; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 16, + line: 1, + }, + ], + }, + { + code: 'class Mx { get p1() { return `hello world`; } }', + output: 'class Mx { readonly p1 = `hello world`; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 16, + line: 1, + }, + ], + }, + { + code: 'class Mx { static get p1() { return "hello world"; } }', + output: 'class Mx { static readonly p1 = "hello world"; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 23, + line: 1, + }, + ], + }, + { + code: + 'class Mx { public static readonly static private public protected get foo() { return 1; } }', + output: 'class Mx { public static readonly foo = 1; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 71, + line: 1, + }, + ], + }, + { + code: ` + class Mx { + public get [myValue]() { + return 'a literal value'; + } + } + `, + output: ` + class Mx { + public readonly [myValue] = 'a literal value'; + } + `, + errors: [ + { + messageId: 'preferFieldStyle', + column: 23, + line: 3, + }, + ], + }, + { + code: ` + class Mx { + public get [myValue]() { + return 12345n; + } + } + `, + output: ` + class Mx { + public readonly [myValue] = 12345n; + } + `, + errors: [ + { + messageId: 'preferFieldStyle', + column: 23, + line: 3, + }, + ], + }, + { + code: ` + class Mx { + public readonly [myValue] = 'a literal value'; + } + `, + output: ` + class Mx { + public get [myValue]() { return 'a literal value'; } + } + `, + errors: [ + { + messageId: 'preferGetterStyle', + column: 28, + line: 3, + }, + ], + options: ['getters'], + }, + { + code: 'class Mx { readonly p1 = "hello world"; }', + output: 'class Mx { get p1() { return "hello world"; } }', + errors: [ + { + messageId: 'preferGetterStyle', + column: 21, + line: 1, + }, + ], + options: ['getters'], + }, + { + code: 'class Mx { readonly p1 = `hello world`; }', + output: 'class Mx { get p1() { return `hello world`; } }', + errors: [ + { + messageId: 'preferGetterStyle', + column: 21, + line: 1, + }, + ], + options: ['getters'], + }, + { + code: 'class Mx { static readonly p1 = "hello world"; }', + output: 'class Mx { static get p1() { return "hello world"; } }', + errors: [ + { + messageId: 'preferGetterStyle', + column: 28, + line: 1, + }, + ], + options: ['getters'], + }, + { + code: 'class Mx { protected get p1() { return "hello world"; } }', + output: 'class Mx { protected readonly p1 = "hello world"; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 26, + line: 1, + }, + ], + options: ['fields'], + }, + { + code: 'class Mx { protected readonly p1 = "hello world"; }', + output: 'class Mx { protected get p1() { return "hello world"; } }', + errors: [ + { + messageId: 'preferGetterStyle', + column: 31, + line: 1, + }, + ], + options: ['getters'], + }, + { + code: 'class Mx { public static get p1() { return "hello world"; } }', + output: 'class Mx { public static readonly p1 = "hello world"; }', + errors: [ + { + messageId: 'preferFieldStyle', + column: 30, + line: 1, + }, + ], + }, + { + code: 'class Mx { public static readonly p1 = "hello world"; }', + output: 'class Mx { public static get p1() { return "hello world"; } }', + errors: [ + { + messageId: 'preferGetterStyle', + column: 35, + line: 1, + }, + ], + options: ['getters'], + }, + { + code: ` + class Mx { + public get myValue() { + return gql\` + { + user(id: 5) { + firstName + lastName + } + } + \`; + } + } + `, + output: ` + class Mx { + public readonly myValue = gql\` + { + user(id: 5) { + firstName + lastName + } + } + \`; + } + `, + errors: [ + { + messageId: 'preferFieldStyle', + column: 22, + line: 3, + }, + ], + }, + { + code: ` + class Mx { + public readonly myValue = gql\` + { + user(id: 5) { + firstName + lastName + } + } + \`; + } + `, + output: ` + class Mx { + public get myValue() { return gql\` + { + user(id: 5) { + firstName + lastName + } + } + \`; } + } + `, + errors: [ + { + messageId: 'preferGetterStyle', + column: 27, + line: 3, + }, + ], + options: ['getters'], + }, + ], +});