Add prefer-class-fields rule

[FRSgit]

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:

• #314: Rule proposal: prefer-class-fields

---

[fisker]

What should we do if the assignment is unreachable?

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

[fregante]

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]

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:

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]

Handled stopping of the static analysis on unsupported case: 3376845

[FRSgit]

Okay, I fixed all test/linting errors connected with my rule - should I fix other as well? They seems to be connected with some default options missing in already existing rules?
cc: @sindresorhus @fregante @fisker

[FRSgit]

Hey! I've merged last changes from main to the branch of this PR - now it's ready to be reviewed again

[sindresorhus]

Change the test file extension from .mjs to .js.

[FRSgit]

@sindresorhus done: 8358a43

I'm not sure yet why the tests are failing - locally they're working just fine.

[FRSgit]

@sindresorhus Can you reopen this PR please? I've just pushed changes

[fisker]

Don't worry about the snapshot files, I can take care.

[fisker]

Let's move the body check into L103 too?

[FRSgit]

done: 242410a

constructorBody is used later on, so I needed to use additional optional chaining operators. In terms of typing of the constructorBody - to achieve the same effect (correct constructorBody typing for later usage) I needed to create a type guard.

[FRSgit]

Thanks, @fisker my macbook broke down (is serviced - that's why I have time to work on this PR again) and I'm working through stackblitz. But it seems that the stackblitz supports only Node 20, so I'm not able to regenerate snapshots properly 😄

[fisker]

@FRSgit I pushed some changes

• Make code easier to understand

• Strictly check computed, static for finding existing property (previously, it incorrectly considered a static/computed property as existing)

• Insert property at end instead of before constructor, safer.

• Play safer by only checking the first expression (currently only ignores EmptyStatement)

  Take the existing example

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

  We can't know what super(message) do, it can change name, the original will override what super() do, but if we use a property, it will override by super class, this is rarely but still possible.

  Not sure how to improve this part.

  Potential issue if we decide to ignore more expressions, the current removeFieldAssignment need deal with the ASI problem.

  for example, if we decide to ignore super() call.

  class Foo {
    constructor() {
      super()  
      this.bar = 1
      ;[a]
    }
  }

• Improved fix logic, only use suggestion if existing property have value

---

@FRSgit This rule should work for private fields, do you think you can continue?

[FRSgit]

@fisker Thanks for your changes!

Regarding

We can't know what super(message) do, it can change name, the original will override what super() do, but if we use a property, it will override by super class, this is rarely but still possible.

I think ti's not the case, is it? Check the playground here (it's typescript playground, but the result will be the same in the developer console in your browser).

It looks like both ways: defining value in constructor and directly as a class field override what is set by parent constructor. Do I miss something?

Everything else is clear to me, thanks!

[fisker]

It looks like both ways: defining value in constructor and directly as a class field override what is set by parent constructor.

Sorry, you are right about this. Maybe we can also skip super().

Anyway, it's just an wrong example, I mean other nodes can change the result.

class A {
	constructor() {
		call(this);
		this.foo = 1;
	}
}

is not safe to fix to

class A {
	constructor() {
		call(this);
	}
	foo = 1
}

Since that function call may override foo. Am I right?

[fisker]

Since it's not easy to say what can be skipped, I'm not sure what we can do about these totally unrelated expressions like this:

class A {
	constructor() {
		const x = 2; // Can skip, but can't skip `const x = call()`
		this.foo = 1;
	}
}

[FRSgit]

@fisker Yes, you're right with the example using function call - I bet there are many other cases that might potentially break unless rules' autofix is pretty restrictive.

IMO to not over complicate, we should allow only EmptyExpressions and other this.foo = declarations to precede lines this rule will try to autofix. In other cases - the rule prints a suggestion and user must resolve by themselves. That's a valid solution I guess 🤷

Ofc, there are probably other cases where autofix could be applied just like that, but that's something for next iterations. When somebody comes with a proposition for improvement of this algo in the future.