Docs: Explain how middleware can execute only for routes

csvn commented 1 year ago

The Fresh documentation seems slightly misleading with regards to when Middleware code is executed. The docs state the following:

Route middleware A middleware is defined [...] to perform custom logic before or after the route handler.

But when generating a new Fresh project and creating a middleware that logs each Request.url, we see URLs logged from files that are not "routes" (i.e. under the routes/* folder):

GET requests for /

``` http://localhost:8001/ http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/deserializer.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/signals.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/chunk-PDMKJVJ5.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/chunk-UGFDDSOV.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/plugin-twind-main.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/main.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/chunk-FJBXTIC3.js http://localhost:8001/_frsh/js/edcc08a7371927e996ddee1a948a0cbd0df68a05/island-counter.js http://localhost:8001/_frsh/refresh.js http://localhost:8001/logo.svg?__frsh_c=edcc08a7371927e996ddee1a948a0cbd0df68a05 http://localhost:8001/_frsh/alive http://localhost:8001/favicon.ico ````

Non-route requests

/_frsh/* for client Javascript and Dev socket (x10)
Files from the /static folder (favicon.ico and logo.svg)

Motivation

I think there are great cases for both cases; running middleware only for /routes and running for all requests (including static files and files served by the Fresh runtime).

Running for all requests

Logging
Custom headers
Access control (hiding access to images for anonymous users etc)

*Running only for `/routes/`**

Setting (session) cookie(s)
Notifications/Other UI status (from e.g. Deno KV)
Retrieving navigation-tree (often loaded from other systems in E-commerce)

For the latter scenarios, it's common to add data to ctx.state so that it's available to routes when rendering the app. But it would be very bad if these run for e.g. Javascript files.

export async function handler(req: Request, ctx: MiddlewareHandlerContext<State>) {
  const [notifications, navTree] = await Promise.all([
    getUserNotifications(req.headers),
    getNavigationTree()
  ];
  ctx.state.notifications = notifications;
  ctx.state.navTree = navTree;
}

Feature request

It would be great if it was possible to somehow configure that /routes/_middleware.ts will run only for all routes in the /routes folder, and ignore static/fresh files. I think the most intuitive thing could be that "global" middleware is configured in main.ts as StartOptions and /routes/_middleware.ts only handles routes, but that would be a breaking change.

If it's out of scope to separate global/route middleware, then it would be nice to have some utility/helper method that can help filter out requests for static/fresh files (see workaround below).

Current workarounds

The best I've come up with so far to identify "route" requests for middleware is:

Ignore URLs that start with /_frsh/
If HTTP method is GET, check headers to determine if top-level navigation, using one of the below options
- Sec-Fetch-Mode header is navigation
- Sec-Fetch-Dest header is document
- Accept header has text/html

The above is not perfect, but at least it seems to mostly work okay, and avoids extra network requests for static and fresh-generated files.

deer commented 1 year ago

Does https://github.com/denoland/fresh/pull/1123 solve your problem? Although I see this isn't mentioned in the docs, so I suppose if 1123 solves your problem, then this request should be changed to "document middleware destination feature".

csvn commented 1 year ago

Oh, I didn't know about ctx.destination! So I guess ctx.destination === 'route' would perfectly match my use case. Yeah, #1123 should match perfectly. I read through all documentation about middleware and checked available properties on the context object, but I guess I didn't immediately connect "destination" to filtering out request based on type.

I think it would be great if documentation would be a bit clearer that by default middleware runs for all requests, but that there is a option to easily filter them to run only for e.g. routes via ctx.destination.

Should I update the issue description to reflect this, or is it better with a new issue?

deer commented 1 year ago

I think it makes sense to change the title of this issue. And are you saying that my PR isn't quite clear enough? Should I add something else or change the wording?

csvn commented 1 year ago

I think it makes sense to change the title of this issue. And are you saying that my PR isn't quite clear enough? Should I add something else or change the wording?

No, I meant since basically the entire issue description was written based on the assumption there was no easy way to determine routes, maybe it was better to create a new issue than edit everything. I wasn't commenting on your PR.

I've updated the title at least to be more clear. Haven't had time yet to update the issue description.

deer commented 1 year ago

Ok, thanks for clarifying. Glad you're pleased with the wording. Hopefully the addition will save people time in the future 🤞

denoland / fresh