-
-
Notifications
You must be signed in to change notification settings - Fork 579
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
According to this PR, we could say that the following policies apply.
|
This is a draft of one approach to the following comments. |
Hi @usualoma ! Thank you for the PR. Using the Node.js API, However, though I think you already noticed it, this We should consider where to put runtime-specific things. |
In addition, if we merge this, this issue can be resolved: #3483 |
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
bun run format:fix && bun run lint:fix
to format the code