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: added the no-goto-without-base rule #679

Merged
merged 14 commits into from
Mar 4, 2024

Conversation

marekdedic
Copy link
Contributor

Closes #675

Copy link

changeset-bot bot commented Jan 28, 2024

🦋 Changeset detected

Latest commit: 9a07b52

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 marekdedic marked this pull request as ready for review January 29, 2024 14:13
@marekdedic
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

baseballyama commented Feb 6, 2024

Added goto interface improvements in kit 3.0 milestones.
sveltejs/kit#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
Copy link
Contributor Author

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
Copy link
Member

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🙏)

src/rules/no-goto-without-base.ts Outdated Show resolved Hide resolved
@ota-meshi
Copy link
Member

ota-meshi commented Mar 3, 2024

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

@marekdedic
Copy link
Contributor Author

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

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 updating this PR! LGTM!
Also, thank you for your opinion. Let's make checking for the <a> tag a separate rule, as different users may have different preferences.
(I fixed the CI error in another PR.)

@ota-meshi ota-meshi merged commit 4e6c681 into sveltejs:main Mar 4, 2024
9 of 12 checks passed
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.

Add rule no-goto-without-base
3 participants