Open aradalvand opened 2 years ago
Same here, I was just about to migrate to the new version and couldn't get this part working. I was previously returning stuff
in my __error.svelte
's load function — which is now supposed to be part of the returned object directly.
A load function is necessary for error pages in a lot of cases, just add one of those red alert boxes in the docs warning people that they shouldn't do any I/O operations or whatnot in their error pages' load functions, but this should be added back.
You can access $page.status
and $page.error
. As long as the errors are sufficiently informative, I believe you can do anything you would have done with a load
, albeit with a bit more work.
Given how much of a footgun +error
load
can be, and how much implementation complexity it adds, I think this is the right trade-off. If the goal is to show a different layout in the case of an error, then +error@whatever.svelte
feels like a more promising alternative.
@Rich-Harris
I believe you can do anything you would have done with a load, albeit with a bit more work.
Could you provide an example? I think that would be extremely awkward, especially if you have a bunch of different layout settings. With load
you could simply do something like this:
return {
meta: {
title: 'Error occured',
},
header: {
layout: 'wide',
},
footer: {
hide: true,
}
};
While on the other hand trying to achieve the same thing with $page.error
entails having a bunch of ugly ternary operators and so on almost everywhere and scattering these configurations (which fundamentally belong to a single page, the error page) throughout different parts of the app:
+layout.svelte
:
<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. This will get exponentially worse if you need to have specific page titles, for example, for specific errors (404, 403, etc.). This is just so unwieldy and unsveltety :( But let me know if you have a different workaround in mind. Thank you.
Given how much of a footgun +error load can be, and how much implementation complexity it adds, I think this is the right trade-off.
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 +layout.svelte
example you gave is exactly what I had in mind. I just don't agree that it's a big problem — this...
<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 $page.params
and $page.routeId
.
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.
If soon something like App.PageData
is introduced (which you hinted at here) then we're effectively defining a contract that "every" page must satisfy (e.g. if, for example, you have a required title: string
property in your App.PageData
, as per your own example in the comment I linked to), and an error page, is, in fact, a page as well. But currently it isn't even "able to" conform to the contract, as it cannot have a load function, that's kind of absurd in my opinion.
I would suggest at least making the load
function of an error page more restrictive, like for example it cannot be async
(i.e. return a Promise
). Wouldn't that solve the issues you're concerned about? While also providing the ability to return data and such.
The +layout.svelte example you gave is exactly what I had in mind. I just don't agree that it's a big problem
@Rich-Harris I must say I'm surprised to hear that, I was confident you'd share my sentiment. That specific example you provided was a one-liner and as such doesn't really demonstrate the problem. Let's take a more complex example: Do you genuinely find this... (please take a moment to actually parse each expression in your head)
+layout.svelte
:
<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}
Header.svelte
:
<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:
+layout.svelte
:
<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}
Header.svelte
:
<div class="header {$page.data.meta.header.layout}">
...
</div>
+error.js
:
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 +error.js
is for the return of load of +error.svelte
, which then passes the returned value (meta header etc) to +layout.svelte
.
The benefit of this is +error.svelte
and other +page.svelte
sharing the same +layout.svelte
. What you need is really passing data from +error.svelte
to +layout.svelte
.
On first glance, +layout.svelte
gets the data from load of other pages / layout, which makes it obvious for error to have load and do the same thing.
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 +error.svelte
. In think the aim of error.svelte
is just to act as a simple error page which other pages / endpoint "throws error at it", such that it won't fetch, pass data etc.
Thus I propose maybe we can add an object into throw error(_status_,_errorMessage_,_object_)
, which would be added / replace key value pairs in $page.data
such that layout.svelte
can consume the same way as is while no complicated load for error.svelte
?
+page.server.js
some hardcode
throw error(500,"Some server error",
{
meta: {
title: `500 occured`, // may need hardcode
},
header: {
layout: status == 404 ? 'wide' : 'normal',
},
footer: {
hide: true,
}
}
)
+layout.svelte
same as above comment
<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}
Header.svelte:
same as above comment
<div class="header {$page.data.meta.header.layout}">
...
</div>
Well, I'd write the <title>
like this for one thing — I wouldn't have two ternaries when one will do:
<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 titleSuffixes.short
is expressed in the alternative code.)
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 <title>{getTitle($page)}</title>
.
I'm not trying to be awkward here. The ability to run an asynchronous load
function when trying to handle errors is a genuine source of complexity. If something goes wrong during that load
(which is likely! If the conditions were right for an error to occur in the layout/page load
functions, such as the user being offline, then there's a better-than-normal chance that they're right for the error load
to fail as well), then we have to start making decisions: do we try and walk our way back up the layout tree, trying parent error boundaries? (Depending on what those load
functions do, we could be waiting a while before we're able to actually render something!) Which error do we use — the original error, or the error that was thrown from the error load
? Do we bail altogether at that point and just display the root error page? What if we hit an error while running that load
?
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 load
functions around it. And that's before people start doing throw redirect(...)
, creating even more opportunity for cascading failures.
Successfully handling error states requires the elimination of ambiguity.
(It's not enough to say 'you can't fetch
stuff in an error load
function' — you need to enforce it, and that means complicating the API. Nor can you ban people from throw error
or throw redirect
in an error load
function — what are you going to do, throw a different error to tell them not to throw errors?)
Against all that, 'I don't want to write a ternary in my <title>
' just isn't a persuasive argument unfortunately.
I think @harrylaulau's proposal is intriguing. It allows data to be returned without any of the footguns that come with an error load
function. We'd miss out on some type safety — assuming we move forward with the App.PageData
proposal in #5951, the third argument would have to be Partial<App.PageData> & Record<string, any>
rather than App.PageData & Record<string, any>
because we wouldn't have the contextual knowledge of which fields had already been supplied by parent layouts — but only a bit. It feels weird to include that sort of data when throwing an error, but not outrageously so.
Another alternative could be to allow export const data = {..}
from an +error.js
That would be less flexible than adding an argument to error(...)
, as you wouldn't have the ability to e.g. vary the message based on the status.
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.
I'm not trying to be awkward here. The ability to run an asynchronous load function when trying to handle errors is a genuine source of complexity.
@Rich-Harris Well, couldn't this be solved by implementing @ponderingexistence's suggestion and not allowing the load
function to be asynchronous? I'm not arguing for an asynchronous load function in this case, I'd be totally fine with making it synchronous.
Is the first example preferable to the second? No. Is it preferable to the second if you have multiple error boundaries? Yes, absolutely.
I don't think I agree there. If you have multiple error pages and you want them to share the same data
and the logic for populating it, then you can simply extract that part into a single function and reuse it. Even then, that's a far less common scenario than simply wanting to return some data from your error page. I don't think this is a particularly compelling argument.
The code in the first example isn't going to make anyone swoon
(I laughed at this way harder than I should've, "swoon" lol)
I think @harrylaulau's proposal is intriguing.... It feels weird to include that sort of data when throwing an error, but not outrageously so.
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. Update: I just noticed you mentioned this yourself here.
If something goes wrong during that load (which is likely! If the conditions were right for an error to occur in the layout/page load functions, such as the user being offline, then there's a better-than-normal chance that they're right for the error load to fail as well), then we have to start making decisions: do we try and walk our way back up the layout tree, trying parent error boundaries? (Depending on what those load functions do, we could be waiting a while before we're able to actually render something!) Which error do we use — the original error, or the error that was thrown from the error load? Do we bail altogether at that point and just display the root error page? What if we hit an error while running that load?
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 throw redirect/error
" one, which in my opinion isn't a realistic enough scenario to center your design around. I can't think of any reasonable scenario where someone would non-jokingly do a redirect or throw another error in an error page's load function.
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. Thank you.
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:
<script context="module">
export function load({ stuff, error, status }) {
const updatedStuff = stuff;
updatedStuff.error = error;
updatedStuff.status = status;
return {
props: updatedStuff
};
}
</script>
<script>
import { page } from "$app/stores";
const campaignId = $page.params.campaignId;
export let fetchedCampaigns;
export let status;
export let error;
$: campaign = $fetchedCampaigns[campaignId];
</script>
<svelte:head>
<title>Error 404 | Characters | {campaign.name} | Critical Notes</title>
</svelte:head>
<h1>Error {status}</h1>
<p>{error.message}</p>
So the real work happened in the root layout's load function, which would put fetchedCampaigns
into stuff
, and the error page could grab it from stuff
and show a nice title. But after the refactor I added export let data
to +error.svelte
, and it's null
. I can't create +error.js
either, which could look like this:
export async function load({ parent }) {
return await parent();
}
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 data
that's already returned from the layout.
That info should be available through $page.data
. @Rich-Harris should we use the same logic of merging the outputs into export let data
and allow +error.svelte
to access that, too? Feels a little off if we don't introduce something like export const data = { ... }
at the same time for +error.js
honestly.
Oh wow, thanks! That indeed works great. Makes me kinda wonder why we even have 2 ways to access data? ($page.data
and export let data
)
Having to write $page.data
everywhere would be cumbersome. If you had to do prop drilling only with export let data
that would be cumbersome, too. Also, $page.data
contains the whole data of the current page while each layout only has the merged data from itself and its parents accessible through export let data
.
The merged data
is already available to the +error
component.
(Folks might have missed this change: we recently changed how export let data
works — instead of only containing the data that was returned from the adjacent load
function, it contains shallowly-merged data from all load
functions up to that point, so your +error.svelte
component can access fetchedCampaigns
from the root layout.)
The other reason to use export let data
rather than $page.data
everywhere is that it gives you a level of type safety that's impossible with a global data object.
(Folks might have missed this change: we recently changed how export let data works — instead of only containing the data that was returned from the adjacent load function, it contains shallowly-merged data from all load functions up to that point, so your +error.svelte component can access fetchedCampaigns from the root layout.)
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 error()
function but we lazy load the components needed for a page. In +page.ts
we load all components that are needed by the server data before starting rendering. But since +error.svelte
doesn't have a load function we don't have anywhere to await the loading of those components. Currently we (mis)use {#await}
for this but that causes a flash when the components are loaded.
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