Open KaelWD opened 2 months ago
I think this is not a bug. If I organize your code, it will look like this:
import { Hono } from "hono";
const app = new Hono();
app.use(async (_, next) => {
console.log("Middleware A start");
await next();
console.log("Middleware A end");
});
app.get("/a", (c) => {
return c.text("A");
});
app.use(async (_, next) => {
console.log("Middleware B start");
await next();
console.log("Middleware B end");
});
app.get("/b", (c) => {
return c.text("B");
});
export default app;
Middleware is executed in the order in which it is registered, but in the case of /a
, middleware registered after it will not be executed.
The execution order is written in the documentation.
https://hono.dev/docs/guides/middleware#execution-order
That doesn't say anything about .route(), I'd expect middleware to only apply to the group.
If I organize your code
Please read the additional information section of the issue, this was originally just one middleware repeated in each group for typescript reasons. I understand how it's happening, this behaviour is unintuitive though and doesn't seem useful. We can create route groups with scoped paths but they still leak middleware to the rest of the app. Expanding the example with groups this is how I expected it to work:
const router = new Hono()
.use(async (_, next) => {
console.log('middleware 2 start')
await next()
console.log('middleware 2 end')
})
.get('/1', (c) => {
console.log('handler 1')
return c.text('Hello!')
})
app.use(async (_, next) => {
console.log('middleware 1 start')
await next()
console.log('middleware 1 end')
})
app.route('/', router)
app.use(async (_, next) => {
console.log('middleware 3 start')
await next()
console.log('middleware 3 end')
})
app.get('/2', (c) => {
console.log('handler 2')
return c.text('Hello!')
})
GET /1
middleware 1 start
middleware 2 start
handler 1
middleware 2 end
middleware 1 end
GET /2
middleware 1 start
middleware 3 start
handler 2
middleware 3 end
middleware 1 end
I found a feature request for something similar, I don't see why this shouldn't just be the default behaviour though: #1674
This would also solve #2249
And #2794
app
.route('/foo/', new Hono().use(authMiddleware).route('/', PageFoo))
.get('/protected', authMiddleware, (ctx) => ctx.text('protected', 200))
Hi @KaelWD
Thank you for the issue. This is not a bug. But as you feel, it may not be intuitive. I'll consider this issue for a while.
The types actually already work like this:
const router = new Hono()
.use<{
Variables: {
test: 'override'
}
}>(async (c, next) => {
c.set('test', 'override')
await next()
})
const app = new Hono()
.use<{
Variables: {
test: 'root'
}
}>(async (c, next) => {
c.set('test', 'root')
await next()
})
.route('/', router)
.get('/test', async (c) => {
const test = c.get('test')
// ^? "root"
return c.text(test) // 'override', type doesn't match
})
Hi @KaelWD Thanks. Can you create a separate issue?
Hmmm, this is a difficult one.
There is no doubt that the current behaviour is in itself a specification. However, as the documentation recommends a similar approach, this certainly appears to be a behaviour that is certainly not expected. Grouping without changing base
route()
Plugs the subApp's routing under the specified path. (Here, it does not matter whether the routing is a middleware or a handler.)
In this case, the error handler is tied to the handler.
Given that compose is done when an error handler is set up, special handling for middleware could also be considered. However, I do not want to complicate things too much here.
Also, we want to realise things like Grouping without changing base in a simple way, so I don't want to complicate the syntax.
Realistically, there are two patterns that hit this problem
One idea would be to output the following warning message.
diff --git a/src/hono-base.ts b/src/hono-base.ts
index 1ee4b10d..e9b713dc 100644
--- a/src/hono-base.ts
+++ b/src/hono-base.ts
@@ -214,6 +214,8 @@ class Hono<E extends Env = Env, S extends Schema = {}, BasePath extends string =
path: SubPath,
app: Hono<SubEnv, SubSchema, SubBasePath>
): Hono<E, MergeSchemaPath<SubSchema, MergePath<BasePath, SubPath>> & S, BasePath> {
+ const subAppPaths = ((this as any)._subAppPaths ||= new Set(['/']))
+
const subApp = this.basePath(path)
app.routes.map((r) => {
let handler
@@ -225,8 +227,16 @@ class Hono<E extends Env = Env, S extends Schema = {}, BasePath extends string =
;(handler as any)[COMPOSED_HANDLER] = r.handler
}
+ if (subAppPaths.has(path) && r.path === '*') {
+ console.warn(
+ `It is not recommended to route() applications with wildcard middleware to same path "${path}" multiple times.`
+ )
+ }
+
subApp.addRoute(r.method, r.path, handler)
})
+
+ subAppPaths.add(path)
return this
}
What version of Hono are you using?
4.4.6
What runtime/platform is your app running on?
Node
What steps can reproduce the bug?
What is the expected behavior?
calling GET /a logs "Middleware A" calling GET /b logs "Middleware B"
What do you see instead?
calling GET /a logs "Middleware A" calling GET /b logs "Middleware A" "Middleware B"
Additional information
I was trying to avoid #2987 with a pattern like this instead of ContextVariableMap but appMiddleware ends up being called multiple times in later groups