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

Add layout endpoints #4274

Closed
Rich-Harris opened this issue Mar 8, 2022 · 31 comments
Closed

Add layout endpoints #4274

Rich-Harris opened this issue Mar 8, 2022 · 31 comments
Labels
feature / enhancement New feature or request
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

Page endpoints have been a popular addition to SvelteKit, despite a few lingering bugs. Since before they landed, people have (quite reasonably) been asking why we don't have layout endpoints.

The biggest reason, frivolous as it might sound, is naming. 'Layout' is very much a UI concept, and so a __layout.js file that is solely concerned with loading data feels a bit weird.

Describe the proposed solution

As part of a wider overhaul of routing (follow-up issue to come, which will address pathless routes and granular layout resets) I propose changing __layout.svelte to __section.svelte and adding a corresponding __section.js (or .ts, etc) endpoint. 'Section' feels like a sufficiently agnostic word — it leans towards UI terminology, but it doesn't feel weird to talk about a 'section endpoint', as in 'the endpoint for the blog section' or the admin section or whatever.

Just as page endpoints expose their data as a virtual child __data.json route, section data will need to be exposed, perhaps by appending __section.json (i.e. src/routes/foo/__section.js is exposed as /foo/__section.json).

One major difference between section endpoints and page endpoints: since sections don't 'own' a specific URL, it doesn't make sense to be able to POST to them etc. I'd therefore argue that non-get handlers should be forbidden in section endpoints.

Alternatives considered

Other frameworks like Nuxt and Remix think about nested routes differently to how SvelteKit does it. Consider a route like /settings/notifications/email where email is nested within the notifications section, which is nested inside the settings section.

In SvelteKit the filesystem would look like this:

routes/
├── settings/
│   ├── billing/
│   ├── └── [files]
│   ├── notifications/
│   ├── ├── desktop.{js,svelte}
│   ├── ├── email.{js,svelte}
│   ├── └── __section.{js,svelte}
│   └── __section.{js,svelte}

Instead of having something like __section, Nuxt and Remix use a filename with the same name as the directory:

routes/
├── settings/
│   ├── billing/
│   ├── └── [files]
│   ├── notifications/
│   ├── ├── desktop.js (or desktop.vue, in Nuxt's case)
│   ├── └── email.js
│   ├── billing.js
│   └── notifications.js
├── settings.js

(In both cases, /settings and /settings/notifications routes can be added with index files.)

I don't think one of these approaches is unambiguously better than the other, but we can enumerate some pros and cons of the SvelteKit approach. Pros:

  • Things are appropriately colocated — all the notifications-related code lives together, rather than being split between notifications/ and notifications.{js,svelte}
  • Renaming 'settings' to 'preferences' happens in one place, not two
  • If you expand a folder like settings in your editor, the breadth of the folder is capped at n+2 rather than 3n where n is the number of nested sections (i.e. billing/, notifications/, __section.js and __section.svelte rather than billing/, notifications/, billing.js, billing.svelte, notifications.js and notifications.svelte)
  • It's arguably more explicit
  • If you needed /settings to have a completely different layout than /settings/* for some reason, it's easy — you can have routes/settings.{js,svelte}. If that file is instead used for layout, /settings/index.{js,svelte} must inherit from it unless you add a new convention for exempting it, like Remix's routes/settings.index.js (I've also seen routes/settings+index.js), which feels less than 100% intuitive to me

Cons:

  • The Nuxt/Remix convention is arguably more aesthetically pleasing than __section. Though as an aside, we're using the __ prefix convention anyway for __error.svelte, and we'll likely add directory-scoped __middleware.js at some point
  • settings.js is a more descriptive basename than __section.js
  • settings.js and settings/notifications.js lend themselves more easily to endpoint URLs (though we would need a way to disambiguate between settings.js and settings/index.js endpoint URLs in any case)
  • We're going against the grain. (Same as it ever was.)

In summary, I lean towards the __section proposal, but I would be curious to hear any arguments in favour of (or against) a more Nuxty/Remixy approach that I haven't considered here.

Importance

would make my life easier

Additional Information

No response

@vwkd
Copy link

vwkd commented Mar 8, 2022

+1 on the name change as layouts can be more useful than only for UI.

The name „section“ is a bit confusing to me as it doesn’t imply that it wraps the page, rather even the opposite. Some alternatives could be „template“, „extension“, „frame“, etc.

@Rich-Harris
Copy link
Member Author

You could argue that 'section' is topologically ambiguous, since it makes sense to talk about an 'outer section' that surrounds an 'inner section' (e.g. blocks of seats in an auditorium). But even without that, you could think of 'section' as representing the subtree rather than the wrapper — e.g. the 'blog section' comprises /blog/* as well as /blog

@kelvindecosta
Copy link

How about __group?

@rmunn
Copy link
Contributor

rmunn commented Mar 9, 2022

On forbidding post and other non-get methods in section endpoints, what is the expected behavior if a POST is sent to an endpoint that the section handles?

Looking at a concrete example will help me explain what I mean.

routes/
├── blog/
│   └── __section.{js,svelte}
│   └── index.{js,svelte}
│   └── [postId].{js,svelte}

In blog/__section.js, there is a get handler that sets up some server-side things common to the whole blog section, e.g. looking up the authenticatedUser object from event.locals (which was set up by a handle hook) and setting a permissions object in locals so that other handlers can just check the permissions object.

Now, that works fine for GET /blog/3. But what about POST /blog to create a new blog post? That's something that only certain users have permission to do. What happens when a POST is send to routes/blog/index.js? Specifically, what (if anything) is run in __section.js? I see two possibilities:

  1. __section.js's get handler is run for every request to /blog/whatever, including POST /blog.
  2. __section.js runs the same handlers as the handlers in the endpoint that's ultimately being called.

In case 1, the get handler for __section.js would run, then the post handler for index.js.
In case 2, the post handler for __section.js would run, then the post handler for index.js.

My preference is for case 2, because I could easily see it being very confusing to someone why a get method is being run for a POST request. Plus, not all section methods would be suitable for all use cases: sometimes the get handler in __section.js should NOT run for a PATCH request down the tree. If so, then omitting a patch handler in __seciton.js would get the desired behavior.

Thoughts?

@Rich-Harris
Copy link
Member Author

Good question @rmunn, I should have elaborated on that.

The way that POST /blog works today if you have a blog/index.js page endpoint is that the post handler runs, then (as long as it didn't redirect or throw an error) the get handler runs, so that /blog is rendered with data from both handlers. We can't not run the get, because then the page will have data missing. (Obviously if the post succeeds, it will likely redirect to the new page, short-circuiting this process, but if it returns validation errors then you need to render them and whatever other data the page depends on.)

The analogous behaviour with section endpoints would be to first run the post from blog/index.js, then run get from __section.js, then get from blog/__section.js, then get from blog/index.js. It wouldn't make sense to run a post inside the root __section.js for any POST request anywhere in the app, when the job of __section.js is to supply data to __section.svelte. We do always need to run the get handler in a __section.js.

Of course, this means you can't use the blog/__section.js to do auth stuff. Which makes sense in my mind: responsibility for that belongs to handle, not the request handler itself. Right now handle obviously runs for every request, not just the blog scope — I'd like us to have a scoped middleware solution, but in the meantime you could put the scoping logic inside handle.

@rmunn
Copy link
Contributor

rmunn commented Mar 9, 2022

Ah. With that understanding in mind, then I see why it makes sense not to allow post and other methods in section endpoints, and I agree. And yes, middleware will be the answer to the use cases I was envisioning.

@benmccann benmccann added this to the 1.0 milestone Mar 10, 2022
@stephane-vanraes
Copy link
Contributor

Would this effectively enable what is described in #2847 ?

@Rich-Harris
Copy link
Member Author

@stephane-vanraes it would, yes. There is a wrinkle we need to consider though — if you have files like this...

routes/
├ foo/
│ ├ bar/
│ │ ├ baz/
│ │ │ ├ __section.svelte
│ │ │ └ index.svelte
│ │ ├ __section.svelte
│ │ └ index.svelte
│ ├ __section.svelte
│ └ index.svelte
├ __section.svelte
└ index.svelte

...and you visit /foo/bar/baz, there are two possible things we could do: run the endpoints serially, or concurrently.

If we run things serially, we introduce a waterfall (which is already present with load functions, because they inherit stuff, though you can work around it by returning props: Promise<Props> rather than awaiting the props). If we run them concurrently, foo/bar/__section.js and foo/bar/baz/__section.js need to account for the possibility that __section.js or foo/__section.js return a non-2xx response.

Personally I'm more inclined towards serial behaviour despite the waterfall, because otherwise it defeats the object of having a guard. We can at least coalesce endpoint responses during client-side navigation so that the waterfall only happens on the server, where the damage is minimised since server-to-server communication is less costly (particularly if everything is running in the same region/data centre). We could also do the same thing we do with load functions — allow body to be a Promise if you're okay with a generic 500 if it rejects.

@rmunn
Copy link
Contributor

rmunn commented Mar 14, 2022

Would it be an error for a __section.js file to exist but not export a get function? Or would that section be treated as having a no-op get function, that does not modify the request in any way?

I could see arguments for either one, but I think I'd lean towards a warning, but not an error, if a __section.js does not export a get function. Because there might be times when you want to comment out the get function during development to test something, and it would be slightly easier to comment out the whole function than to replace it with a no-op. But maybe we want to make it an error, because the more common mistake by newbies will be to forget to write export in front of async function get and we want to push them into the Pit of Success.

This thought was triggered by seeing #4316, where a post but no get in a page endpoint is causing a 404. That made me wonder whether that was intended, and also whether section endpoints would have the same requirements, or whether get-less endpoints are allowed for pages but not sections.

@stephane-vanraes
Copy link
Contributor

'Layout' is very much a UI concept, and so a __layout.js file that is solely concerned with loading data feels a bit weird.

I feel this is a bad reason for renaming, if anything section feels even weirder. At least with layout you can argue that is naming consistency between different features, especially considering that most developers will likely encounter this in the context of layouts and not of standalone pure data endpoints.

Those are more of an advanced topic, at which time you got used to the naming. That it is not "100% semantically correct" can be easily handwaved away at that point and I doubt many users would make a big deal out of it.

Saying that layout does not refer to data is ambiguous on it's own, you could very well argue that you are 'laying out data'1

Furthermore looking at #4275 I believe that named layouts are easier to understand than named sections,

Finaly with sections (especially named sections) I would expect to be able to designated different 'sections' in a page (like header, footer, ...) similar to how slots work.

Footnotes

  1. actual argument left to the reader as an exercise

@brittneypostma
Copy link
Contributor

I very much agree that section feels stranger than layout, also layout feels more familiar back to Sapper days. A Layout wraps a page and can have data with it. A section feels like you should have multiple per page.

I like template better than section if renamed, but again it feels almost like a breaking change just for the sake of a convention.

@Rich-Harris
Copy link
Member Author

Yeah, we had a discussion about this in the maintainers' chat and concluded that keeping __layout is probably better on balance. A key realisation for me was that you wouldn't want to get the data for a layout/section individually, i.e. there'd be no __layout.json or __section.json — instead, a page's __data.json should include the layout data as well as the leaf data. Otherwise you'll find yourself making unnecessary round trips.

@benmccann benmccann added the feature / enhancement New feature or request label Mar 17, 2022
@benmccann benmccann changed the title Rename __layout to __section and add section endpoints Add section endpoints Mar 17, 2022
@benmccann benmccann changed the title Add section endpoints Add layout endpoints Mar 17, 2022
@coryvirok
Copy link
Contributor

Reading through all of these comments makes me wonder what the proposed difference would be between an endpoint "layout" or "section" vs endpoint middleware. It seems like the primary ask of this thread is to have endpoint wrapper logic, which seems like server middleware to me.

My primary use-case for this would be auth guards. I currently safelist certain paths and check for them in handle(). I'd much rather have something like routes/api/__middleware.ts that executes it's own handle() like function for everything in its subtree.

@lukaszpolowczyk
Copy link
Contributor

I'll just be brief:
There should be a reset or something like "named layout" for middleware too.
Does anyone agree?

@madeleineostoja
Copy link

Just wanted to quickly voice that there is a good use-case for 'layout' endpoints rather than middleware in fetching data to be shared across routes, via stuff or otherwise. For example global header, footer, etc data in a CMS-backed SSG. Doing that in middleware would be unintuitive and clunky imo.

I'm a bit fuzzy on the proposal to include this shared data in every route's __data.json, in that case how would you share data downwards to individual components (vs routes) in the tree below a __layout? Doesn't that kind of defeat (part of) the purpose of a shared endpoint, since you'd have to grab the data in load on every route you needed it, vs via stuff or context set in __layout?

@richarddavenport
Copy link
Contributor

We rely on stuff quite a bit. We have data fetchers in our layout's because that is in fact one of the duties of a __layout.svelte page, namely breadcrumbs, headers, footers, sidenav, etc... It doesn't make sense to repeat those calls over and over in each page that inherits from the layout.

@avarun42
Copy link
Contributor

avarun42 commented Jun 8, 2022

Echoing the request for middleware scoped to certain directories and route hierarchies — it would be really helpful for handling authentication and redirects for different sections of an app that each have different rules and rely on different calls to external APIs.

@bjmckenz
Copy link

+1 to the need for something like this to implement an auth gate. That said, doing it in __layout doesn't feel sveltey to me because I interpret "layout" as "layout" instead of "make decisions about redirection". (Disclaimer: IANASE [I am not a Svelte expert])

I've been noodling over how to do this on my project. It hasn't been obvious. At the end of the day, I only want to write code in one place -- I'm not happy replicating auth checks and redirects everywhere. I've experimented doing it in __layout, and it's not been great [my design, that is]. One of the problems is that I want to tag pages as (not) needing auth, and I can't see a great way to avoid coupling the yes/no into the layout (as opposed to a property of the route. Middleware kinda implies a a way to make these decision.

@Spenhouet
Copy link

Not wanting to hijack this issue but making sure I don't create a duplicate.

I have a search bar component ($lib/components/search.svelte), now this search bar should show suggestions based on what was typed in. I expected to be able to provide these suggestions via a page endpoint $lib/components/search.js but this does not seem to be executed and provided to search.svelte.

Would this type of component endpoint also be addressed with this issue? Or should this already work and I just fail at implementing it? Or is it an independent feature request?

@jason0x43
Copy link

@Spenhouet That should already work; this issue is specifically about endpoints for __layout.svelte routes, which don't currently work. Make sure your endpoint exports a get request handler function (not load, as I tried the first time I implemented a page endpoint 🙂).

@Spenhouet
Copy link

@jason0x43 Sadly it's not working for me. I have other endpoints running which use the typical routing pages but for components it's not working for me. Just to make this clear, with component I mean that it is imported somewhere and is not loaded via sveltes routing.
In my case I'm importing it in a Header component like this:

<script>
  import Search from $lib/components/search.svelte';
</script>
<Search />

Which itself is imported and used in __layout.svelte.
Before I create a separate bug report and before I chat here about it I opened a thread on the Svelte Discord: https://discord.com/channels/457912077277855764/939868205869072444/987845135129796659

@MadeByMike
Copy link

Seems to me that another possible solution could be to expand the ResolveOptions and then you could put data in a hook.

Something like:

export const handle = async ({ event, resolve }) => { 
  const response = await resolve(event, { params: { ...CUSTOMDATA });
  return response;
}

@jason0x43
Copy link

@Spenhouet Ah, sorry, I missed the fact that you were talking about non-route components. Never mind me... 🙂

@jason0x43
Copy link

Just out of curiosity, how would a page endpoint work in the case of a non-page component? Like, what would be its expected behavior? A page endpoint is called when a route is accessed to supply data to the associated page, but that doesn't seem applicable to a non-page component.

@Spenhouet
Copy link

Just out of curiosity, how would a page endpoint work in the case of a non-page component? Like, what would be its expected behavior? A page endpoint is called when a route is accessed to supply data to the associated page, but that doesn't seem applicable to a non-page component.

Like in my case, I'm adding a search bar to the header. The search bar is basically just a combobox showing data filtered based on what one entered into the search bar.
The underlying data needs to come from somewhere.
Writing an endpoint for that seems straight forward to me. In my case search.js which is supposed to load the data for the search bar component.
One can use this to load data for any form of sub component.

So would you consider this to be an extension of this feature request or a separate "component endpoints" feature request?

@jason0x43
Copy link

So would you consider this to be an extension of this feature request or a separate "component endpoints" feature request?

For the search bar, I would think a standard endpoint, like /routes/api/search.js, would be the way to go. A page endpoint provides data to an associated component when that page is loaded, but a search bar doesn't seem like it would need data at load time (unless the search is entirely client side). Rather, it would need an endpoint to send queries to.

@Spenhouet
Copy link

@jason0x43 Did I mention that the page is static? So yes, it's purely client side.

@jason0x43
Copy link

That does seem like a reasonable use case for a non-page component endpoint.

@madeleineostoja
Copy link

I feel like general non-route endpoints for any old component is a very slippery slope, and could add a lot of complexity to Sveltekit's data layer without a much gain over just using a standalone endpoint and fetching it in the component.

If you needed server fetched data for a component, you could just get it in a __layout endpoint (the focus of this issue) and throw it in $page.stuff

@homerjam
Copy link

+1 for __middleware.ts + __layout.svelte

I think removing the ambiguity about what goes where is helpful (it's always bugged me that I have arbitrary setup code in layout which should be strictly for the template IMHO).

@Rich-Harris
Copy link
Member Author

Closed via #5778

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

No branches or pull requests