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 from __layout.svelte ignored, when an unkown route is hit? #1575

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

Comments

@alexbjorlig
Copy link
Contributor

I have a small reproducible example here.

In the above repo, there is __layout.svelte file, that will redirect like this:

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

Works great when a route is matched, but if there is an error the redirect is not respected.
The load function does actually run, but the browser is not redirected to login.

Is this a bug, not allowed - or do I misunderstand something here?

@benmccann
Copy link
Member

benmccann commented May 28, 2021

Can you explain your use case and why you'd want to redirect in the layout even if an error occurs?

@alexbjorlig
Copy link
Contributor Author

I expect the following:

  1. User attempts to access application, an unknown route is hit
  2. __layouat.svelte is then processed, and determines in this case it's best to redirect to /login

BUT the above is not what happens, despite the layout's load function actually being called(!)

@benmccann
Copy link
Member

Right. But what's your use case? I.e. what application are you building, what is the user doing in your application, what are you trying to accomplish?

@alexbjorlig
Copy link
Contributor Author

alexbjorlig commented May 30, 2021

My usecase here is to redirect user to login, when trying to access a route that does not exists. Combined with layout resets, that would be a powerfull way to show some users 404 pages, and redirect others to login (depending on what layout is used).

@benmccann
Copy link
Member

If you have a redirect on the error page as requested in #1574 why do you need one on the layout to do that?

@alexbjorlig
Copy link
Contributor Author

Ok so It's a bit hard to explain without a conversation for me. But let's be concrete. Why is a redirect from __layout.svelte ignored? The load function runs, and returns a redirect. The redirect is then, silenty, ignored. That's is the purpose of this issue to put clarification to. As a developer, I expect a redirect to happen when I return a redirect 😅

@benmccann
Copy link
Member

Why is a redirect from __layout.svelte ignored?

It's not ignored in the general case. But if you hit an error then it might not have a chance to execute. That actually seems like the more intuitive behavior in my mind.

@Kapsonfire-DE
Copy link
Contributor

Kapsonfire-DE commented Jun 3, 2021

But the __error.svelte is using the __layout.svelte, isnt it?
so the redirect should execute. this is not the case - I had this issue before.
Why I needed it: I didnt want to give the user a 404 Error page for my mobile game - Instead I wanted to recover to a proper state (home route). I ended up, to make a redirect on the __error.svelte itsself with goto function - this worked.

But to sump up, the bug is here. __error.svelte ignores the redirect of the __layout.load
But there are circumstances which should be pointed out....

What happens if multiple redirect statements are up here, like nested layouts or one in layout and one in the route itsself?

@benmccann
Copy link
Member

I didnt want to give the user a 404 Error page for my mobile game - Instead I wanted to recover to a proper state (home route)

Wouldn't you expect to put the redirect in __error.svelte in this case? I think __error.svelte is called when navigating to a route that doesn't exist. I'm not quite sure how you'd detect this in __layout (there's an open issue to be able to detect if it's being rendered with an error state #598)

@Kapsonfire-DE
Copy link
Contributor

the issue goes for __error.svelte as well, it does not respect the redirect return - you linked it already
#1574

But in my case, I worked in folders and nested layouts and with reset, So i knew, if the basic layout loaded, its either 404 or not logged in - special case. but still we have to point out

  1. why is it not respected?
  2. what happens if multiple files returns a redirect (nested layouts or layout + route)

@annaghi
Copy link

annaghi commented Jul 5, 2021

My use case is the following: when a page not found, instead of a dedicated error page I would like to be able to display a notice on the page to which I redirected to. Something like this:

page-flows

I can imagine two solutions for this:

  • define explicitly the page to redirect to, and be able to catch if a page displayed "normally" or as a result of redirection
  • automatically try to find a matching route by cutting out more and more segments from the end of the URL. For example:
    given a non-existing URL:
    mysite.com/existing/path/ruined/here -> non-existing, try to redirect
    mysite.com/existing/path/ruined -> still non-existing, try further
    mysite.com/existing/path/ -> this one exists, redirect here, and if needed display a notice about the missing page

The latter might be useful in case of non-existing details page automatically redirect to the list page:
/posts/non-existing-slug -> /posts

@benmccann
Copy link
Member

I'm still not understanding how the layout is involved in that case

@Kapsonfire-DE
Copy link
Contributor

Think about a /admin/ route... Put there a layout file, and do the isAdmin check here in the load function of the layout file, and redirect if not logged in to a login page.

Doing this is useful, because you dont have to do it in every route of the /admin/ folder
/admin/users.svelte
/admin/news.svelte
/admin/groups.svelte

and so on....

But well we could just export a variable in load function and check it afterwards and redirect with goto

@benmccann
Copy link
Member

@Kapsonfire-DE if you're hitting those routes and they exist then it's not a 404. The question would be what should happen when you hit /admin/does-not-exist. Should you 404 or redirect to the login page? I'd probably say that you should 404

@Kapsonfire-DE
Copy link
Contributor

in that case yes - but I have another usecase in my mobile game, where i want to reset to a valid state and in that case im gonna redirect

@allezxandre
Copy link
Contributor

allezxandre commented Sep 9, 2021

I think I might have figured out a likely culprit to missing redirects (be it in __layout files or even their leaf pages).

The redirect returned by your page's load function gets lost when the result goes through _get_navigation_result_from_branch: the result returned by the function never includes a redirect.

So after this function call of _get_navigation_result_from_branch, this check of a redirect never passes, and a redirect is missed even if a load function returned a redirect.

@baseballyama
Copy link
Member

Now I'm working on #2424.
And inside this issue, there is a comment that #2424 is maybe related to this.

So I read all comments but still, I'm confusing what is the issue.

I summarized this issue.
So Could you please tell me if wrong or lacking point is there.
And let me know about the Issue3 about your opinion.

(CLOSED) Issue1 : Redirect if request page is 404

User attempts to access application, an unknown route is hit
__layouat.svelte is then processed, and determines in this case it's best to redirect to /login

in that case yes - but I have another usecase in my mobile game, where i want to reset to a valid state and in that case im gonna redirect

Already maintainers talked about it at maintainer's meeting.
And their suggestion is below way (Please see the link).
#1574 (comment)

Some comments in the issue mentioned regarding 404 case,
but this is already closed and we don't need to discuss it.

(CLOSED maybe) Issue2: Redirect if some condition is true (e.g. Login Status)

Think about a /admin/ route... Put there a layout file, and do the isAdmin check here in the load function of the layout file, and redirect if not logged in to a login page.
Doing this is useful, because you dont have to do it in every route of the /admin/ folder
/admin/users.svelte
/admin/news.svelte
/admin/groups.svelte
and so on....

In that case we can use __layout.svelte or __layout.reset.svelte.
And load function is working properly at least in my environment.
(I can redirect wherever I want.)

So I think this is also not issue. (Is this correct understanding??)

(???) Issue3: Documentation

According Issue1, __error.svelte can not use redirect.
But actually, the document doesn't mention that.
So maybe more people will have the same confusion in the future.
We should update the document about it IMO but what do you think?

https://kit.svelte.dev/docs#layouts-error-pages
https://kit.svelte.dev/docs#loading-output-redirect

@Rich-Harris
Copy link
Member

The semantics get awfully fuzzy if you try to redirect while rendering an error page. What if an error happens while rendering the page you were redirected to — do we start again, re-executing the root __layout and following any redirects encountered? By what mechanism do we prevent infinite loops? It's something that sounds obvious (I asked for a redirect, and I want a redirect dammit!) but as soon as you get into the implementation details it turns out to be fraught.

In fact I would say that in general, interfering with errors is dangerous — if something goes wrong, it's usually better to just communicate it than to try and course-correct, since the number of ways in which things can fail usually exceeds our ability to plan for them. Luckily though, there is a very easy way to get the behaviour you want in the 404 case — just add a catch-all rest route (and I would typically do the redirecting there rather than in the __layout).

We probably do need better error messages here — if a redirect is encountered while rendering an error page (whether in a __layout or an __error component) we shouldn't just silently ignore it.

@Rich-Harris Rich-Harris added this to the whenever milestone May 16, 2022
Rich-Harris added a commit that referenced this issue May 18, 2022
…ng error pages (#4945)

* document that status, error, redirect, cache are ignored when rendering error pages (#1575)

* move note

* move it again

* Update documentation/docs/04-loading.md

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@Rich-Harris
Copy link
Member

Going to close this as it's been obsoleted by the changes in #5748 — it's no longer possible for an error page to have a load function, meaning this situation can never arise (and errors need to be dealt with more explicitly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants