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 support for per-directory +hooks.server.js route files #6731

Open
accuser opened this issue Sep 11, 2022 · 15 comments
Open

Add support for per-directory +hooks.server.js route files #6731

accuser opened this issue Sep 11, 2022 · 15 comments
Labels
feature / enhancement New feature or request
Milestone

Comments

@accuser
Copy link

accuser commented Sep 11, 2022

Describe the problem

The current hooks functionality is really useful, particularly when used with sequence() to create an event pipeline. However, I'm increasingly finding that I want to specialise hooks for a given request paths, which requires pathname checking. Whilst this works, it is brittle - changing the route folder requires changing the pathname check. It would be preferable to colocate specialised hooks with the routes they pertain to.

Describe the proposed solution

I would like to be able to add a +hooks.server.js (or .ts) route file to a route folder that would export server hooks specifically for that route and its children. The request event handler would call the exported handle function after the current hooks.server handle function had been called, effectively appending it to the global hook sequence.

Where a child route folder has parent route folders with hooks, these would be appended prior to any hooks specific to the child.

Alternatives considered

Prior to the introduction of generated types I had used an interceptor pattern to wrap Handle and / or RequestHandler, but this has become too unwieldy to type correctly, and has resulted in unnecessary boilerplate-like code.

Importance

would make my life easier

Additional Information

No response

@cdcarson
Copy link
Contributor

This might be a good way to handle the problem described in #6315

@dummdidumm dummdidumm added the feature / enhancement New feature or request label Sep 12, 2022
@Rich-Harris
Copy link
Member

It would be helpful to have some concrete use cases for this. We've discussed it a fair amount but so far determined that it wouldn't be a good addition, because it would add a lot more complexity.

For example, if you have a situation like this...

src/
├ routes/
│ ├ foo/
│ │ ├ bar/
│ │ │ ├ baz/
│ │ │ │ ├ +layout.js
│ │ │ │ │ +page.js
│ │ │ │ └ +page.svelte
│ │ │ └ +layout.js
│ │ └ +layout.js
│ └ +layout.js
└ hooks.server.js

...then there's some stuff you have to understand about what happens when, but it basically makes sense:

  • handle in src/hooks.server.js starts
    • load functions in the +layout.js and +page.js files run concurrently
    • once load functions finish, we render +page.svelte
  • handle finishes

If we now add a bunch of +hooks.server.js files...

src/
├ routes/
│ ├ foo/
│ │ ├ bar/
│ │ │ ├ baz/
+ │ │ │ ├ +hooks.server.js
│ │ │ │ ├ +layout.js
│ │ │ │ ├ +page.js
│ │ │ │ └ +page.svelte
+ │ │ ├ +hooks.server.js
│ │ │ └ +layout.js
+ │ ├ +hooks.server.js
│ │ └ +layout.js
+ ├ +hooks.server.js
│ └ +layout.js
└ hooks.server.js

...then the order gets somewhat more complex:

  • handle in src/hooks.server.js starts
    • handle in src/routes/+hooks.server.js starts
      • handle in src/routes/foo/+hooks.server.js starts
        • handle in src/routes/foo/bar/+hooks.server.js starts
          • handle in src/routes/foo/bar/baz/+hooks.server.js starts
            • load functions in the +layout.js and +page.js files run concurrently
            • once load functions finish, we render +page.svelte
          • handle in src/routes/foo/bar/baz/+hooks.server.js finishes
        • handle in src/routes/foo/bar/+hooks.server.js finishes
      • handle in src/routes/foo/+hooks.server.js finishes
    • handle in src/routes/+hooks.server.js finishes
  • handle in src/hooks.server.js finishes

I feel like the mixture of sequential and concurrent code would be a real source of confusion for people, particularly the fact that e.g. src/routes/foo/bar/+hooks.server.js runs before src/routes/+layout.js (and also after it, sort of). The waterfall is an opportunity for slowness to creep in. And the flip side of being able to scope handle code with more granularity is that it becomes harder to figure out where a particular thing is happening.

On top of that, while it makes sense to chain handle functions, it probably doesn't make sense to chain handleError. Meanwhile handleFetch could probably go either way. These sorts of ambiguities are best avoided unless they're absolutely necessary.

@Rich-Harris Rich-Harris added this to the post-1.0 milestone Sep 20, 2022
@accuser
Copy link
Author

accuser commented Sep 20, 2022

Thank you for taking time to consider this @Rich-Harris.

Authorisation is my primary use case for adding optional +hooks.server.js to routes. I want to check a user's authorised scopes before handling a request, and sometimes this needs to be nested. I was able to do this with a simple interceptor that wrapped the handler, but it wasn't easily portable to the newly typed interfaces. The checks can become boilerplate-like quite quickly, and so pulling them out to a hooks module would be useful.

I've just read the proposal to add guard functions to layouts #6912 and this is similar to what I am looking for, albeit extending the +layout.server.js module.

@accuser
Copy link
Author

accuser commented Sep 27, 2022

Reflecting on your response - and just thinking about handle - if the hooks in your example were necessary, they would reside with hooks.server.js right now, and would be called sequentially. It would be simpler from the framework's point of view, since they would be exported using sequence() or a single handle implementation. From a developer point of view, this would be more complex.

Also, +hooks.server.js proceeds +layout{.server}.js in lexicographical order, and so I don't think it would be too much of a stretch for developers to understand the order in which the modules are called (I don't think we should name future route files with this in mind, but it is useful in this instance.) While the hook will run before the layout and page load functions, the fact is that is can operate before, after, and around (i.e., before and after) the load. This can afford lifecycle functions to individual routes, and goes a long way to providing a middleware solution for SvelteKit.

The benefit of colocating hooks with the routes that they apply to reduces the overhead of guard functions to check if the hook applies to the request or not. I accept that it would introduce complexity for the framework as it would be necessary to create route specific sequences, but the benefit will be for the developer.

A lot could be achieved by in +layout{.server}.js but this is adding business logic to layout, even if the functions were imported from elsewhere. I do think that a route +hooks.server.js resolves this, and is obviously entirely optional for the developer.

Edit: I should add, I hadn't considered handleError and handleFetch in the original post, and agree that the utility of these is ambiguous right now.

@dslatkin
Copy link

I feel like this issue can almost be renamed to "Add support for route-level middleware".

To summarize the two big use cases:

  1. Enabling "route guards" or "protected routes"
  2. Empowering the SvelteKit ecosystem with pluggable route-level middleware
    • Right now only possible through brittle string matching in hooks.*.js

I'm not personally convinced this is actually that complicated to introduce to developers. The mental model could be as simple as:

  • "Before" hooks run first, synchronously
  • Load functions run next, async by default
  • "After" hooks run last, synchronously

Load functions are introduced earlier in the docs and tutorial, encouraging async by default, with hooks beign introduced under the "advanced" section in both the docs and tutorial. You could really hammer the point home with a warning like:

Use +hooks.client.js and +hooks.server.js sparingly for things like authentication where you intentionally want route-level, blocking waterfalls.

As for the question about handle, handleError, handleFetch, they could be simply answered with "they all work the same as with any other middleware". If a route wants to handle requests, errors, or intercept fetches, let it. If a developer wants to selectively opt-in to anything else, they can do so with their own middleware.

Finally, this all could be made a bit more simple in SvelteKit v2 by deprecating src/hooks.*.js in favor of a top-level src/routes/+hooks.*.js. 🙂

This was referenced Dec 2, 2023
@endigma
Copy link

endigma commented Feb 12, 2024

I think if people are going to use SK as their backend with a filesystem based router that router regardless of its description needs to support middleware. If I were using express or koa or whatever I would have middleware groups, which solve this problem entirely.

Whether it's called hooks or not, I don't see how a functional router can exist without supporting middleware. If I made an API router that you had to string match the URI to choose to run middleware it would be DOA, I don't see how SK can be a mature full stack solution without this.

@benmccann benmccann changed the title Add support for +hooks.server.js route files Add support for per-directory +hooks.server.js route files Feb 12, 2024
@benmccann
Copy link
Member

SvelteKit does now support +hooks.server.js: https://kit.svelte.dev/docs/hooks#server-hooks

I've renamed this issue to clarify that the request here is to be able to include such a file in each routes sub-directory

@oyejorge
Copy link

If a hooks.server.js per directory is too complicated, maybe a hooks.server.js per routing group would be a good compromise.

src/routes/
│ (app)/
│ ├ hooks.server.js <----
│ ├ dashboard/
│ ├ item/
│ └ +layout.svelte
│ (marketing)/
│ ├ hooks.server.js <----
│ ├ about/
│ ├ testimonials/
│ └ +layout.svelte
├ admin/
└ +layout.svelte

@dslatkin
Copy link

Personally, I feel like doing it per routing group would make it more complicated.

Hooks can be defined in one top-level hooks file for all routes in /src/hooks.server.js or they may be defined per routing group in /src/routes/. If you want a set of hooks to affect only one directory, create a routing group with just that one directory.

vs. something like

All hooks are defined per route through +hooks.server.js files, similar to +layout.server.js files.

@endigma
Copy link

endigma commented Aug 14, 2024

I don't think there's a major difference in complicatedness, and having both would be even better.

@dslatkin
Copy link

having both would be even better.

"Having both" is achieved with the second description I shared which is why I would advocate for it over a special exception for something that only works within layout groups.

@oyejorge
Copy link

oyejorge commented Aug 20, 2024

Personally, I feel like doing it per directory would make it more complicated.

Hooks can be defined in one top-level hooks file for all routes in /src/hooks.server.js or they may be defined in /src/routes or any directory under /src/routes. If you want a set of hooks to affect only one directory, you won't be able to create hooks.server.js file in any of the parent directories or in the /src folder.

vs. something like

Hooks can be defined either in /src/hooks.server.js or /src/routes/(routing-group)/hooks.server.js

@dslatkin
Copy link

dslatkin commented Aug 20, 2024

That quote is not what I'm suggesting - remove /src/hooks.server.js in favor of /src/routes/**/+hooks.server.js files following a middleware pattern, so the old /src/hooks.server.js becomes /src/routes/+hooks.server.js.

@EMPTYVOID-DEV
Copy link

That quote is not what I'm suggesting - remove /src/hooks.server.js in favor of /src/routes/**/+hooks.server.js files following a middleware pattern, and the old /src/hooks.server.js is is replaced by a top-level /src/routes/+hooks.server.js.

I feel like that just makes sense. Pattern matching the pathname in a large application is a pain, and it sometimes causes performance issues since the same middleware runs for every single request.

@eltigerchino
Copy link
Member

eltigerchino commented Oct 6, 2024

Wanted to share Rich's user-land implementation for having per-directory hooks
https://github.com/Rich-Harris/declarative-handlers#option-two--glob-imports

and an interesting read from the same README about why we might not want to implement this natively:
https://github.com/Rich-Harris/declarative-handlers#declarative-handlers-in-sveltekit

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

10 participants