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

Why is redirect not supported from __error.svelte cmpt? #1574

Closed
alexbjorlig opened this issue May 28, 2021 · 12 comments
Closed

Why is redirect not supported from __error.svelte cmpt? #1574

alexbjorlig opened this issue May 28, 2021 · 12 comments
Labels
error handling p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Milestone

Comments

@alexbjorlig
Copy link
Contributor

I have a small reproducible example here.

In the __error.svelte cmpt I have thrown in a redirect like this in the load function:

 return {
                status: 302,
                redirect: '/login'
            }

But this is not working. Is this a bug, not allowed - or do I misunderstand something here?

@benmccann
Copy link
Member

benmccann commented May 28, 2021

It looks like the desired use case here is to redirect instead of showing a 404 page:

https://github.com/alexbjorlig/sveltekit-redirect-question/blob/e6fe88e47fda8d04c01a83233ee9eaa9428ea774/src/routes/__error.svelte#L7

Related bug about not being able to redirect from a layout page: #1575

@benmccann benmccann added this to the 1.0 milestone May 28, 2021
@alexbjorlig

This comment has been minimized.

@frederikhors

This comment has been minimized.

@benmccann benmccann added p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. error handling labels Aug 4, 2021
@benmccann
Copy link
Member

benmccann commented Aug 6, 2021

One danger here is a possible infinite redirect if you redirect to a page that has an error or can't be found, etc. But as long as the infinite redirect is happening in the browser so that the developer can see it (which I think it would) then that's their problem

@benmccann
Copy link
Member

We just need to add a check for error_loaded.loaded.redirect right before this line:

branch = branch.slice(0, j + 1).concat(error_loaded);

@bleucitron
Copy link
Contributor

bleucitron commented Aug 6, 2021

From what i could try, for 404s, server/page/respond never gets called since, when accessing an unknown page, it never gets past

if (!route.pattern.test(request.path)) continue;

and goes straight to respond_with_error, which does not seem to implement the redirect features.

I haven't tested for errors other than 404 yet, behaviour probably is different.

@benmccann
Copy link
Member

Ah, yeah, it looks like 404 and the rest are implemented differently. My last comment is about how you'd implement for the rest

One thing we'd need to test is to make sure you can return a redirect status on _error.svelte (e.g. { status: 302, redirect: ... }). If you leave the status as 404 the redirect will be ignored by the browser: #2068 (comment)

@bleucitron bleucitron mentioned this issue Aug 7, 2021
7 tasks
@bleucitron
Copy link
Contributor

bleucitron commented Aug 7, 2021

Some work is being done here, still in draft.

I think i handled redirections from __error in these cases:

  • server redirect on errors
  • server redirect on 404
  • client redirect on errors
  • client redirect on 404
  • client redirect only on browser (edgy)

Would there be other cases i missed ? Let me know if anything pops to mind.
I might also need to write some tests, but i haven't looked into it that much yet.

I'll probably open a PR in the days to come.

P.S: There is also #1575 that could be handled in this PR, but maybe i should work on it in a different PR. What do you think ?

@benmccann
Copy link
Member

That's great! Thanks so much

It looks good so far. Tests would be a good idea. You could add them in packages/kit/test/apps/basics/src/routes/redirect/ based off the existing tests

#1575 could be handled in the same PR or a different PR

@benmccann
Copy link
Member

I talked about this with the rest of the maintainers at today's maintainer's meeting. Folks suggested you could already do this in one of a couple ways:

  • for the 404 case, create a [fallthough].svelte that would catch any unhandled routes and redirect there
  • for other cases, do it in handle. (call resolve, check the returned status, and then do a redirect based on that)

There was a consensus that redirecting on error pages is in general a bit of a weird thing to do and so folks didn't want to add support for it directly into the router, but instead suggested it be handled in these other ways since it's a relatively uncommon use case

@PlopTheReal
Copy link

Hey I've added a [fallthough].svelte but it doesn't catch 404 form urls with subsequent path:
/someInvalidUrl => ok
/someInvalidUrl/otherPage => not ok
Any ideas to catch all urls that are giving 404s?

@PaintingWithCode
Copy link

@PlopTheReal I don't know if you were able to solve it, but I managed to get a catch-all 404 redirect working by adding a +error.svelte at the root of /routes with the following script:

<script>
	import { page } from '$app/stores';
	import { goto } from '$app/navigation';

	if ($page.status === 404) {
		goto('/');
	}
</script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants