sveltejs / eslint-plugin-svelte

ESLint plugin for Svelte using AST
https://sveltejs.github.io/eslint-plugin-svelte/
MIT License
306 stars 38 forks source link

Add rule `no-link-without-base` #891

Open marekdedic opened 3 weeks ago

marekdedic commented 3 weeks ago

Motivation

The motivation is basically similar to #675 - when doing internal navigation, you almost always want to prepend the base path. However, unlike goto, links can also be used for external navigation. To navigate this complicated issue, I propose to only check relative links (at least by default).

Description

Add a rule that would trigger on any link without a base path. However, there are quite some edge cases

Examples

<script>
  import { base } from "$app/paths";
  import { base as whatever } from "$app/paths";
  import { goto } from "$app/navigation";
</script>

  <!-- ✓ GOOD -->
<a href={base + "/foo"}>Text</a>
<a href={`${base}/foo`}>Text</a>
<a href={whatever + "/foo"}>Text</a>
<a href={`${whatever}/foo`}>Text</a>
<a href="https://absolute.url">Text</a>

  <!-- ✗ BAD -->
<a href={"/foo"}>Text</a>
<a href={"/foo" + base}>Text</a>
<a href={`foo/${base}`}>Text</a>
</svelte>

Additional comments

Based on https://github.com/sveltejs/eslint-plugin-svelte/pull/679#issuecomment-1975067137

marekdedic commented 3 weeks ago

@ota-meshi There's also pushState and replaceState - I could add a rule for them or fold them into no-goto-without-base, what do you think? Or maybe all of this could be merged into one no-navigation-without-base rule?

ota-meshi commented 3 weeks ago

Thank you for proposing the rule!

There's also pushState and replaceState

Shouldn't we use goto() instead of pushState() and replaceState()? I think there should probably be another rule that forbids any scripted navigation other than goto(). What do you think?

I could add a rule for them or fold them into no-goto-without-base, what do you think? Or maybe all of this could be merged into one no-navigation-without-base rule?

As you say, I think no-link-without-base and no-goto-without-base could be a single no-navigation-without-base rule. I think the purpose of these rules is the same.

marekdedic commented 3 weeks ago

Thinking about this more. pushState and replaceState serve a slightly different purpose from goto (shallow routing). For example, you cannot use goto to navigate to the current URL with different state, you have to use pushState. So all three are valid and useful.

As for merging the rules, there are some additional considerations:

How would we go about a unified rule? Deprecate no-goto-without-base and introduce a new, broader rule?

ota-meshi commented 3 weeks ago

Deprecate no-goto-without-base and introduce a new, broader rule?

I like this idea, and I also like your suggestion for the new rule name no-navigation-without-base.

Thinking about this more. pushState and replaceState serve a slightly different purpose from goto (shallow routing). For example, you cannot use goto to navigate to the current URL with different state, you have to use pushState. So all three are valid and useful.

I see, thank you for the explanation 😄

goto should only be used for internal navigation and should always prepend base. However, this rule may eventually be obsoleted by https://github.com/sveltejs/kit/issues/11803.

The rule will need to change when the API changes, but until then I think it's useful for people.

<a> links can be used for both internal and external navigation (both are valid use-cases). So only relative links should be checked.

Yeah 👍 I think it would be useful for people to have that check implemented.

pushState and replaceState can only be used for internal navigation. Unlike goto, you can also pass an empty string to them which preserves the current URL (including base).

So that means we should allow things like pushState(x, "", "?foo=42"), right?