-
-
Notifications
You must be signed in to change notification settings - Fork 574
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] feat: introduce Partial Content Middleware #3516
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 #3516 +/- ##
==========================================
- Coverage 95.58% 94.32% -1.26%
==========================================
Files 155 158 +3
Lines 9357 9589 +232
Branches 2749 2786 +37
==========================================
+ Hits 8944 9045 +101
- Misses 413 544 +131 ☔ View full report in Codecov by Sentry. |
I like this design since it is much simpler and supports various adapters and middlewares. One small concern is performance. This design assumes an underlying stream supports range access via Stream.getReader/subarray. But if the underlying Stream reads bytes from the beginning eagerly, range access is not efficient. On the other hand, #3461 requires middleware to provide range access. |
Most of JS runtimes's file system API provides seek API. e.g.: using file = await Deno.open('./image.jpg', { read: true })
await file.seek(6) // Move the file pointer 6 bytes forward
// ... If the middleware or serveStatic can access those APIs, performance problem might be resolved. In web standards, there is |
Hi @yusukebe! What are the real-world problems we need to solve? If the case of returning “206 Partial Content” is almost always serve-static, and if it is assumed to be used in a production environment, it seems better to be implemented in the serve-static feature rather than being made independent as middleware. There are no users who want to use serve-static but don't want to support Range headers, so I think it would be more convenient if Range headers were supported automatically when using serve-static. Is there a use case other than serve-static? If there are use cases, I think it would be best to make them into separate middleware like this pull request and then optimize serve-static by adding a separate abstraction layer. |
For Hono users on AWS or GCP (not Cloudflare), serving content from object storage (like AWS S3 or GCP Cloud Storage) will be an use-case of Partial Content middleware, without Server Static middleware. |
Hi @exoego, Thanks for the reply.
I simply don't know much about this, and I'd like to ask for some guidance, but if the app is acting as a proxy for AWS S3 or GCP Cloud Storage, I think it would be better to pass the Range header straight through to the storage and return the response as is, but is that not possible? If you're caching the data in your app, that's one thing, but if you're not caching it, I think it would be a bit of a pain to “fetch all the data from AWS S3 or GCP Cloud, put it in memory, and then return a portion of it by specifying the Renge header”. |
That's an interesting topic. Quick response! Also, if you use Cloudflare R2, which is object storage, you have to create Workers with Hono to distribute assets (though there is another way to distribute it without Workers): import { Hono } from 'hono';
type Env = {
Bindings: {
BUCKET: R2Bucket
}
}
const app = new Hono<Env>()
app.get('/', async (c) => {
const file = await c.env.BUCKET.get('file-name')
return new Response(file?.body)
}) |
It looks like AWS S3 supports partial content natively. One needs to specifie So please forget AWS S3 (I don't know GCP though) |
app.get('/', async (c) => {
const file = await c.env.BUCKET.get('file-name')
return new Response(file?.body)
}) In this example of Cloudflare R2, the underlying file is fully-read, which is not ideal for Partial Content. |
I've investigated. For Cloudflare R2, |
Indeed, there may not be a use case. BUT, I am personally concerned about Serve Static getting fat. That needs to be re-designed and discussed. |
Anyway, thank you for your comments! I'll make this PR as a draft and prepare to discuss about Serve Static and others. Now, we are implementing runtime-specific matters like file system access and WebSockets in adapters in I think it's time to re-think the scope of the Related issues and PRs: |
This PR will introduce a new middleware, "Partial Content Middleware".
It's based on #3461 by @exoego . That PR means the Serve Static Middleware supports 206 Partial Content. But this PR can support all contents not only by Serve Static Middleware:
Then, the
/hello.jpg
can support 206 Partial Content. It will be enabled when the Response has aContent-Legnth
header. The partial contents will be returned based on those values. So, if you want to use it with Serve Satatic, it should return the correct content length withContent-Length
--- but the current Serve Static Middleware does not emit content length. So, to do it, we have to modify it to get the file size using a method such asBun.file()
and set it as content length.This PR will be co-authored by @exoego
We have to discuss this feature.