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

Conversation

FRSgit
Copy link

@FRSgit FRSgit commented Dec 16, 2024

Adds prefer-class-fields rule which turns:

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

into:

class Foo {
  foo = 'foo';
}

Fixes #314


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


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


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

@fisker
Copy link
Collaborator

fisker commented Dec 17, 2024

What should we do if the assignment is unreachable?

			class Foo {
				constructor(x) {
					if (x) {return}
					this.foo = 'foo';
				}
			}

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


```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.

docs/rules/prefer-class-fields.md Show resolved Hide resolved
@fregante
Copy link
Collaborator

What should we do if the assignment is unreachable?

It's dynamically set, so it cannot be statically defined. It should not be reported by this rule.

@FRSgit
Copy link
Author

FRSgit commented Dec 20, 2024

Hey @fregante!
Thanks for your input about the linting - is there a ruleset somewhere which I could use? The lint task in this repo is not doing this much (at least for me) and it removes some of your changes 😄 E.g. it seems that the part /** @type {import('eslint').Rule.RuleModule} */ is required by the lint task - after your change it was failing with:
image
So, I've reverted this one change.

Or maybe we can just agree that once the work in this PR is done you'll maybe just run your linter on it once?

@FRSgit FRSgit force-pushed the feat/add-prefer-class-fields branch from 06d643a to 3a9c5f8 Compare December 20, 2024 02:02
@FRSgit
Copy link
Author

FRSgit commented Dec 22, 2024

Handled stopping of the static analysis on unsupported case: 3376845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: prefer-class-fields
4 participants