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: add custom headers option to serve static #195

Closed

Conversation

BlankParticle
Copy link

I was creating a custom server which would serve built css/js files, the files are hashed which means I can make their cache-control headers immutable.
serveStatic does not have an option to set any custom headers, so I added a new option that takes a function which can set custom headers and has the context as a parameter.

I have updated the tests and Readme to reflect the changes.

@BlankParticle
Copy link
Author

From a suggestion on discord, here is my usecase for this new API
currently I have a remix app where i need to serve static assets and fallback to the remix handler if there is no static asset.

So this was my initiall implementaion

app
  .use("*", serveStatic(...))
  .use("*", remixHandler(...))

This has the issue that I can't set cache control headers on only static assets due to lack of API

so I had to implement this custom solution

const assets = await readdir('./build/client', { recursive: true }).then(
  (files) => files.map((file) => `/${file}`)
);

const app = new Hono()
const handler = createRequestHandler(...);

app.use('*', async (c, next) => {
    if (assets.includes(c.req.path)) {
      c.header('Cache-Control', 'public, immutable, max-age=31536000');
      return serveStatic({ root: 'build/client' })(c, next);
    } else {
      return handler(c.req.raw);
    }
  })

this works, but it is verbose

with this new api, I can just do this

app
  .use("*", serveStatic({root: "build/client", headers: ()=>({'Cache-Control':'public, immutable, max-age=31536000'})}))
  .use("*", remixHandler(...))

@yusukebe
Copy link
Member

yusukebe commented Sep 8, 2024

Hi @BlankParticle

You can write the following:

app.use(
  '/static/*',
  async (c, next) => {
    await next()
    c.header('x-custom', 'foo')
  },
  serveStatic({ root: './' })
)

@BlankParticle
Copy link
Author

Hi @BlankParticle

You can write the following:

app.use(
  '/static/*',
  async (c, next) => {
    await next()
    c.header('x-custom', 'foo')
  },
  serveStatic({ root: './' })
)

I need to run both the static server and remix handler at *, there is no fixed route like /static/*
and need to make sure that the headers doesn't get applied if the asset is not served by static server.

@yusukebe
Copy link
Member

yusukebe commented Sep 8, 2024

@BlankParticle

How about honojs/hono#3396?

It is more flexible. The API design is good.

@BlankParticle
Copy link
Author

BlankParticle commented Sep 8, 2024

@BlankParticle

How about honojs/hono#3396?

It is more flexible. The API design is good.

That is actually better I think, it can be used to do more stuff than just headers

Also there are 2 serveStatic (one built into hono and one from node-server), do we need to keep them in sync?

@yusukebe
Copy link
Member

yusukebe commented Sep 8, 2024

@BlankParticle

do we need to keep them in sync?

Yes. We have to write the same logic on the Node.js serve static (Ideally, it's better the Node.js serve static also uses the hono's serve static).

@BlankParticle
Copy link
Author

hono's serve Static is not documented anywhere, thats why I reached for node-server's serve static, I think that needs some work too

@BlankParticle
Copy link
Author

closed as the equavalent has already been implemented in hono's builtin serveStatic

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