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

Add prefer-class-fields rule #2512

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
41 changes: 41 additions & 0 deletions docs/rules/prefer-class-fields.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Prefer class field declarations over assigning static values in constructor using `this`
FRSgit marked this conversation as resolved.
Show resolved Hide resolved

💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs).

🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

This rule enforces the use of class field declarations for static values, instead of assigning them in constructors using `this`.

> To avoid leaving empty constructors after autofixing, use the [`no-useless-constructor` rule](https://eslint.org/docs/latest/rules/no-useless-constructor).

## Fail

```js
class Foo {
constructor() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab-indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.foo = 'foo';
}
}

class MyError extends Error {
constructor(message: string) {
super(message);
this.name = "MyError";
}
}
```

## Pass

```js
class Foo {
foo = 'foo';
}

class MyError extends Error {
name = "MyError"
}
```
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
| [prefer-array-some](docs/rules/prefer-array-some.md) | Prefer `.some(…)` over `.filter(…).length` check and `.{find,findLast,findIndex,findLastIndex}(…)`. | ✅ | 🔧 | 💡 |
| [prefer-at](docs/rules/prefer-at.md) | Prefer `.at()` method for index access and `String#charAt()`. | ✅ | 🔧 | 💡 |
| [prefer-blob-reading-methods](docs/rules/prefer-blob-reading-methods.md) | Prefer `Blob#arrayBuffer()` over `FileReader#readAsArrayBuffer(…)` and `Blob#text()` over `FileReader#readAsText(…)`. | ✅ | | |
| [prefer-class-fields](docs/rules/prefer-class-fields.md) | Prefer class field declarations over assigning static values in constructor using `this`. | ✅ | 🔧 | |
| [prefer-code-point](docs/rules/prefer-code-point.md) | Prefer `String#codePointAt(…)` over `String#charCodeAt(…)` and `String.fromCodePoint(…)` over `String.fromCharCode(…)`. | ✅ | | 💡 |
| [prefer-date-now](docs/rules/prefer-date-now.md) | Prefer `Date.now()` to get the number of milliseconds since the Unix Epoch. | ✅ | 🔧 | |
| [prefer-default-parameters](docs/rules/prefer-default-parameters.md) | Prefer default parameters over reassignment. | ✅ | 🔧 | 💡 |
Expand Down
116 changes: 116 additions & 0 deletions rules/prefer-class-fields.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
'use strict';
const getIndentString = require('./utils/get-indent-string.js');

const MESSAGE_ID = 'prefer-class-fields/error';
const messages = {
[MESSAGE_ID]:
'Prefer class field declaration over `this` assignment in constructor for static values.',
};

/**
* @param {import('eslint').Rule.Node} node
* @returns {node is import('estree').ExpressionStatement & {expression: import('estree').AssignmentExpression & {left: import('estree').MemberExpression & {object: import('estree').ThisExpression}}}}
*/
const isThisAssignmentExpression = node => {
if (
node.type !== 'ExpressionStatement'
|| node.expression.type !== 'AssignmentExpression'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operator didn't checked

			class Foo {
				constructor() {
					this.foo += 'foo';
				}
			}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it should be ignored. Fixed here: 21e3cf1

) {
return false;
}

const lhs = node.expression.left;

if (!lhs.object || lhs.object.type !== 'ThisExpression') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The left property didn't checked

			class Foo {
				constructor() {
					this[foo.bar] = 'foo';
				}
			}

Copy link
Author

@FRSgit FRSgit Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case we should ignore any kind of reporting - foo.bar might be dynamic and checking if it is or not is a rabbit hole with potentially lots of nested jumps.

I can handle cases like this['foo'] = 'foo'; though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a valid test case to cover the dynamic case: 633829c

Copy link
Author

@FRSgit FRSgit Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here: 0e6afb3 added support for simple strings & template strings.

I ended up removing the support for strings/template strings. With support for multiple "name formats" the whole process looks like this:

  1. check whether string is actually translatable into other "name formats",
  2. iterate through all of the formats and remove class fields that are already there: foo='foo', ['foo']='foo', etc.
  3. move field declaration from constructor to the class itself: foo='foo'

Supporting those IMO brings lots of unespected edge cases in the points 1 & 2. It's hard to predict which string and how can be represented. Simple this.foo can be written as this['foo'], this["foo"] or this[`foo`], but string with kebab-case like: "foo-something" can be only used as this['foo-something'], this["foo-something"] or this[`foo-something`]. For every of those "name formats" there are other characters that are restricted (and I don't want even start about the escaped characters 😄 ).
It's just too much of a hassle and places that can potentially break

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to ask support for strings, please test

			class Foo {
				constructor() {
					this[foo] = 'foo';
				}
			}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, I didn't understood it well then :)
Done: f9caf1b

return false;
}

return true;
};

/**
* @template Array
* @param {Array} array
* @returns {Array}
*/
const reverseArray = array => [...array].reverse();

/**
* @param {import('eslint').Rule.Node} node
* @param {import('eslint').Rule.RuleContext['sourceCode']} sourceCode
* @param {import('eslint').Rule.RuleFixer} fixer
*/
const removeThisFieldAssignment = (node, sourceCode, fixer) => {
const {line} = node.loc.start;
const nodeText = sourceCode.getText(node);
const lineText = sourceCode.lines[line - 1];
const isOnlyNodeOnLine = lineText.trim() === nodeText;

return isOnlyNodeOnLine
? fixer.removeRange([
sourceCode.getIndexFromLoc({line, column: 0}),
sourceCode.getIndexFromLoc({line: line + 1, column: 0}),
])
: fixer.remove(node);
};

/** @type {import('eslint').Rule.RuleModule['create']} */
const create = context => {
const {sourceCode} = context;

return {
ClassBody(node) {
const constructor = node.body.find(x => x.kind === 'constructor');

if (!constructor || constructor.type !== 'MethodDefinition') {
return;
}

const constructorBody = constructor.value.body?.body;

if (!constructorBody) {
return;
}

const classBodyStartRange = [node.range[0], node.range[0] + 1];
const indent = getIndentString(constructor, sourceCode);

for (const node of reverseArray(constructorBody)) {
if (
isThisAssignmentExpression(node)
&& node.expression.right?.type === 'Literal'
) {
return {
node,
messageId: MESSAGE_ID,

/** @param {import('eslint').Rule.RuleFixer} fixer */
* fix(fixer) {
yield removeThisFieldAssignment(node, sourceCode, fixer);
yield fixer.insertTextAfterRange(
classBodyStartRange,
`\n${indent}${node.expression.left.property.name} = ${node.expression.right.raw};`,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix also changes code if the property already exists.

			class Foo {
				foo = 'bar';
				constructor() {
					this.foo = 'foo';
				}
			}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. IMO in this case the output should be:

class Foo {
    foo = 'foo';
    constructor() {
    }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: a7edb37

},
};
}
}
},
};
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
description:
'Prefer class field declarations over assigning static values in constructor using `this`.',
recommended: true,
},
fixable: 'code',
hasSuggestions: false,
messages,
},
};
91 changes: 91 additions & 0 deletions test/prefer-class-fields.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import outdent from 'outdent';
import {getTester} from './utils/test.mjs';

const {test} = getTester(import.meta);

const MESSAGE_ID = 'prefer-class-fields/error';

test.snapshot({
valid: [
outdent`
class Foo {
foo = 'foo';
}
`,
outdent`
class MyError extends Error {
name = "MyError";
}
`,
],
invalid: [
outdent`
class Foo {
constructor() {
this.foo = 'foo';
}
}
`,
outdent`
class Foo {
constructor() {
this.foo = 'foo';
this.foo2 = 'foo2';
}
}
`,
outdent`
class Foo {
constructor(argument) {
this.foo = 'foo';
this.foo2 = argument + 'test';
this.foo3 = 'foo3';
}
}
`,
outdent`
class MyError extends Error {
constructor(message) {
super(message);
this.name = "MyError";
}
}
`,
],
});

test.typescript({
valid: [
outdent`
class Foo {
foo: string = 'foo';
}
`,
outdent`
declare class Foo {
constructor(foo?: string);
}
`,
],
invalid: [
{
code: outdent`
class MyError extends Error {
constructor(message: string) {
super(message);
this.name = "MyError";
}
}
`,
errors: [{messageId: MESSAGE_ID}],
output: outdent`
class MyError extends Error {
name = "MyError";
constructor(message: string) {
super(message);
}
}
`,
},
],
});
Loading
Loading