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

Fall back to full page reload if link href doesn't match anything #3969

Merged
merged 4 commits into from Feb 17, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 16, 2022

Currently, clicking a link like <a href="/foo"> will fail if /foo doesn't 'belong' to the app — it will result in the error page being rendered with a synthetic 404. In cases where /foo is a real resource (handled by a separate app on the same domain, or a file in static, etc) we need to add rel="external" to trick the router into ignoring it in order to avoid that unwanted behaviour.

With this PR, if a link doesn't match a route in the manifest, we fall back to a full page navigation instead, allowing the browser to access the resource. In the case where the link doesn't go anywhere, Kit will still respond with a 404, it'll just be from the server instead of the client-side router.

Closes #3935.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2022

🦋 Changeset detected

Latest commit: eaba628

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@Conduitry
Copy link
Member

Are you thinking we'd adjust any of the rules that the router already uses to know to perform a regular navigation to a link, or just add this one on top of them?

We should also be able to tidy the /chat link in the header with this. I don't remember what repo that lives in.

@Rich-Harris
Copy link
Member Author

We're not really adding any rules, we're just changing what happens when the router fails to match anything

@Rich-Harris
Copy link
Member Author

Hmm. This is failing because adapter-static has a test for SPAs: if you deploy your SPA somewhere and use a fallback 200.html page to capture all requests...

const handler = sirv(`${cwd}/build`, {
single: '200.html'
});
...then previously SvelteKit would be responsible for rendering a 404 if nothing matched.

With this change that won't be the case. I think that's okay, but it means users will see the platform 404 page rather than the app 404 page. Does that make it a breaking change? Does it change our plans at all?

@Conduitry
Copy link
Member

Wait, what's changing with the static adapter output? How does anything decide whether to render the host's 404 page instead of your app's catch-all page? That would only happen if it were configured to only catch certain routes and not all of them, right?

@Rich-Harris
Copy link
Member Author

I was slightly mistaken in my diagnosis. Nothing is changing with the output, but one of the tests navigates to /nosuchpage and expects to see a SvelteKit error page in response. Instead, SvelteKit attempts a full page navigation to /nosuchpage, which causes an infinite loop.

@mrkishi
Copy link
Member

mrkishi commented Feb 17, 2022

That makes sense as browser delegation shouldn't happen on first render, only on subsequent navigations, yet start({ spa: true }) will trigger a goto() during initialization.

I guess we could add an extra forceClient option to goto() that'd make its way into renderer.handle_navigation, but since its use would be extremely limited [1], keeping that flag private inside the router and flipping it after init_listeners could be an alternative.

[1] the one scenario where this would make sense is an application that wanted to serve a url from outside SvelteKit when hit directly, but still have it accessible as a SvelteKit route when navigating client-side. I wouldn't dare saying no one would find a use-case for it, but it'd be very unorthodox. A single refresh would switch you from one to the other.

e: here's a take on the private flag version.

Co-authored-by: mrkishi <mauriciokishi@gmail.com>
@Rich-Harris
Copy link
Member Author

Thanks, that was what I landed on when I shut my laptop last night, so I was pleased to wake up to your branch. Feeling good about this change

@PH4NTOMiki
Copy link
Contributor

@Rich-Harris this kinda has a flaw, when using SPA mode, when 404 occurs this leads to infinite reloads. We have to fix this somehow, maybe set some sessionStorage value that counts number of redirects and if greater than 1 then stop. or something like that.

@PH4NTOMiki
Copy link
Contributor

I know there is code for initialized in this PR but still it breaks somehow

@PH4NTOMiki
Copy link
Contributor

Not found gets rendered for a small time, and then reload, and then again.

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.

Rethinking rel=external
4 participants