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

[PoC] refactor: implement serve-static middleware with Node.js API #3523

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Oct 17, 2024

This PR also fixes #3324. Although it is less thorough than #3461 or #3516, it implements the minimum necessary processing of the Renge request header. (And I think the performance is not bad.)

What is this?

This also has something to do with the organization of serve-static and its integration with node-serve, but as for serve-static, I think that by importing “node:*”, we can use node-serve's serve-static.ts as-is in deno and bun. (I haven't checked whether it works with older versions of Deno and Bun, but I think it should work with the current versions.)
https://github.com/honojs/node-server/blob/main/src/serve-static.ts

Currently, the serve-static is abstracting to bridge the differences in runtime. Still, I think eliminating the abstraction would be simpler, and I would stop trying to absorb the differences with Hono.
https://github.com/honojs/hono/blob/main/src/middleware/serve-static/index.ts#L36-L40

If serve-static in cloudflare-workers is deprecated, I think it would be a good idea to consolidate middleware/serve-static when a major version is released and unify it into a version that uses “node:*”.

Alternatively, I think it might be a good idea to have an “adapter for serve-static that proxies to cloud storage such as R2, S3 and GCP Cloud Storage”. However, if you were to implement this, I think it would be better to have independent code without the current abstraction layer. I think the “Worse is Better” approach would be appropriate here.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 63.76812% with 50 lines in your changes missing coverage. Please review.

Project coverage is 94.22%. Comparing base (90833d2) to head (01817e4).

Files with missing lines Patch % Lines
src/adapter/node/serve-static.ts 63.97% 49 Missing ⚠️
src/adapter/deno/serve-static.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3523      +/-   ##
==========================================
- Coverage   94.29%   94.22%   -0.08%     
==========================================
  Files         157      158       +1     
  Lines        9488     9561      +73     
  Branches     2763     2786      +23     
==========================================
+ Hits         8947     9009      +62     
- Misses        541      552      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@usualoma
Copy link
Member Author

According to this PR, we could say that the following policies apply.

  • Web requests and responses follow web standards
  • Access to the file system follows the Node.js API

@usualoma
Copy link
Member Author

This is a draft of one approach to the following comments.

#3516 (comment)

@yusukebe
Copy link
Member

Hi @usualoma !

Thank you for the PR. Using the Node.js API, node:*, for Deno and Bun is a good approach!

However, though I think you already noticed it, this hono repo still has runtime-specific APIs, including for WebSockets and ConnInfo. In my thoughts, the ideal solution may be to remove runtime-specific APIs from this hono repo and use only WebStandards APIs. If so, we must create another namespace for modules like this serve-static middleware.

We should consider where to put runtime-specific things.

@yusukebe
Copy link
Member

In addition, if we merge this, this issue can be resolved: #3483

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.

Support returning 206 Partial Content with serveStatic
2 participants