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

Move Node.js serveStatic to this repo #3483

Open
yusukebe opened this issue Oct 3, 2024 · 4 comments
Open

Move Node.js serveStatic to this repo #3483

yusukebe opened this issue Oct 3, 2024 · 4 comments
Labels
enhancement New feature or request.

Comments

@yusukebe
Copy link
Member

yusukebe commented Oct 3, 2024

I'll propose to move the Node.js Server's Serve Static Middleware in honojs/node-server to this honojs/hono repo.

Create serveStatic in hono/nodejs

As mentioned in the comment: honojs/node-server#203, we have a base implementation of Serve Static Middleware, but Node.js Server(@hono/node-server developed in honojs/node-server) does not use it. That implements the same logic, and there are lots of duplicated stuff.

We can use the base Serve Static Middleware in the honojs/node-server, but there are problems:

  • The base Serve Static will be modified in the future.
  • The Serve Static in honojs/node-server should follow it.
  • If so, we have to update the peerDependecies(hono package) in package.json of honojs/node-server.

This is the pain point. So I considered and and found that it's good to move the Node.js Server's Server Static to this repo like:

import { serve } from '@hono/node-server'
import { Hono } from 'hono'
import { serveStatic } from 'hono/nodejs'


const app = new Hono()

app.use('/static/*', serveStatic({ root: './' }))

serve(app)

This means we create a Node.js Adapter (not an "Adapter Server") like the adapters for Cloudflare Workers, Deno, and Bun.

Should The Node.js Server be in the independent repo?

@exoego said in the comment:

it is much simpler if Node.js adapter is in Hono main repo.
*In this sentence, Node.js adapter means Node.js (Adapter) Server in honojs/node-server.

I can understand that opinion, but the Node.js Server should be honojs/node-server though the Serve Static for Node.js will be in this main repo.

Node.js almost completely has Web Standards APIs like fetch, Request, Respoense, etc. But Node.js does not have an "entry point" for Web Standards APIs.

For Cloudflare Workers, Deno, and Bun, we can run the following app with one command:

export default {
  fetch: () => new Response("Hi!")
}

Below is Bun case:

bun run hello.ts

However, how to run the app on Node.js?? The answer is we have to convert Node.js's HTTP API IncomingMessage and OutgoingMessage to Web Standards' Request and Reponse. The Node.js Server is doing that.

How about making it an adapter in this repo like hono/cloudflare-workers or hono/bun or hono/deno? They are for adapting between differences in runtimes. For example, Serve Static and WebSockets. However, the role of converting Node.js's API between Web Standards is improper. It assumes a greater role. So, keep using hono/node-server is good.

Plus, @hono/node-server is used in other packages like @hono/vite-dev-server as an independent library against hono package.

Next Actions

Shall we move the Server Static in hono/node-server to this repo?

  • Stop developing the Serve Static in hono/node-server. Close feat(serve-static): support absolute path for root node-server#202
  • Mark the Serve Static in hono/node-server as deprecated.
  • Create serveStatic in hono/nodejs, which uses Serve Static base implementation in this repo.
  • We probably have to re-design and add implementation to the base Serve Static Middleware.
  • If it succeeds, it will be easy to follow the base Serve Static Middleware change. We don't have to care about the peerDependcies of @hono/node-server.
@yusukebe
Copy link
Member Author

yusukebe commented Oct 3, 2024

Hey @usualoma @exoego @sor4chi @nakasyou @watany-dev @ryuapp ant others!

Please share your opinion if you have it. And if you want to implement it, tell us!

@nakasyou
Copy link
Contributor

nakasyou commented Oct 3, 2024

I basically agree to move Node.js serveStatic to this repo.

As you said, we can change serveStatic API easier. Currently, we have to create at least 2 PR (this repo, Node.js) to change serveStatic interface.
I guess one of the reasons to putting Node.js serveStatic adapter on other repo is that the serveStatic adapter depends node: API. However, Bun serveStatic adapter also depends node: API.

Or, to make creating serveStatic easy, I have an idea.

export const serveStatic: ServeStatic = defineServeStatic({
  async open(path: string): ReadableStream<Uint8Array> {
    // ...
  }
})

In this idea, we can bundle common processes in on one place. And modifying API interface is easy.

And, in my opinion, I don't like to move Node.js adapter to this repo. Runtimes which this repo supports as adapter such as Deno, Cloudflare Workers, and Bun are using Request/Response API. In contrast it's necessary to translate Request/Response for using Node.js. The process can't be called as small. So I think it's good that Node.js adapter is in other repo.

@exoego
Copy link
Contributor

exoego commented Oct 3, 2024

I think it is more natural to have the entire Node.js adapter here, since

  • Translating Node.js's HTTP API to Web Standards' API sounds like the exact role expected for adapter.
  • This repo already have adapters for aws-lambda, lambda-edge, netlify and so on, which are basically Node.js, IIUC.
  • It is much simpler for contributors if a common change is needed to many adapters.

But it's OK for me to move only serveStatic middleware, which seems a quickly agreeable option at this moment.

@rafaell-lycan
Copy link

I'd avoid using it from hono/nodejs in favor of hono/serve-static instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

4 participants