kit icon indicating copy to clipboard operation
kit copied to clipboard

fix: handling 404 when redirect from +layout.ts in SPA mode

Open manhhungpc opened this issue 1 year ago • 4 comments

When redirect to an unknow route in SPA mode, which has a root load function in +layout.ts, it will fallback to the root +error.svelte instead of a blank page. See #11099. Issue #11494 seems to be releated


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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] 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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

manhhungpc avatar Feb 01 '24 04:02 manhhungpc

⚠️ No Changeset found

Latest commit: cf60aeba4866a204496dbb8999010417399c70e9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Feb 01 '24 04:02 changeset-bot[bot]

Thanks for opening this PR, although I’m not sure if this is the right solution. We need to load the root layout when rendering the root error page. I’m guessing the tests are failing because of the root layout node being omitted. I think we should be catching the error instead as I explained here https://github.com/sveltejs/kit/issues/11099#issuecomment-1823044646 adding a test for this scenario should help with testing the solution

teemingc avatar Feb 15 '24 12:02 teemingc

Thanks for pointing that out! And I'm working on your solution

manhhungpc avatar Feb 17 '24 14:02 manhhungpc

I've marked this as a draft for now. Please hit "ready for review" when it's ready for another look

benmccann avatar Feb 18 '24 16:02 benmccann

closed in favour of https://github.com/sveltejs/kit/pull/12005

teemingc avatar Mar 19 '24 17:03 teemingc