-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
+error.svelte
needs to have a load
function
#6106
Comments
Same here, I was just about to migrate to the new version and couldn't get this part working. I was previously returning |
You can access Given how much of a footgun |
Could you provide an example? I think that would be extremely awkward, especially if you have a bunch of different layout settings. With return {
meta: {
title: 'Error occured',
},
header: {
layout: 'wide',
},
footer: {
hide: true,
}
}; While on the other hand trying to achieve the same thing with
<svelte:head>
<title>{$page.error ? 'Error occurred' : $page.data.meta.title}</title>
<meta name="description" content="{$page.error ? 'whatever' : $page.data.meta.title}" />
</svelte:head>
{#if !$page.error && !$page.data.footer.hide}
<Footer />
{/if} Having special cases defined everywhere like this is the definition of ugly code (and could quickly turn into a maintenance nightmare) I think you would agree.
Don't you think just mentioning that users need to be mindful about what they put inside an error page's load function in the documentation would suffice? There's a variety of things one might want to do in a load function that doesn't involve potential screw-ups, and I provided the prime example of that. I would argue the framework shouldn't take this capability away from the developer entirely just because it could cause problems if they do dumb things. |
The <title>{$page.error ? 'Error occurred' : $page.data.meta.title}</title> ...seems preferable to having to have this for every single error boundary: export function load() {
return {
meta: {
title: 'Error occurred'
}
};
} It strikes me as unlikely that you'd often need to vary the messages on a per-error-boundary rather than on a per-status or per-error basis, in which case having the logic centralised in the layout is better. But if you really do need to vary it based on the route, you have access to I'd argue that the current approach makes common approaches easy and less-common approaches possible. What you're arguing for makes the less-common approaches slightly easier, but at the cost of a more complex mental model. |
@Rich-Harris I don't find the justification convincing. I would suggest at least making the |
@Rich-Harris I must say I'm surprised to hear that, I was confident you'd share my sentiment.
<svelte:head>
<title>{($page.error ? `${$page.status} error occurred` : $page.data.meta.title) + $page.error ? titleSuffixes.short ? : $page.data.meta.titleSuffix}</title>
<meta name="description" content={$page.error ? 'whatever' : $page.data.meta.description} />
</svelte:head>
<Header />
{#if !$page.error && !$page.data.footer.hide}
<Footer />
{/if}
<div class="header {$page.error ? ($page.status == 404 ? 'wide' : 'normal') : $page.data.meta.header.layout}">
...
</div> etc. ...to be cleaner/more scannable/better organized, than this:
<svelte:head>
<title>{$page.data.meta.title + $page.data.meta.titleSuffix}</title>
<meta name="description" content={$page.data.meta.description} />
</svelte:head>
<Header />
{#if !$page.data.footer.hide}
<Footer />
{/if}
<div class="header {$page.data.meta.header.layout}">
...
</div>
return {
meta: {
title: `${status} occured`,
},
header: {
layout: status == 404 ? 'wide' : 'normal',
},
footer: {
hide: true,
}
}; Again, I feel like it shouldn't really be controversial. Isn't it clear which approach is saner? To have to declare a special case (through constant use of ternary operators and so on) in every single place, just to accommodate the lack of the ability to return page data from an error page, frankly feels preposterous to me. But definitely feel free to correct me if I'm missing something. |
I am not very sure if I comprehend everything correctly, but I think this is actually another problem. Please correct me if I misunderstood. I suppose the The benefit of this is On first glance, However, load in layout and pages could do much more stuff (fetching data, passing data to child etc), which I think is not the purpose of Thus I propose maybe we can add an object into
throw error(500,"Some server error",
{
meta: {
title: `500 occured`, // may need hardcode
},
header: {
layout: status == 404 ? 'wide' : 'normal',
},
footer: {
hide: true,
}
}
)
<svelte:head>
<title>{$page.data.meta.title + $page.data.meta.titleSuffix}</title>
<meta name="description" content={$page.data.meta.description} />
</svelte:head>
<Header />
{#if !$page.data.footer.hide}
<Footer />
{/if}
<div class="header {$page.data.meta.header.layout}">
...
</div> |
Well, I'd write the <svelte:head>
- <title>{($page.error ? `${$page.status} error occurred` : $page.data.meta.title) + $page.error ? titleSuffixes.short ? : $page.data.meta.titleSuffix}</title>
+ <title>{($page.error ? `${$page.status} error occurred${titleSuffixes.short}` : $page.data.meta.title + $page.data.meta.titleSuffix}</title>
<meta name="description" content={$page.error ? 'whatever' : $page.data.meta.description} />
</svelte:head> (I'm also not sure where Is the first example preferable to the second? No. Is it preferable to the second if you have multiple error boundaries? Yes, absolutely. The code in the first example isn't going to make anyone swoon, but it's fine, and if it really starts to get convoluted then you can just do I'm not trying to be awkward here. The ability to run an asynchronous There's no objectively right answer to any of those questions, but because we've created room for that ambiguity, and make the mental model that much harder to grasp, we have to document all this behaviour, and app developers need to read that documentation and organise their Successfully handling error states requires the elimination of ambiguity. (It's not enough to say 'you can't Against all that, 'I don't want to write a ternary in my I think @harrylaulau's proposal is intriguing. It allows data to be returned without any of the footguns that come with an error |
Another alternative could be to allow |
That would be less flexible than adding an argument to Though, one obvious-once-you-notice it thing: if there's an unexpected failure (like a network error), then there's no opportunity to return data. |
@Rich-Harris Well, couldn't this be solved by implementing @ponderingexistence's suggestion and not allowing the
I don't think I agree there. If you have multiple error pages and you want them to share the same
(I laughed at this way harder than I should've, "swoon" lol)
Which is why I'm not fond of that approach, as you said, it doesn't make sense to specify that kind of data when throwing the error, that's the concern of the error page, not the concern of the page that throws the error, it simply doesn't belong there. More crucially, this fails to cover cases where a non-intentional 500 error is thrown (because of some unexpected failure in the execution of a load function, say); this renders it essentially unfeasible.
Assuming the following signature for the error load: // No promise (must be synchronous), no `parent` to await, etc.
type ErrorLoad = ({ status, url, setHeaders }) => Record<string, any>; Most of those concerns are or more less rectified, apart from the "the user can still Let me say that I I think I do understand your perspective here, to some degree at least, and I find a few of your points compelling, but it's ultimately a question of trade-off, and I believe that this is worth adding as I'm convinced the advantages trump the potential footguns you talked about, and many of those potential footguns can either be solved pretty simply or are just too unrealistic. And I've presented my reasoning. In my view you're trying to police everything a little too much here. There's a lot of ways developers can screw things up and not everything can be enforced through code (this is why documentation and common sense exist), I would argue it's just unnecessary policing, at the cost of losing some rather necessary functionality. Although if you ultimately decide against this, I will respect that, no hard feelings. But I'd ask you to not dismiss this out of hand. |
After the load refactor, I now have a problem with my error pages as well, specifically where I can't access any data from the layout. For example my error template used to look like this:
So the real work happened in the root layout's load function, which would put
So I am really wondering how you are supposed to use any data from the layout's load function in the error page now. I don't even need a whole load function, I just want to grab the |
That info should be available through |
Oh wow, thanks! That indeed works great. Makes me kinda wonder why we even have 2 ways to access data? ( |
Having to write |
The merged (Folks might have missed this change: we recently changed how The other reason to use |
This is very good info and resolved why I found this thread! Would be great to have that on https://kit.svelte.dev/docs/errors. |
My use case for having a load function in the error page is because the content of our 404 page is controlled by a CMS. The content itself is loaded and passed to the |
I ran into this issue as well. When using auth, I need to check the user's session on every page load so I return the session from load. If an error occurs, there will not be session data available to the Error page so the app acts as if the user is no longer logged in. I use $page.data in my root layout to show the user's profile picture, etc. and all of that disappears when the error page is shown. |
Describe the problem
Previously error pages could have
load
functions, just like regular pages, from #5748 onwards that's no longer possible, the rationale given was that an error page doesn't really need a load function and a load function could potentially screw things up further.But I think a very important and common scenario was overlooked: You might want to return data from the error page's load function that the layout will then consume, and just things of that nature in general.
So for instance:
error.ts
:This is actually very serious. I just realized there's no way of doing this right now, but it's needed in a lot of cases obviously. The current design doesn't provide any alternative.
Describe the proposed solution
Just allow error pages to have
load
functions like before please. You could then just simply point out in the docs that the load function of an error page must avoid doing anything that could potentially result in another error.Alternatives considered
No response
Importance
i cannot use SvelteKit without it
Additional Information
No response
The text was updated successfully, but these errors were encountered: