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 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
468a9c2
feat: add `prefer-class-fields` rule
FRSgit Dec 16, 2024
8c28503
fix: handle edge case of constructor without body
FRSgit Dec 16, 2024
8bc2304
Update prefer-class-fields.md
sindresorhus Dec 18, 2024
452dcf7
Update prefer-class-fields.js
sindresorhus Dec 18, 2024
5d88a08
perf: iterate over original array instead of copying it over and reve…
FRSgit Dec 20, 2024
acb5680
chore: handle only simple assignments
FRSgit Dec 20, 2024
9cb0fc9
lint: fix
FRSgit Dec 20, 2024
11a3621
chore: spaces to tabs
FRSgit Dec 20, 2024
6758bae
chore: add dynamic field names to valid test cases
FRSgit Dec 20, 2024
40bfb3c
feat: support strings and template strings
FRSgit Dec 20, 2024
62c82d6
chore: replace existing field if present
FRSgit Dec 20, 2024
097f673
chore: lint fix
FRSgit Dec 20, 2024
3b4d399
feat: handle only non-computed properties
FRSgit Dec 21, 2024
73d47e2
chore: stop static analysis on unsupported cases
FRSgit Dec 22, 2024
60983c3
chore: add missing semicolon
FRSgit Dec 25, 2024
14e4676
docs: update docs/rules/prefer-class-fields.md
FRSgit Jan 17, 2025
425f00f
docs: update rule description
FRSgit Jan 17, 2025
ea96a91
Update prefer-class-fields.md
sindresorhus Jan 18, 2025
a849f2b
Update prefer-class-fields.md
sindresorhus Jan 18, 2025
a9c9f4b
Update prefer-class-fields.js
sindresorhus Jan 19, 2025
b4f5349
chore: update rules/prefer-class-fields.js
FRSgit Jan 20, 2025
3a520cb
chore: change examples style
FRSgit Jan 21, 2025
ca160be
chore: rename test files
FRSgit Jan 21, 2025
1ef1a3f
chore: add prefer-class-fields to index.js
FRSgit Jan 29, 2025
f9ede71
chore: run fix:eslint docs (works only on node 22)
FRSgit Jan 29, 2025
d27356b
Update prefer-class-fields.md
sindresorhus Jan 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions docs/rules/prefer-class-fields.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Prefer class field declarations over `this` assignments in constructors

💼 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).

## Examples

```js
// ❌
class Foo {
constructor() {
this.foo = 'foo';
}
}

// ✅
class Foo {
foo = 'foo';
}
```

```js
// ❌
class MyError extends Error {
constructor(message: string) {
super(message);
this.name = 'MyError';
}
}

// ✅
class MyError extends Error {
name = 'MyError'
}
```

```js
// ❌
class Foo {
foo = 'foo';
constructor() {
this.foo = 'bar';
}
}

// ✅
class Foo {
foo = 'bar';
}
```
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ export default [
| [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 `this` assignments in constructors. | ✅ | 🔧 | |
| [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
2 changes: 2 additions & 0 deletions rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import preferArrayIndexOf from './prefer-array-index-of.js';
import preferArraySome from './prefer-array-some.js';
import preferAt from './prefer-at.js';
import preferBlobReadingMethods from './prefer-blob-reading-methods.js';
import preferClassFields from './prefer-class-fields.js';
import preferCodePoint from './prefer-code-point.js';
import preferDateNow from './prefer-date-now.js';
import preferDefaultParameters from './prefer-default-parameters.js';
Expand Down Expand Up @@ -203,6 +204,7 @@ const rules = {
'prefer-array-some': createRule(preferArraySome, 'prefer-array-some'),
'prefer-at': createRule(preferAt, 'prefer-at'),
'prefer-blob-reading-methods': createRule(preferBlobReadingMethods, 'prefer-blob-reading-methods'),
'prefer-class-fields': createRule(preferClassFields, 'prefer-class-fields'),
'prefer-code-point': createRule(preferCodePoint, 'prefer-code-point'),
'prefer-date-now': createRule(preferDateNow, 'prefer-date-now'),
'prefer-default-parameters': createRule(preferDefaultParameters, 'prefer-default-parameters'),
Expand Down
182 changes: 182 additions & 0 deletions rules/prefer-class-fields.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
import getIndentString from './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'
FRSgit marked this conversation as resolved.
Show resolved Hide resolved
) {
return false;
}

const lhs = node.expression.left;

if (!lhs.object || lhs.object.type !== 'ThisExpression') {
FRSgit marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

return true;
};

/**
@param {import('eslint').Rule.Node} node
@param {import('eslint').Rule.RuleContext['sourceCode']} sourceCode
@param {import('eslint').Rule.RuleFixer} fixer
*/
const removeFieldAssignment = (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);
};

/**
@param {string} propertyName
@param {import('estree').ClassBody} classBody
*/
const findClassFieldNamed = (propertyName, classBody) => {
for (const classBodyChild of classBody.body) {
if (
classBodyChild.type === 'PropertyDefinition'
&& classBodyChild.key.type === 'Identifier'
&& classBodyChild.key.name === propertyName
) {
return classBodyChild;
}
}
};

/**
@param {string} propertyName
@param {string} propertyValue
@param {import('estree').ClassBody} classBody
@param {import('estree').MethodDefinition} constructor
@param {import('eslint').Rule.RuleContext['sourceCode']} sourceCode
@param {import('eslint').Rule.RuleFixer} fixer
*/
const addOrReplaceClassFieldDeclaration = (
propertyName,
propertyValue,
classBody,
constructor,
sourceCode,
fixer,
) => {

Check warning on line 80 in rules/prefer-class-fields.js

View workflow job for this annotation

GitHub Actions / lint-test (ubuntu-latest)

Arrow function has too many parameters (6). Maximum allowed is 4

Check warning on line 80 in rules/prefer-class-fields.js

View workflow job for this annotation

GitHub Actions / lint-test (windows-latest)

Arrow function has too many parameters (6). Maximum allowed is 4
const alreadyExistingDeclaration = findClassFieldNamed(
propertyName,
classBody,
);

if (alreadyExistingDeclaration) {
return fixer.replaceText(
alreadyExistingDeclaration,
`${propertyName} = ${propertyValue};`,
);
}

const classBodyStartRange = [classBody.range[0], classBody.range[0] + 1];
const indent = getIndentString(constructor, sourceCode);
return fixer.insertTextAfterRange(
classBodyStartRange,
`\n${indent}${propertyName} = ${propertyValue};`,
);
};

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

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

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

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

if (!constructorBody) {
return;
}

const firstInvalidProperty = constructorBody.findIndex(
node => node.type !== 'ExpressionStatement',
);
Comment on lines +121 to +123
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please help me understand this, are you assuming assignments before non-ExpressionStatements are reachable? And it's addressing #2512 (comment) ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, exactly - that's the assumption. I'm not an expert on node types in this ESTree AST, but my thinking was: it's better to whitelist (allow only ExpressionStatements) than to try writing down all node types that might make rest of the code unreachable. Just to be on the safe side

const validConstructorProperties
= firstInvalidProperty === -1
? constructorBody
: constructorBody.slice(0, firstInvalidProperty);

for (
let index = validConstructorProperties.length - 1;
index >= 0;
index--
) {
const node = validConstructorProperties[index];
if (
isThisAssignmentExpression(node)
&& node.expression.right?.type === 'Literal'
&& node.expression.operator === '='
&& node.expression.left.property.type === 'Identifier'
&& !node.expression.left.computed
) {
return {
node,
messageId: MESSAGE_ID,

/**
@param {import('eslint').Rule.RuleFixer} fixer
*/
* fix(fixer) {
yield removeFieldAssignment(node, sourceCode, fixer);
yield addOrReplaceClassFieldDeclaration(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not remove code, especially there can be side effects. We can use suggestions or simply just report error, if property already exists.

Copy link
Author

Choose a reason for hiding this comment

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

My thinking was: property in the constructor always takes priority over class field declaration, so there are no side-effects. IMO it's a safe assumption.
WDYT?

Copy link
Collaborator

@fisker fisker Feb 5, 2025

Choose a reason for hiding this comment

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

const getFoo = () => console.log('called')

new class {
  foo = getFoo();  // <-- this function call is removed.
  constructor() {
     this.foo = 1
  }
}

node.expression.left.property.name,
node.expression.right.raw,
classBody,
constructor,
sourceCode,
fixer,
);
FRSgit marked this conversation as resolved.
Show resolved Hide resolved
},
};
}
}
},
};
};

/** @type {import('eslint').Rule.RuleModule} */
const config = {
create,
meta: {
type: 'suggestion',
docs: {
description: 'Prefer class field declarations over `this` assignments in constructors.',
recommended: true,
},
fixable: 'code',
hasSuggestions: false,
messages,
},
};

export default config;
5 changes: 1 addition & 4 deletions rules/utils/rule.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ import getDocumentationUrl from './get-documentation-url.js';
const isIterable = object => typeof object?.[Symbol.iterator] === 'function';

class FixAbortError extends Error {
constructor() {
super();
this.name = 'FixAbortError';
}
name = 'FixAbortError';
}
const fixOptions = {
abort() {
Expand Down
1 change: 1 addition & 0 deletions test/package.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const RULES_WITHOUT_PASS_FAIL_SECTIONS = new Set([
'prefer-modern-math-apis',
'prefer-math-min-max',
'consistent-existence-index-check',
'prefer-class-fields',
'prefer-global-this',
'no-instanceof-builtins',
'no-named-default',
Expand Down
Loading
Loading