Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 18 commits into from Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1b5c815
feat(eslint-plugin): create `class-literals-style` rule
G-Rath Feb 9, 2020
24c4c2e
docs(eslint-plugin): fix typos for `class-literals-style`
G-Rath Feb 12, 2020
2c5f030
fix(eslint-plugin): use optional tuple for option
G-Rath Feb 12, 2020
79349d5
chore(eslint-plugin): fix test file name
G-Rath Feb 12, 2020
f8573be
docs(eslint-plugin): link to intended rule
G-Rath Feb 12, 2020
40b0a45
docs(eslint-plugin): add clarity to `class-literals-style` rule docs
G-Rath Mar 10, 2020
3761809
fix(eslint-plugin): make `fields` default for `class-literals-style`
G-Rath Mar 10, 2020
85d6b04
fix(eslint-plugin): check if class field is `declare`d
G-Rath Mar 10, 2020
705b85f
test(eslint-plugin): add cases for parasble semantically incorrect code
G-Rath Mar 10, 2020
1a8859f
test(eslint-plugin): add case for silly absurd parsable code
G-Rath Mar 10, 2020
027902a
fix(eslint-plugin): add space around equals in fixer
G-Rath Mar 10, 2020
e093483
test(eslint-plugin): remove duplicated cases
G-Rath Mar 11, 2020
11bba63
fix(eslint-plugin): replace whole node on fixing `class-literal-styles`
G-Rath Mar 11, 2020
d7257d6
feat(eslint-plugin): support `TaggedTemplateExpression`
G-Rath Mar 11, 2020
30e0c02
test(eslint-plugin): add `line` & `column` expectations
G-Rath Mar 11, 2020
41b637b
fix(eslint-plugin): remove unneeded conditional check
G-Rath Mar 11, 2020
92c6b34
Merge branch 'master' into create-class-literals-style-rule
bradzacher Mar 20, 2020
9647f56
chore(eslint-plugin): rename rule to `class-literal-property-style`
G-Rath Mar 23, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
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
@@ -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
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
@@ -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
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