Skip to content

Commit

Permalink
feat(eslint-plugin): add class-literal-property-style rule (#1582)
Browse files Browse the repository at this point in the history
  • Loading branch information
G-Rath authored Mar 23, 2020
1 parent 3eb5d45 commit b2dbd89
Show file tree
Hide file tree
Showing 6 changed files with 614 additions and 0 deletions.
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: | | |
Expand Down
96 changes: 96 additions & 0 deletions packages/eslint-plugin/docs/rules/class-literal-property-style.md
Original file line number Diff line number Diff line change
@@ -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.
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
136 changes: 136 additions & 0 deletions packages/eslint-plugin/src/rules/class-literal-property-style.ts
Original file line number Diff line number Diff line change
@@ -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);
},
});
},
};
},
});
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit b2dbd89

Please sign in to comment.