feat: added the `no-goto-without-base` rule

sveltejs / eslint-plugin-svelte

ESLint plugin for Svelte using AST

https://sveltejs.github.io/eslint-plugin-svelte/

MIT License

277 stars 30 forks source link

feat: added the `no-goto-without-base` rule #679

Closed marekdedic closed 4 months ago

marekdedic commented 5 months ago

Closes #675

changeset-bot[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: 9a07b52d5e8154d9b30f834c7c2547c510701667

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

marekdedic commented 5 months ago

Hi, this is basically done from my side. However:

I would really like a review from somebody who knows SvelteKit better than me
There are probably some exceptions, when you don't want this rule to trigger - if you know of any, I'd add them in, but I could think only of absolute URLs (but since goto is not for external URLs, maybe absolute URLs should be prohibited altogether?). They can be added later of course or put behind some config flag.

ota-meshi commented 5 months ago

Thank you for this PR! But, I'm not familiar with creating web apps with SvelteKit. @baseballyama Can you check if what the rules report useful or not?

baseballyama commented 5 months ago

Added goto interface improvements in kit 3.0 milestones. https://github.com/sveltejs/kit/issues/11803

Until then, I think this rule is useful but I think it's not plugin:svelte/recommended rule because part of users doesn't use basepath.

marekdedic commented 5 months ago

Hi, thanks, I thought about why SvelteKit just doesn't do this by default, but it feels like a quite big breaking change, so I didn't propose it to kit directly, maybe I should've...

I don't think this needs to be recommended, but actually if you don't use basepath, prepending it is still correct in the sense that it doesn't break anything and enables you to use a basepath down the line. But if this is something that would be added to SvelteKit itself down the line, it doesn't really make sense to me to make this recommended either...

baseballyama commented 5 months ago

In my opinion, anyway, ESLint allows users to set rules freely, so as long as the rules are not annoying, they can be merged. I will check this PR will in this weekend. (Sorry for the delay but I’m bit busy for my work now🙏)

ota-meshi commented 4 months ago

Maybe this rule should also check <a href="...">? What do you think?

marekdedic commented 4 months ago

Maybe this rule should also check <a href="...">? What do you think?

Well, that's a little bit more complicated. That is because as per the SvelteKit docs, you should use goto() for internal navigation (where it always makes sense to check for the base path). <a> links can be use for both internal as well as external navigation. We could, however, maybe check all <a> elements and report missing base iff the path is relative...