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(prefer-const): add rule #933

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

mikededo
Copy link
Contributor

@mikededo mikededo commented Nov 28, 2024

Adds a new rule named prefer-const which:

This PR is an updated version of #816, which did not include types. Also, some
opinionated changes were made in terms of code style, not in terms of behavior.

Avoided re-implemeting the core rule, by using the getCoreRule and proxying the
default implementation.

I found that this rule could be very useful for Svelte, and there could be many
developers who would benefit from it. At the same time, I thought that the
initial implementation was a good starting point, and that adding types and
would make the rule more robust to changes.

However, the rule should not have many changes, as how variables are handled in
JS is not likely to change.

Acknowledgement: this rule is based on the prefer-const rule from ESLint and
the initial work done by @bfanger.

Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: df7abaa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-svelte Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mikededo
Copy link
Contributor Author

Since this rule logic has been adapted from the original prefer-const, I plan on adding the same tests as there are in the prefer-const implementation (see the test source).

I feel it's mandatory to also evaluate all the expected behaviour, not only what happens in *.svelte.

@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch 2 times, most recently from 2052cf5 to 84be629 Compare November 29, 2024 16:29
@ota-meshi
Copy link
Member

ota-meshi commented Nov 30, 2024

Thanks for working on implementing this rule!
However, could you please implement it using getCoreRule() in the same way as no-inner-declarations?
https://github.com/sveltejs/eslint-plugin-svelte/blob/main/packages/eslint-plugin-svelte/src/rules/no-inner-declarations.ts

I think it just avoids calling listeners on VariableDeclaration that use runes.

I imagine it would probably be something like this:

	create(context) {
		return defineWrapperListener(coreRule, context, {
			createListenerProxy(coreListener) {
				return {
					...coreListener,
					VariableDeclaration(node) {
						for (const decl of node.declarations) {
							if (
								decl.init?.type === 'CallExpression' &&
								decl.init.callee.type === 'Identifier' &&
								['$state', '$props', '$derived'].includes(decl.init.callee.name)
							) {
								return;
							}
						}
						coreListener.VariableDeclaration?.(node);
					}
				};
			}
		});
	}

@mikededo mikededo marked this pull request as ready for review November 30, 2024 07:45
@mikededo
Copy link
Contributor Author

Thanks @ota-meshi for pointing this out, my initial idea was to use a proxy for the rule, to avoid having a complete re-write. Should've asked before spending so many hours 😅 🤦🏼

Will look into it. Maybe I'll end up closing this PR in favour of another one.

@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch from 53282a2 to 1b1b4f7 Compare November 30, 2024 17:42
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

packages/eslint-plugin-svelte/src/rules/prefer-const.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-svelte/src/rules/prefer-const.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-svelte/src/rules/prefer-const.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-svelte/src/rules/prefer-const.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-svelte/src/rules/prefer-const.ts Outdated Show resolved Hide resolved
@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch from e8ab658 to 77fd054 Compare December 2, 2024 07:21
@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch 3 times, most recently from 41256b4 to 5e01c1a Compare December 2, 2024 18:04
@mikededo mikededo requested a review from ota-meshi December 2, 2024 18:37
@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch from 5e01c1a to 9f6105e Compare December 11, 2024 12:35
After having finished the implementation, I've decided to extract the
logic into a function, since I strongly believe it'll be easier to
understand, as functions are equivalent to classes, if done right.
The reason of this commit is to save the current implementation in case
I want to revert changes.
Code has been moved into `prefer-const-helpers` folder, so the rule file
is simpler.
By adding the original rule tests, I was able to identify issues with
the rule, that had been caused because of the refactoring. Now, not only
I think the code is more readable and maintainable, but also the
behavior is as expected, and there are tests to ensure that.

There's a tests that has been skipped since it reporting a false
positive, which is dues to `@typescript-eslint`. I've reported the issue
in typescript-eslint/typescript-eslint#10426
Remove the duplicated code implementation in the rule, by using the
helpers to get the actual rule ✨
- Removed unnecessary if branches
- Updated tests cases
- Since we are now using the default implementation of the rule, there's
  no need to keep the original tests, since the default rule is already
  tested.
- Removed unnecessary test cases
@mikededo mikededo force-pushed the feat/add-prefer-const-rule branch from 9f6105e to df7abaa Compare December 11, 2024 12:37
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.

2 participants