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

Rethinking rel=external #3935

Closed
mrkishi opened this issue Feb 15, 2022 · 4 comments · Fixed by #3969
Closed

Rethinking rel=external #3935

mrkishi opened this issue Feb 15, 2022 · 4 comments · Fixed by #3969
Labels

Comments

@mrkishi
Copy link
Member

mrkishi commented Feb 15, 2022

Describe the problem

Right now, SvelteKit requires us to add a rel="external" attribute to links that should cause a regular browser navigation instead of being handled by the client-side router. This allows us to direct users to routes in the same domain that are handled outside SvelteKit.

There are a few issues I can see:

  • rel="external" is supposed to denote a link points to an external site. While different routes on a single domain being served by SvelteKit and other services could mean exactly that, they are orthogonal concepts—they could arguably be considered a single site. The impact is probably minor but unknown: search engines could use this unfavorably.
  • It's required not only for external services, but also for static assets and standalone endpoints, which is surprising.
  • It optimizes for the exceptional case:
    • The router will render a 404 for any unmatched routes under the app's domain, with the benefit that it doesn't cause a full page navigation and doesn't hit the server.
    • However, 404s are the exception, not the norm: if you have a /external link and it doesn't match a route, the expectation is that it exists as a standalone endpoint, static asset or separate service. If it was in fact a link to SvelteKit-handled route that should 404, it'd arguably be the exception.

Describe the proposed solution

With that in mind, I propose that the client-side router should relegate links without a matching route to the browser for a full page navigation by default.

  • In the usual case, that would be the desired effect: it'd be a static asset, a standalone endpoint or an external server.
  • In the exceptional case (invalid or stale urls, etc), the experience would gracefully degrade: SvelteKit would end up ssr'ing the 404 page and the client-side app would restart normally.

On cases where the link would match a SvelteKit route (eg. /[...path].svelte as a catch-all would mean any route would match), we'd still need an attribute to disable SvelteKit's client-side routing. We could keep rel="external" or come up with a SvelteKit-specific attribute like sveltekit:noroute (we have the sveltekit:noscroll prior art, but it does sound odd 🐐).

Alternatives considered

This change would mean the exceptional case (a non-route link should be handled by SvelteKit's client-side router) would require a round-trip. We could, instead, aim to minimize the amount of sveltekit:noroute links: instead of requiring it on static assets, standalone endpoints and external services, we could require it only on external services by either:

  • Making the client-side router aware of standalone endpoints and static assets: not ideal as it'd increase the payload for the app, in some cases considerably ([breaking] only route pages on the client-side #2656 got rid of endpoints from the payload).
  • Automatically adding the attribute at compile-time if we see a matching endpoint or asset: viable improvement.

Still, I don't think it'd be worth the hassle to add the attribute to all externally handled routes just to skip a navigation on 404s, as I'd rather strive to eliminate 404s instead.

Importance

would make my life easier

Additional Information

No response

@Conduitry
Copy link
Member

I'm -1 on changing this. If you don't like overloading and having to imply the other things that rel=external already means, you can use a target=_self instead.

@Rich-Harris
Copy link
Member

I think this change makes sense. @Conduitry what's the reason not to do it? I don't think it would even be a breaking change, unless we consider server-rendering 404s for broken links (instead of rendering them in the client) to be breaking.

@Conduitry
Copy link
Member

I'll withhold judgement until I read the proposal carefully again. I may have been having a negative knee-jerk reaction to what sounded like "let's throw away all the years of precedent for client-side routing so that we can do something that might be more helpful by default but which is harder to explain and to understand and to customize if you need to".

@mrkishi
Copy link
Member Author

mrkishi commented Feb 16, 2022

re. harder on-ramping: right now understanding rel="external" is needed when linking static assets, standalone endpoints and routes handled externally. With this proposal, that'd drop to a subset of routes handled externally—namely, only those in conflict with matching SvelteKit pages.

Everything else would work as usual, with the exception that 404s would be handled server-side instead of client-side. Since 404s are the exception, I believe that's an acceptable trade-off.

Rich-Harris added a commit that referenced this issue Feb 16, 2022
Rich-Harris added a commit that referenced this issue Feb 17, 2022
)

* failing test for #3935

* fall back to full page navigation for unmatched routes - closes #3935

* changeset

* fix initial spa render of 404 errors (#3980)

Co-authored-by: mrkishi <mauriciokishi@gmail.com>

Co-authored-by: mrkishi <mauriciokishi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants