🦋 Changeset detected

Latest commit: 0e28cc1

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

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

Open in Stackblitz

npm i https://pkg.pr.new/eslint-plugin-svelte@980

commit: 0e28cc1

@ota-meshi Could you please review the policy of the PR? If there are no issues, I believe I can handle the remaining tasks on my own.

Hi @baseballyama, I am looking at the three alternative proposals (this one, #957, #943) and I am a little bit torn. Implementation-wise, I like this one the most because it "just works". However, I think this one will also be the hardest to explain to the users (such as me :D) clearly, because it somewhat boils down to "This will magically disable rules, trust us". In this "understandability" regard, I think the approach with separate rulesets seems easier to grasp. However, it introduces additional complexity for users, which is probably not the best choice.

Additionally, I expect that at some point in the future, obsolete versions of Svelte will be dropped altogether, so it'd be great to design this such that the users don't need to re-do their config once again at that point.

@marekdedic Many users are not interested in configuring ESLint in detail. They don’t want to think about things like “I need this config because I’m using Svelte 5” or “I need that config because I’m using SvelteKit.” It’s much simpler and better if everything just works by using the recommended config.

This issue can also be addressed by changing perspectives. Instead of seeing it as “magic,” we can treat it as a feature to eliminate false positives. For example, if a user is only using Svelte 5, there shouldn’t be any reports related to SvelteKit rules. However, AST-based inspections may still produce false positives. With this PR, false positives can be completely eliminated.

Similarly, enabling SvelteKit-specific rules without actually using SvelteKit can also cause issues. This PR prevents such problems as well.

I plan to completely review the rules included in the recommended config from scratch after merging this PR.

Many users are not interested in configuring ESLint in detail.

Yeah, I agree... Let me rephrase: I think this is probably the best of the 3 alternatives. At the same time, I think it will require a good explanation in the documentation so that the users that do want to configure eslint understand what's happening in the recommended config.

Sorry, looking at it more, I have a question to the design of this feature: Why is the checking of applicability of the rule left to the rule implementation? Isn't it something that could be configured in the rule meta and checked automatically? It seems to me that that would be less error-prone (especially when 3rd party contributors such as me add new rules) and maybe could be used to indicate the support in docs as well? But maybe I am missing something, I don't know the rule meta stuff that good...

Is it possible to control whether a rule is invoked by accessing its meta information at runtime? (AFAIK, there is no way) However, such improvements might be possible.

export default createRule('rule-name', {
	meta: {
		executeCondition: {
			svelte: [5],
			isKitRule: true,
			kitRouteTypes: ["+page.svelte", "+page.js"]
		},
		...
	},
	create(context) {
		return executeIf(this.meta.executeCondition, (context) => {
			// ... implement rule ...
		});
	}
});

We can easily check that executeIf is called at the top of create by custom ESLint rule.

Is it possible to control whether a rule is invoked by accessing its meta information at runtime?

I think it might be possible by injecting the check here:

https://github.com/sveltejs/eslint-plugin-svelte/blob/d0e76a330cf2cce18461ffa72204c71cf2ea6069/packages/eslint-plugin-svelte/src/utils/index.ts#L19

Ahh. Nice! I will update the PR tomorrow. (It’s already night in Japan, so I’m going to sleep.)

@marekdedic What do you think about the current implementation? I want to add tests somehow but before that I would like to ask your opinion.

Hey @baseballyama, great work on a quick design in one day! I think the general idea is good, I have a lot of questions/notes on the particulars - I left a lot of comments :D

Now most parts of this can be agreeable. Once this is merged, we can add conditions to all the rules and finally update the recommended config. At that point, the recommended config should be able to include all the recommended rules regardless of the Svelte version or whether SvelteKit is used.

@marekdedic

Thank you for the review again! Some points that felt like personal preferences were not changed at this time. There are still a few remaining points to discuss, so I’d appreciate your input on those as well. It would be great if we could merge this today. That would bring us significantly closer to v3!

Hi @baseballyama, I think we are getting there (honestly now it's just a few small details). Thanks for taking the time!

@marekdedic

Thank you for taking the time to review this! I think we are in agreement on almost areas and will merge to move forward.👍

Great, thanks for the work on this, I think this design is pretty great now :)