Less strict `no-navigation-without-resolve` default

[jycouet]

Description

Since no-navigation-without-resolve is now in recommended (thx to #1308) it's triggering some discussions in different places.

In sv -> sveltejs/cli#702
About tutorial -> #1308 (comment)
Here 1 -> #1327
Here 2 -> #1319

Maybe, at the moment, the default config is a bit too aggressive.
What do you think about relaxing it a bit like:

{
  "svelte/no-navigation-without-resolve": [
    "error",
    {
      "ignoreGoto": false,
-      "ignoreLinks": false,
+      "ignoreLinks": true,
      "ignorePushState": false,
      "ignoreReplaceState": false
    }
  ]
}

Let me know

[marekdedic]

I think we should fix the issues with the rule (working on it, but PRs/reviews are slow-ish :( ) and then decide. Also, I don't see why links should be ignored and the others left turned on.

[jycouet]

It looks like quite a few issues have been raised recently regarding no-navigation-without-resolve, probably because it’s now part of the recommended set.

I was thinking it might be better to make the rule a bit less aggressive while it’s still fresh. That way, adoption could be smoother in the long run. (as ignoreLinks is the first hit, I was suggesting this one, but maybe all reverted for now could be an option too)

Relaxing the recommendation a little would also mean we don’t have to change defaults later on (in ou projects), and it would give us (You? ^^, maybe me? :) ) time to improve things in a more relaxed manner.

[joeychilson]

I think we should fix the issues with the rule (working on it, but PRs/reviews are slow-ish :( ) and then decide. Also, I don't see why links should be ignored and the others left turned on.

I think the problem people are running into with links compared to the other options is links can be external and internal and I don't think it possible to resolve a link that's optional/undefined because there is not way to know if the link is external or internal. I think this is the reason someone would ignore links.

[hyunbinseo]

I am having problems with external links as well.

Sure, I can ignore them individually, but it's a pain.

<script>
  import { PUBLIC_GITHUB } from '$env/static/public';
</script>

<!-- Found a link with a url that isn't resolved. -->
<a href={PUBLIC_GITHUB} target="_blank">TEXT</a>,

[Narigo]

This rule affects my page quite a bit as well and I find myself ignoring many of the errors as I used variables. It looks like they don't get checked whether they used resolve or may even come from an external source.

Also, I'm unsure whether a link flagged rel="external" should be checked at all.

Maybe a good compromise could be able to ignore links that are variables? See example given by @hyunbinseo
So instead of having a boolean setting, the options could allow something like: "all" | "variables" | "none"

At the same time I'm a bit confused to use options like ignoreLinks: false (or ignoreLinks: "none" if using the proposal) to enable it (double negation).

[echocrow]

+1 re relaxing or revising the evaluation. we had to fully turn if off for now.

though great for destinations defined in source code, in our case the vast majority of href={..} and goto(..) instances source their values from a CMS.

• when we pass those string values into <a href> or goto() as-is, this rule complains that the link is unresolved.
• when we follow the rule's suggestion and wrap the value in resolve(), TS then complains: "Type [string]  is not assignable to type  [route: ...].
  • (depending on the usecase, this may also not be the right solution, e.g. when a base path is used, and the external source already has that base value embedded)

unsure if feasible in eslint, but should this rule perhaps only evaluate string literals? e.g.

• check: goto('/string/literal')
• likely check: goto(`{base}/template/literal`)
• likely check: goto('/string' + '/expression')
• ignore: goto(someVariable)

[marekdedic]

Hi,
the rule currently evaluates string concatenation, template literals and variables. There is one big limitation, which is that eslint works per-file, so if you import absolute URL variables, there's no possible way to detect that.

I'd like to add support for detecting imported resolved values based on their TS types (see #1321), but I'm currently blocked by other PRs not being merged yet.

[echocrow]

I'd like to add support for detecting imported resolved values based on their TS types (see #1321), but I'm currently blocked by other PRs not being merged yet.

neat idea! but unfortunately i don't think this would resolve false reports when the destination is sourced externally and types as string?

is there room for a rule option to disabling checks for variables? might be a duct-tape solution tho

[marekdedic]

neat idea! but unfortunately i don't think this would resolve false reports when the destination is sourced externally and types as string?

No, it wouldn't, that's the limitation I wrote about.

is there room for a rule option to disabling checks for variables? might be a duct-tape solution tho

How about an option to ignore imported variables?

[jycouet]

I think that it's all about timing. It was probably a bit early to put it in recommended with all these small things left. (at the same time it's was good to discover them!)

I originally suggested "ignoreLinks": true, but maybe it could be out of recommended until it's solid?

Having searchParams seems to be an issue as well.

[marekdedic]

I think that it's all about timing. It was probably a bit early to put it in recommended with all these small things left. (at the same time it's was good to discover them!)

In retrospect yes, but the rule has been in the plugin since March 2024 (in its older variant no-goto-without-base) and we only got all of the reports since including it in recommended 🤷

Having searchParams seems to be an issue as well.

Yes, that's in another issue as well :/

[jycouet]

Yes, people don't opt-in rules regularly ^^
The older variant was not that strict (resolve was not there).

IMO, resolve should also manage searchParams, but it's some work to do (not identified). If it's managed one day inside resolve, then it's not a eslint direct concern.