-
-
Notifications
You must be signed in to change notification settings - Fork 621
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
Extending Context #414
Comments
It must be something good thinking. it implies in all design philosophy of project |
Hi, @yusukebe 👋 I saw this #410 and I came in here. I want to set any values to the context in the middleware. and use it on main handler. In my middleware (firebase-auth) is example here. async (c: Context, next: Next) => {
const authorization = c.req.headers.get(config.AuthorizationHeaderKey);
if (authorization === null) {
return new Response(null, {
status: 400,
});
}
const jwt = authorization.replace(/Bearer\s+/i, "");
const auth = Auth.getOrInitialize(
c.env.PROJECT_ID,
config.KeyStore ??
WorkersKVStoreSingle.getOrInitialize(
c.env.PUBLIC_JWK_CACHE_KEY,
c.env.PUBLIC_JWK_CACHE_KV
)
);
try {
// I want to set "firebaseToken" value to the context. (but I can't do it now?)
const firebaseToken = await auth.verifyIdToken(jwt, c.env);
await next();
} catch (err) {
console.error({
message: "failed to verify the requested firebase token",
err,
});
return new Response(null, {
status: 401,
});
}
} |
Hi @Code-Hex ! In fact, there is such the feature that is not documented: const v = c.get('key')
c.set('key', 'value') How about using these methods? If they are used often, I would like to document them. However The And, also we want to implement adding "function" to Context, such as |
@yusukebe Nice API!! It's enough for me. Personally, I can't agree with extending context after adding some functions because if we use some middleware and their middleware extends the context, it has the potential to cause unexpected behavior by overriding same name methods. I think it would be easier for users to understand if the context was as simple as possible to implement. I wonder though maybe get method should return any or unknown and the user should use typeguard if want a specific type. |
@Code-Hex Exactly, extending the context will make that user confused. It should be easy to understand and should be a simple implementation. Hmm. We may not have to implement "extending context"... We can make the most of "get/set" method instead. |
I would like this ability, but mostly because I would like to add something like a That said, I'm fine creating a helper like |
Hi @yusukebe
Could you provide some use cases for this, in my thinking, if we are put something heavy into
Could you share more about the |
There would a |
Hi @metrue !
For example, I'm planning to make the "Validator Middleware" #425. In this middleware, the user must get the results in a handler. So, I thought that app.use('/', validator(...))
app.get('/', (c) => {
const result = c.getResult()
return c.json(result)
}) BUT, I notice we don't have to extend the Context. We can write it without app.use('/', validator(...))
app.get('/', (c) => {
const res = validatorResult(c)
return c.json(res)
}) Let's see implementation. In the "validator" method, set the result using ctx.set('validatorResult', result) And, the const validatorResult = (ctx: Context): Result => {
return ctx.get('validatorResult')
} I think this way is better than extending Context. For instance, @Code-Hex 's "Firebase Auth Middleware" is using this method: https://github.com/honojs/firebase-auth/blob/main/src/index.ts#L77-L84 |
Hi @yusukebe I also think it is not a good practice for middleware to add methods to Context object. However, I think that in some cases it is not a bad idea for middleware to extend type definitions. For example, making changes like the following PR... Middleware will be able to add types as follows... declare module 'hono' {
interface ContextVariableMap {
calculationResults: number
}
} This allows users to benefit from type definitions. It can also detect before execution if the same key is already in use by other middleware. I think helper functions are the best practice for this issue, so #429 will be an adjunct, but I think it will help solve some of the problems. |
@usualoma Ultra Great! I've tried it for "Validator Middleware" which is WIP, it worked, and I've felt fine DX. It seems that writing Let's get this feature in! And, we need to consider... const result = c.get('validatorResults') // function pattern
const result = validatorResult(c) // method pattern We/Middleware developers can provide both patterns, but should we choose one? or both? |
I think that is what bothers me too. While being able to declare type to values of set/get is an interesting feature, I believe the following is often the good pattern for users (who are not middleware developers). const result = validatorResult(c) |
Hi @usualoma !
Agree. I think it would be easier for users to understand if there is an explicit method. Though we have this guideline, feature #429 is good for us. I'll merge it. |
@yusukebe Thinking about this stuff I had a thought. What if there was a way to provide the initializer for a context key: app.var("validatorResults", (c) => {
return ...
}) With some internal handling of get calls: class Context {
...
var(key: string) {
let value = this.get(key)
if (!value && this._initializers[key]) {
this.set(key, value = this._initializers[key](this))
}
return value
}
} |
Not yet big experience with Hono, but from my superficial usage I have 2 thoughts: Alternative: Infer additional types from middleware type ContextExtend = {
foo: string
}
const myMiddleware: Middleware<ContextExtend> = ...
app.get('/my-endpoint-with-middleware', myMiddleware, c => {
// context was extend by middleware, via generic inference on the myMiddleware argument
c.context.foo
}) And in the case when the middleware is defined on the app level, we could try to use a fluent interface or inject middlewares during construction, to give the extend context in the resulting type const app = createHono(...middlewares)
// alternative
const app = createHono()
.use(middleware)
.use(middelware2) I hope I could communicate the base ideas. This is just high level idea and they probably require some typescript trickery, but theoretically possible. |
I just spend a short while on doing a demo of how something like this could be achieved. I do not think it you should be scared of letting the middleware changing the type Context<Type> = {
[Property in keyof Type]: Type[Property]
}
type App<T> = {
use: <U>(a: (ctx: Context<T>) => Context<T> & Context<U>) => App<U> & App<T>
get: (path: string, callback: (ctx: Context<T>) => void) => void
}
const app: App<unknown> = {} as any
const x = (ctx: Context<any>) => {
ctx.hello = 'world'
const newContext: typeof ctx & Context<{ hello: 'world' }> = ctx
return newContext
}
app.use(x).get('', (ctx) => {
ctx.hello // Exists
return ctx
})
app.get('', (ctx) => {
ctx.hello // Doesn't exist
return ctx
}) As you can see from the example, when using |
Hi, I'm interested in this too. I'm using Prisma with Hono for Cloudflare Workers target. It would be cool to have a property |
While it's true that writing declare module 'hono' {
interface ContextVariableMap {
prisma: PrismaClient
}
}
const prismaMiddleware: MiddlewareHandler = async (c, next) => {
// ...
c.set('prisma', prismaClinet)
await next()
}
app.get('/list', prismaMiddleware, (c) => {
const prisma = c.get('prisma')
// ....
}) |
Thanks @yusukebe. Is there a specific reason why extending the I'm using Cloudflare Workers and facing issues with the Prisma client library. Instantiating the client outside of a request is not possible due to limitations with Cloudflare Workers or the library itself. The values required for client instantiation are only accessible through |
You can do that with this code: type Environment = {
Bindings: {
PRISMA_URL: string
}
}
const app = new Hono<Environment>()
const prismaMiddleware: MiddlewareHandler<Environment> = async (c, next) => {
const prismaUrl = c.env.PRISMA_URL
// pseudo code
const client = new PrismaClient({
url: prismaUrl,
})
c.set('prisma', client)
await next()
}
As discussed above. We don't use what we don't need and we don't want to complicate Types. At least we won't be getting into this any time soon. |
Thank you @yusukebe |
I have been trying to write a similar dependency injection middleware, and while it does work... I still get type errors/warnings in my IDE (latest WebStorm 2023.2.1). I have tried various methods, including the one described on the docs but still get type errors on type Variables = {
container: Record<string, any>
}
const app = new Hono<{Variables: Variables}>()
const containerMiddleware = async (c, next) => {
const container: Record<string, any> = {
// ... register various services here
}
c.set('container', container)
await next()
}
app.get('/test', containerMiddleware, (c) => {
const container = c.get('container')
// ^-- "Argument types do not match parameters"
return c.text(`${container.message}: ${container.name}`)
}) Considering the code works fine, I am okay ignoring it but it seems like I am not getting any type-safety since it still thinks My expectation would to be have something like an optional // Initialize some dependency injection container
const container: Record<string, any> = {}
type AppState = {
container: Record<string, any>
someOptional?: any
// other properties...
}
// Create the app using a constant value
const app = new Hono<AppState>({ container })
// Or a factory in the case of dynamic initialization
const app = new Hono<AppState>(() => {
// do some initialization...
return { container }
}) Then this could be accessed via app.use('*', async (c, next) => {
const { container } = c.state
container.message = 'hello from the container'
await next()
})
app.get('/', (c) => {
const { message } = c.state.container
return c.text(`The container says: ${message}`)
}) |
Is there a way to only declare the attribute on ContextVariableMap if the given middleware is used on the path? So only allow c.get("foo") when FooMiddleware is actually used in the chain before the request. |
Hi @vassbence , You can do it with this code: import { createMiddleware } from 'hono/factory'
const echoMiddleware = createMiddleware<{
Variables: {
echo: (str: string) => string
}
}>(async (c, next) => {
c.set('echo', (str) => str)
await next()
})
app.get('/echo', echoMiddleware, (c) => {
return c.text(c.var.echo('Hello!'))
}) |
Lovely, thanks! |
The variable types do not seem to get propagated if the middleware is wired up via const someMiddleware = createMiddleware<{ Variables: { foo: string } }>(...);
const app = new Hono()
.use(someMiddleware())
.get('/', c => {
c.var.foo // <- fails here
})
.get('/other', someMiddleware(), c => {
c.var.foo // <- works here
}) I feel like |
That is a thing which is not implemented or a just bug. But we may be enable to fix it. |
Is there a reason not to do it similar to Express with https://expressjs.com/en/5x/api.html#res.locals Could you make the I feel like this is easier to reason about and easier to debug. If I want to know what's been added by mine or any other middleware I just log |
Express is not a typescript native. Those types were added after.
In typescript we like to be precise. |
@vsviridov I'm not a Typescript expert, so please forgive my ignorance, but I don't see how the black box of |
Is there a way to get this working with JSDoc-annotated JavaScript project in VSCode? I tried adding the module thing in a .d.ts file, but ti does not work. |
I export black boxes when I want to make it clear that you should be doing runtime validation of a variables content. Especially when finding a way to express that statically would be too much work. I'll use https://zod.dev to synch my runtime validation with my static types. |
Don't you think this system looks unsafe especially on large applications? If the application gets large enough and you have multiple services in the context that depend on each other, there could be problems. Especially after refactorings when you need to swap some middlewares. The fastify plugin/decoration system also has typing problems in this respect, but you can explicitly describe that for example to initialize a context in this plugin you need a plugin with a certain name. Perhaps hono should also add something like that? |
Astro's system is better IMHO. It's fully typed in my editor. https://docs.astro.build/en/guides/middleware/#middleware-types |
Thanks a ton for this—just spent an hour trying to track this down. Made a PR on the website (honojs/website#460) to add it to the docs. |
I think this is the best solution for now. The only issue I see is that the Context has the echo var before it's set, so forgeting the to add c.set('echo', (str) => str) and calling c.get('echo') in a handler would be ok for TS but will break at runtime. |
Hey, we wanted to add a global logger via context, and we could use set/get, but it is a bit more verbose than simply doing |
With a few extra characters you can do |
I noticed that when using Example:
|
Using both https://github.com/honojs/node-server?tab=readme-ov-file#accessing-nodejs-apiv |
We can close this issue because we now have the |
Agreed, it makes sense to use I was pointing out that you can't add
|
There is a request to extend Context. Discussed at #410 and honojs/website#1
The possibility of adding
c.afunction()
by the middleware is a good feature, I think.But I don't have any idea to implement it. One idea is using
declare module
:It seems to be not smart. Is there someone have other ideas?
The text was updated successfully, but these errors were encountered: