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

feat: pass context to onNotFound callback in serveStatic #1865

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

sor4chi
Copy link
Contributor

@sor4chi sor4chi commented Dec 27, 2023

#1810

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 27, 2023

I had no idea how to infer Middleware Handler's Context and pass it to onNotFound argument's context.

@sor4chi sor4chi marked this pull request as draft December 27, 2023 05:44
@sor4chi sor4chi force-pushed the feat/serve-static-not-found-context branch from c92892e to e978bd9 Compare December 27, 2023 06:33
@sor4chi sor4chi force-pushed the feat/serve-static-not-found-context branch from e978bd9 to dfe060a Compare December 27, 2023 07:16
@sor4chi sor4chi marked this pull request as ready for review December 27, 2023 07:16
@yusukebe
Copy link
Member

@sor4chi

I had no idea how to infer Middleware Handler's Context and pass it to onNotFound argument's context.

Please tell us along with specific examples.

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 29, 2023

export type ServeStaticOptions<E extends Env> = {
  root?: string
  path?: string
  rewriteRequestPath?: (path: string) => string
  onNotFound?: (path: string, c: Context<E>) => void | Promise<void>
}

export const serveStatic = <E extends Env = Env>(options: ServeStaticOptions<E> = { root: '' }): MiddlewareHandler<E> => {

If we could do something like this, we should be able to access envs type-safely from the callback context, but is this impossible with the current design?

@yusukebe
Copy link
Member

@sor4chi

I struggled for hours to implement it, but I can't. Maybe it's impossible. I'll make more time to try.

@sor4chi
Copy link
Contributor Author

sor4chi commented Dec 30, 2023

Okay, Thank you so much!
Even if this is not possible, it might help users just to be able to put Env Generics in serveStatic. If this is not possible, I will do so.

@yusukebe yusukebe added the v3.12 label Dec 31, 2023
@yusukebe
Copy link
Member

yusukebe commented Jan 1, 2024

@sor4chi

Are you imagining the following code? If so, please implement that to make it true!

const app = new Hono<Env>()

app.get(
  '/static/*',
  serveStatic<Env>({
    onNotFound: (path, c) => {},
  })
)

@sor4chi
Copy link
Contributor Author

sor4chi commented Jan 1, 2024

Hi, @yusukebe.
I finished supporting Env support for context.

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

@sor4chi

Thanks! But, also please change the adapter/cloudflare-workers/server-static-module.ts.

@sor4chi
Copy link
Contributor Author

sor4chi commented Jan 2, 2024

@yusukebe fixed! thanks for finding it!

@yusukebe
Copy link
Member

yusukebe commented Jan 2, 2024

@sor4chi

Thanks! I'll merge this into "next" now.

@yusukebe yusukebe merged commit b8b0b38 into honojs:next Jan 2, 2024
11 checks passed
nakasyou pushed a commit to nakasyou/hono that referenced this pull request Jan 13, 2024
* feat: pass context to onNotFound callback in serveStatic

* chore: denoify

* test: update notFoundHandler's callback expect

* feat: add Env generics for serveStatic to support `c.env` type in callback handler

* feat: add Env generics for serveStatic to support c.env type in cloudflare workers module's callback handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants