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

Add rule no-link-without-base #891

Open
marekdedic opened this issue Oct 28, 2024 · 4 comments · May be fixed by #900
Open

Add rule no-link-without-base #891

marekdedic opened this issue Oct 28, 2024 · 4 comments · May be fixed by #900
Labels
enhancement New feature or request new rule

Comments

@marekdedic
Copy link
Contributor

marekdedic commented Oct 28, 2024

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 #679 (comment)

@marekdedic marekdedic added enhancement New feature or request new rule labels Oct 28, 2024
@marekdedic
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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:

  • goto should only be used for internal navigation and should always prepend base. However, this rule may eventually be obsoleted by Prepend base in goto kit#11803.
  • <a> links can be used for both internal and external navigation (both are valid use-cases). So only relative links should be checked.
  • 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).

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

@ota-meshi
Copy link
Member

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 sveltejs/kit#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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants