Introduce a feature for bottom-up page-to-layout communication #5951
Comments
|
The discussion at #5748 (comment) suggested using stores for this purpose. The top-level layout puts a store, let's say it's called const { meta } = data;
$meta.title = 'New title for this page';That should fire the subscription in the top-level layout, which would then update the title with the new value. |
|
I don't think we will reintroduce something else, the "this pops up in my data" is negligible to me - you don't have to use it in your component. The type story is worse for now, but we will implement some level of type safety again for this. |
|
@rmunn That's an interesting idea, but I tried it and it doesn't work during SSR which is a total deal breaker, as you would obviously need meta information like
Unfortuantely that's precisely what doesn't happen (during SSR), for some reason. |
@dummdidumm As I said, that one is the most minor issue, but I thought it's worth mentioning as it could be indicative of a larger conceptual problem. The type-safety is non-existent, and I'm not sure exactly why you're opposed to introducing a new distinct construct for this, but even if you don't want to do that, at the very least I would say just make using stores possible here. As soon as I read @rmunn's comment I thought yeah that's probably the perfect solution for this, but it turns out it doesn't work during SSR and that's a deal breaker, I would appreciate it if you explain why. To be more specific, when I set a store value inside a page's |
|
The type safety you had before was illusory — nothing was forcing your page Stores won't work for this in SSR because you're not setting the value until the leaf is rendered, by which point the layout... has already been rendered. |
|
@Rich-Harris So then what approach would you take for implementing things like this currently? And it's not just meta information, you might have a This gets worse if you need, for example, default values for these settings. All of this renders |
A derived store in the root where you read the props you need from |
|
@abdo643-HULK That still has all the downsides and limitations of |
|
Something like I hope this is reconsidered. SvelteKit doesn't currently have a satisfactory answer to this need. |
|
Because it is a derived store you can create your own type for ts. And you also said that things like meta aren't concerns of the |
@abdo643-HULK You can create your own TS type already, and then just convert
Perhaps that wasn't the best of examples, fair enough, but I also provided others such as |
|
It is also awkward to have to create a separate I must say I find it deeply frustrating to have to argue so elaborately for something that I feel should be obvious to anyone after minimal examination. |
|
You think you "elaborately" argue, I think you are being passive-aggressive. What's left is the TypeScript-story, which I agree needs to get better. |
Yes but it's obviously exacerbated by virtue of the fact that now you need to have a separate file for the
No, it's far deeper than that, even if the typing is improved, this will still not be a perfectly suitable solution for this kind of scenario. For instance, providing default values, as I mentioned above, would be non-obvious and awkward in this approach. And if you have multiple levels of layouts, the merging behavior won't work as expected, since a shallow merge is performed on the object. Consider the following (rather simple) example:
<script>
import { page } from '$app/stores';
</script>
<Header type={$page.data.layout.headerType} />
<!-- As a side note, this is more or less how you would have to provide default values, and it's not ideal since the default values are scattered around the entire file and you would potentially have to repeat them in multiple places or otherwise resort to more awkward workarounds -->
{#if $page.data.layout.showFooter ?? true}
<Footer />
{/if}
export function load(){
return {
layout: {
headerType: 'secondary'
}
};
}
export function load(){
return {
layout: {
showFooter: false,
}
};
}The resulting
export async function load({ parent }){
const { layout } = await parent();
return {
layout: {
...layout
showFooter: false,
}
};
}But now these two load functions are run serially as opposed to concurrently and that would have a performance cost if they're sending fetch requests and whatnot. I don't really need them to run serially, this is just what I have to do in this particular case because there's no other way to achieve what is needed. The same problem more or less existed for |
|
Maybe route specific hooks might partially address the problem (#5763), like |
|
#6017 will mostly fix this issue, allowing Automatic types for |
|
One thought that occurs to me: in the same way we used to have // src/app.d.ts
declare namespace App {
interface PageData {
title: string;
}
}// $types.d.ts
export type PageLoad = Kit.PageLoad<Params, InputData, App.PageData & Record<string, any>>;import type { PageLoad } from './$types';
export const load: PageLoad = () => ({
title: `if this is missing, it'll cause red squigglies`,
other: {
stuff: 'yay'
}
});// types/ambient.d.ts inside Kit
export interface Page<Params extends Record<string, string> = Record<string, string>> {
url: URL;
params: Params;
routeId: string | null;
status: number;
error: HttpError | Error | null;
- data: Record<string, any>;
+ data: App.PageData & Record<string, any>;
}A fancier version would allow a |
|
I'd make one change: |
|
It wouldn't apply to all |
I'm saying that the object shouldn't be "A page's load should return all of the required data", because the number of cases in which all of my pages share a set of data that they should all return 100% of the time is 0. What actually does happen is that all of my pages share a set of data that they might or might not return, and what's important is that if they do return it, that it be under the correct key and have the correct type. (It would also allow us to apply it to layout |
That's not necessarily true, if you return page metadata like title, description, and so on as part of the data, then every page would return those values. |
|
Right — the goal here is to be able to guarantee that you can do this sort of thing: <svelte:head>
<title>{$page.data.title}<title>
</svelte:head>For that to work, every It sounds like you want an |
That's true -- you could make the properties optional... would that allow us to use this for layout |
|
Or |
|
This just isn't true.
|
|
What if different routes have different shared page data by the way? This is rather common, I believe. But this solution (as is right now) would fail to account for those scenarios. Imagine you have |
This is exactly why I advocate for |
But that doesn't seem like the optimal solution, ideally we should have "route-scoped" shared data, if that makes sense, instead of an app-wide one defined in |
In an ideal world I'd agree with you, but we have to assign |
Well why not though? Couldn't SvelteKit utilize the same trick used for Not doing this will result in a sub-par type-safety experience however much you tweak it. |
Because |
But a layout (whether it's the default layout or a nested or reset one) could return it on behalf of its section of pages, right? It doesn't make sense for |
|
These problems are the reason why I've been trying to make the point that bottom-up data flow between pages and layouts is best handled by some new construct/feature, as opposed to page data, which is more suitable for top-down stuff, and becomes increasingly awkward when you try to use it for these types of scenarios. This also still fails to take into account the problem of default values, there's no obvious way to set default values for these shared page data properties. |
Yep, that's what I meant by this:
The discussion won't get far without a concrete proposal.
Making declare namespace App {
interface PageData {
foo?: string;
}
}We could make it so that layout |
That's an acceptable compromise for me. It covers the |
|
Just trying to parse through this issue, unless there's a PR imminently in the works for the lack of typing on FWIW love the proposal of adding a |
|
It seems to me that what we're trying to achieve here, fundamentally, is "set layout props", in a sense.
<script lang="ts">
export let showFooter: boolean = true;
</script>
export const load: PageLoad = ({ setLayoutProps }) => {
setLayoutProps({
showFooter: true,
});
};This is just a raw idea, so it could be refined. @Rich-Harris Wouldn't something like this be feasible? Thank you. |
|
I´m very unsure if this is even possible but i´l try to explain my idea: The types pretty much travel down the tree including every layout until we reach the page we are currently working in. // .svelte-kit/types/src/routes/$types.d.ts
export type LayoutData = { session: { user: { id: string } } }
// .svelte-kit/types/src/routes/profile/$types.d.ts
import type { LayoutData as ParentData } from '../$types';
export type LayoutData = { random: string } & ParentData;
// .svelte-kit/types/src/routes/profile/settings/$types.d.ts
import type { LayoutData as ParentData } from '../$types';
export type PageData = { value: number } & ParentDatathe result should be a merged type that includes all parent data types. Should also be possible with named layouts. The problem would about how to actually use this type. One way could be importing the src/routes/profile/settings/+page.svelte <script>
import { page } from './$stores';
// typed as string
$page.session.user.id;
// typed as string
$page.random;
// typed as number
$page.number;
</script>Edit: |
|
The problem with |
|
I´ve updated my comment.
Just replacing Edit: // vite.config.js
/** @type {import('vite').UserConfig} */
const config = {
plugins: [sveltekit()],
resolve: {
alias: {
'./$stores': '$app/stores',
},
},
}; |
|
Regarding the default value issue that a few people raised here, can't you do something like this?
<script lang="ts">
$: ({
showFooter = true,
} = $page.data.layout);
</script> |
Describe the problem
Prior to the recent API overhaul (#5748), I used to have a
Meta.sveltecomponent that was rendered by my root layout. This component would read from$page.stuff— which I had typed inapp.d.ts— and set various page-level head tags such as<title>,<meta name="description">and so on based on thosestuffvalues. Every page would then have to provide this information in itsloadfunction, like so:My
app.d.tsfile looked something like:And the
<Meta>component:Now, ever since the API has been re-designed and the
stuffthing has been removed, I've been struggling to re-create this properly.Asking questions on the Discord server and reading this part of the migration guide lead me to think that returning this information as part of the data returned by each load function is apparently the way things like this are meant to be handled now. But I find that this is an almost unacceptable alternative, for two major reasons + a more minor one:
<Meta>componentMeta.svelte:+page.sveltefile included as part of itsdataprop, which is weird since it's actually none of its business.metaisn't really something that this page is concerned with, so this is slightly awkward.This (particularly the lack of type-safety) clearly yields a drastically poorer developer experience compared to the previous API. I sense that not enough thought has been put into this particular requirement when re-designing the API, namely bottom-up communication between pages and layouts. Page data isn't really fit for this use case at all.
Describe the proposed solution
I don't have a clear-cut proposal here, the general idea is that perhaps you could consider introducing a new construct designed specifically for bottom-up page-to-layout communication, in order to properly accommodate use cases like this, which are by no means uncommon or unusual, or niche. It actually gets more complex in a real-world app where there are typically multiple levels of layouts, and each of them receives a different set of configurations/settings; which is why I believe this is something that deserves a dedicated feature, so to speak.
Importance
crucial
Additional Information
Please don't just hand wave this away, thanks. I know it's annoying that literally dozens of issues get created on this repo every single day and naturally it's almost impossible to give each and every single one of them a lot of attention, but this one is a real problem, I think, and one that is in need of a response from your side.
And tell me if I'm missing something, because I certainly raised an eyebrow while trying to do this but failing to find a satisfying solution, so it's possible that I'm unaware of something here.
The text was updated successfully, but these errors were encountered: