feat: add control for executing rules based on Svelte/SvelteKit context #980

baseballyama · 2024-12-30T14:01:51Z

This PR introduces the getSvelteContext function. This function will be invoked at the beginning of every rule before the release of eslint-plugin-svelte v3. It enables the conditional execution of linting based on factors such as the Svelte version SvelteKit route　etc. By doing so, it allows users can use recommended config regardless of the Svelte version they are using. As a result, unnecessary rules will be effectively disabled automatically.

After merging this PR and applying the changes to all rules, I plan to update the recommended config and release v3. Consequently, I will close #957.

changeset-bot · 2024-12-30T14:01:54Z

🦋 Changeset detected

Latest commit: 0e28cc1

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

pkg-pr-new · 2024-12-30T14:02:49Z

Open in Stackblitz

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

commit: 0e28cc1

baseballyama · 2024-12-30T14:13:31Z

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

packages/eslint-plugin-svelte/src/rules/no-export-load-in-svelte-module-in-kit-pages.ts

packages/eslint-plugin-svelte/src/utils/svelte-kit.ts

packages/eslint-plugin-svelte/tests/utils/utils.ts

packages/eslint-plugin-svelte/tools/new-rule.ts

marekdedic · 2025-01-02T10:19:53Z

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.

baseballyama · 2025-01-02T11:51:04Z

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

marekdedic · 2025-01-02T12:06:55Z

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.

marekdedic · 2025-01-02T12:11:37Z

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

baseballyama · 2025-01-02T12:43:08Z

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.

marekdedic · 2025-01-02T14:42:53Z

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:

eslint-plugin-svelte/packages/eslint-plugin-svelte/src/utils/index.ts

Line 19 in d0e76a3

create: rule.create as never

baseballyama · 2025-01-02T15:43:44Z

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

…n and SvelteKit routes etc.

baseballyama · 2025-01-03T09:49:02Z

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

packages/eslint-plugin-svelte/src/types.ts

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

marekdedic · 2025-01-03T17:14:34Z

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

+
+export type SvelteContext = {
+	svelteVersion: string;
+	fileType: '.svelte' | '.svelte.[js|ts]';


Hmm, I don't like the design of having separate fileType and svelteKitFileType. I see 2 ways to do it better:

Merge them

Don't provide these, but go conceptually one level further - why would we want to know the file type? Maybe what we actually want to know is things like

Are we in a SFC file?

Are we in the server context?

etc.

I think the second solution is more work (and requires quite careful design), but ultimately serves us better, because rules are fundamentally going to be enabled based on the properties of the current context/file, not based on its type...

At here, it’s better to keep things simple and store only basic information. This is because the definition of "context" can change depending on the rules. For example, we might want to create a rule that prevents placing files like xxx.svelte.js in the same directory as +page.svelte.
Another example is that xxx.svelte.js might be used for implementing composables, and we might want to enforce a rule that their names must start with useX.

In short, since the definition of "context" depends on the rules, it’s better to just manage file types here.

I'm still not convinced. I think we should instead add something like a fileContext field where we add some information about the file and we can possibly extend it in the future. I am not sure your first example is possible in eslint, as it operates on files separately. But anyway, we would probably need a separate flag for .svelte.js/ts files. But I don't think it is useful to have a way to distinguish +page.js, +page.server.js, +layout.js and +layout.server.js - what you would actually want in most situations is 2 flags - "Is this a layout file?" and "Is this a server-only file?"

Does that make sense to you or am I being too convoluted?

This is one of specific example why "Is this a layout file?" and "Is this a server-only file" is not enough.
#438

This rule should check only +layout.(js|ts). (+layout.svelte should not check.)

Hmm, maybe I'm wrong (I haven't used SvelteKit server-side yet), but #438 should also check +page.(js|ts), no? Then it's exactly what I've been getting at :D The differentiator isn't really a list of filename suffixes, but a role the file plays in SvelteKit...

Anyway, if my change is not accepted, then I would like to think a little bit more about how fileType and svelteKitFileType interact...

And yes, the list of "file role flags" would probably be longer and would need to expand over time and some rules would check multiple flags at once...

And yes, the list of "file role flags" would probably be longer and would need to expand over time and some rules would check multiple flags at once...

Yes. In that case, we can add helper functions for common validation patterns.
I think it’s best to start with basic functionality and then add more convenient features later if they prove to be valuable.

Hmm, maybe I'm wrong (I haven't used SvelteKit server-side yet), but #438 should also check +page.(js|ts),

Yes. We need to check +page.(js|ts) also, but what I wanted to say is that "Is this a layout file?" and "Is this a server-only file" is not enough.

Yes. In that case, we can add helper functions for common validation patterns.

Hmm, yeah, that's probably a good enough alternative.

I was thinking about the fileType a little bit more as well. I get that in svelteKitFileType, the list of file suffixes makes sense, but I think it gets confusing with fileType pretty quickly - if I understand it correctly, a +page.svelte would havea file type .svelte and +page.js would have a file type of .svelte.js - which is a little counterintuitive. Maybe fileType could have values "component"|"plain" or something similar? Then it wouldn't give the illusion of being a filename extension, which it isn't in case of SvelteKit files...

rename to svelteFileType.

bbfae0e

if I understand it correctly, a +page.svelte would havea file type .svelte and +page.js would have a file type of .svelte.js

No. .svelte.[js|ts] will be used for xxx.svelte.js or xxx.svelte.ts. But not +page.js,

.svelte and .svelte.[js|ts] is taken from the docs. (I intended svelteFileType to be determined by the file extension.)
https://svelte.dev/docs/svelte/svelte-js-files

svelteFileType should be null for +page.js, so I fixed this bug.

8a02534

packages/eslint-plugin-svelte/src/utils/index.ts

packages/eslint-plugin-svelte/src/types.ts

marekdedic

Thanks for the changes, unfortunately, I have more questions/comments... But I think we are getting there, good work on this!

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

marekdedic · 2025-01-06T11:28:01Z

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

+
+export type SvelteContext = {
+	svelteVersion: string;
+	fileType: '.svelte' | '.svelte.[js|ts]';


Hmm, maybe I'm wrong (I haven't used SvelteKit server-side yet), but #438 should also check +page.(js|ts), no? Then it's exactly what I've been getting at :D The differentiator isn't really a list of filename suffixes, but a role the file plays in SvelteKit...

Anyway, if my change is not accepted, then I would like to think a little bit more about how fileType and svelteKitFileType interact...

marekdedic · 2025-01-06T11:29:16Z

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

+
+export type SvelteContext = {
+	svelteVersion: string;
+	fileType: '.svelte' | '.svelte.[js|ts]';


And yes, the list of "file role flags" would probably be longer and would need to expand over time and some rules would check multiple flags at once...

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

packages/eslint-plugin-svelte/src/utils/index.ts

marekdedic · 2025-01-06T11:36:29Z

packages/eslint-plugin-svelte/src/utils/index.ts

+	if (
+		isNotSatisfies(svelteContext.svelteVersion, condition.svelteVersions) ||
+		isNotSatisfies(svelteContext.fileType, condition.fileTypes) ||
+		isNotSatisfies(svelteContext.runes, condition.runes) ||


runes are a boolean, isNotSatisfies doesn't work for that...

I already changed it to array. How can I make "runes" even more plural?😅

baseballyama · 2025-01-06T12:21:22Z

@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!

packages/eslint-plugin-svelte/src/types.ts

marekdedic · 2025-01-07T13:26:34Z

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

marekdedic

One final thing and then we're done :)

packages/eslint-plugin-svelte/src/types.ts

marekdedic

The new SvelteContext is much clearer, thanks! Unfortunately, that means there's new things for me not to understand :D

marekdedic · 2025-01-08T21:22:01Z

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

+export type SvelteContext = (
+	| ({
+			svelteVersion: '3/4';
+	  } & {


Why is this split into 2?

For more strict typing. Reinforcing the types eliminates improbable patterns.

I don't understant, I thought {a: string} & {b: int} is equal to {a: string; b: int}, isn't it?

updated: 0e28cc1

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

marekdedic · 2025-01-08T21:26:32Z

packages/eslint-plugin-svelte/src/utils/svelte-context.ts

+			| {
+					svelteFileType: '.svelte' | '.svelte.[js|ts]';
+					/** If a user uses a parser other than `svelte-eslint-parser`, `undetermined` will be set. */
+					runes: boolean | 'undetermined';


Hmm, I'm having a hard time understanding the difference between null and 'undetermined' here - they both mean we couldn't figure it out, no?

We can automatically recognize this via typing.

null: The concept of runes itself does not exist.

true: Using runes

false: Not using runes

undetermined: The concept of runes exists but I don't know if it is used

Ah, ok. Is there any menaingful difference between runes don't exist and not using runes? Because they semm to me to be equivalent in all useful aspects...

I think only facts should be expressed here, as usefulness depends on context

…-control

baseballyama · 2025-01-11T12:51:11Z

@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.👍

marekdedic · 2025-01-11T18:56:08Z

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

baseballyama requested a review from ota-meshi December 30, 2024 14:12

baseballyama mentioned this pull request Dec 30, 2024

breaking: Redesigned ESLint config #957

Closed

baseballyama commented Dec 30, 2024

View reviewed changes

This was referenced Dec 30, 2024

Roadmap #966

Open

chore: removed Svelte 4 specific rules from recommended set #943

Closed

baseballyama added 4 commits January 3, 2025 17:38

feat: Implement util to conditionally run lint based on Svelte versio…

0b44be4

…n and SvelteKit routes etc.

add changeset

4ca476a

fix and refactor

4fbe97d

re implement

cb70fea

baseballyama force-pushed the feat/fine-gained-file-control branch from 7d2c565 to cb70fea Compare January 3, 2025 09:39

baseballyama mentioned this pull request Jan 3, 2025

Deprecate rules no-export-load-in-svelte-module-in-kit-pages and valid-prop-names-in-kit-pages #901

Open

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/types.ts Outdated Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/utils/svelte-context.ts Outdated Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/utils/svelte-context.ts Outdated Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/utils/index.ts Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/utils/index.ts Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/types.ts Outdated Show resolved Hide resolved

marekdedic reviewed Jan 3, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/types.ts Outdated Show resolved Hide resolved

baseballyama requested a review from marekdedic January 5, 2025 02:16

marekdedic suggested changes Jan 6, 2025

View reviewed changes

baseballyama requested a review from marekdedic January 6, 2025 12:18

number to string

531911a

marekdedic reviewed Jan 7, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/types.ts Show resolved Hide resolved

baseballyama added 5 commits January 7, 2025 22:54

-> doesNotSatisfy

a36d935

-> svelteFileType

bbfae0e

fix bug

8a02534

refactor

0707e79

fix

280ae67

marekdedic suggested changes Jan 7, 2025

View reviewed changes

packages/eslint-plugin-svelte/src/types.ts Show resolved Hide resolved

add undetermined

afd422d

baseballyama force-pushed the feat/fine-gained-file-control branch from 186c8ab to afd422d Compare January 7, 2025 22:34

more robust

f686b4d

baseballyama force-pushed the feat/fine-gained-file-control branch from eab3041 to f686b4d Compare January 7, 2025 23:27

baseballyama requested a review from marekdedic January 8, 2025 02:15

remove other

ae40057

marekdedic reviewed Jan 8, 2025

View reviewed changes

baseballyama added 3 commits January 9, 2025 09:48

js -> js|ts

4640ef5

Merge remote-tracking branch 'origin/main' into feat/fine-gained-file…

d7b588f

…-control

simplify the type

0e28cc1

baseballyama changed the title ~~feat: Implement util to conditionally run lint based on Svelte version and SvelteKit routes etc~~ feat: add control for executing rules based on Svelte/SvelteKit context Jan 11, 2025

baseballyama merged commit 0b28198 into main Jan 11, 2025
18 checks passed

baseballyama deleted the feat/fine-gained-file-control branch January 11, 2025 12:52

github-actions bot mentioned this pull request Jan 11, 2025

chore: release eslint-plugin-svelte (next) #994

Merged

baseballyama mentioned this pull request Jan 12, 2025

fix: update method for extracting major version #1007

Merged

feat: add control for executing rules based on Svelte/SvelteKit context #980

feat: add control for executing rules based on Svelte/SvelteKit context #980

Conversation

baseballyama commented Dec 30, 2024 • edited Loading

changeset-bot bot commented Dec 30, 2024 • edited Loading

🦋 Changeset detected

pkg-pr-new bot commented Dec 30, 2024 • edited Loading

baseballyama commented Dec 30, 2024

marekdedic commented Jan 2, 2025

baseballyama commented Jan 2, 2025

marekdedic commented Jan 2, 2025

marekdedic commented Jan 2, 2025

baseballyama commented Jan 2, 2025 • edited Loading

marekdedic commented Jan 2, 2025

baseballyama commented Jan 2, 2025

baseballyama commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekdedic Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

baseballyama Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

marekdedic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baseballyama Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

baseballyama commented Jan 6, 2025

marekdedic commented Jan 7, 2025

marekdedic left a comment

Choose a reason for hiding this comment

marekdedic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekdedic Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baseballyama commented Jan 11, 2025

marekdedic commented Jan 11, 2025

baseballyama commented Dec 30, 2024 •

edited

Loading

changeset-bot bot commented Dec 30, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 30, 2024 •

edited

Loading

baseballyama commented Jan 2, 2025 •

edited

Loading

marekdedic Jan 7, 2025 •

edited

Loading

baseballyama Jan 7, 2025 •

edited

Loading

baseballyama Jan 6, 2025 •

edited

Loading

marekdedic Jan 10, 2025 •

edited

Loading