feat(http/unstable): accept HeadersInit for serveDir and serveFile headers#7190
feat(http/unstable): accept HeadersInit for serveDir and serveFile headers#7190minato32 wants to merge 3 commits into
HeadersInit for serveDir and serveFile headers#7190Conversation
…le` headers Closes denoland#6723
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7190 +/- ##
==========================================
- Coverage 94.64% 94.63% -0.02%
==========================================
Files 629 630 +1
Lines 51895 51902 +7
Branches 9373 9376 +3
==========================================
- Hits 49116 49115 -1
- Misses 2211 2217 +6
- Partials 568 570 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
Nice ergonomic improvement — accepting HeadersInit is much friendlier than hand-built "name:value" strings, and the implementation reads cleanly. Verified the redirect-skip matches stable serveDir and that headers is correctly stripped before delegating so there's no double-application. Coverage is good (record + Headers instance + redirect skip, for both serveDir/serveFile).
A couple of non-blocking notes inline. Also worth calling out in the changelog: this is a hard break for the unstable API (old headers: ["X-Extra: extra header"] callers now fail to type-check and throw at runtime via new Headers(...)). That's fine for unstable — just be explicit about it. LGTM overall.
| function appendHeaders(target: Headers, headers: HeadersInit): void { | ||
| const normalized = new Headers(headers); | ||
| for (const [name, value] of normalized) { | ||
| target.append(name, value); |
There was a problem hiding this comment.
Using .append() means custom headers are added alongside any the file server already set, rather than replacing them — e.g. passing cache-control would yield a comma-joined value rather than overriding. This matches the previous string[] behavior so it's not a regression, but now that the API takes a rich HeadersInit, callers may reasonably expect to override (e.g. set their own Cache-Control). Worth a one-line doc note on the headers option clarifying that values are appended, not replaced. (The cache-control test only passes because the dir-listing response doesn't pre-set that header.)
| const req = new Request("http://localhost/hello"); | ||
| const res = await unstableServeDir(req, { | ||
| ...serveDirOptions, | ||
| ...serveDirOptions as UnstableServeDirOptions, |
There was a problem hiding this comment.
The as UnstableServeDirOptions cast (repeated at 5 call sites) is forced by Omit<StableServeDirOptions, "headers"> + redefine: the spread carries stable's headers?: string[], which isn't assignable to HeadersInit. You could avoid all the casts by typing the shared serveDirOptions fixture as UnstableServeDirOptions (or using satisfies). Minor, test-only.
Document that serveDir/serveFile headers are appended rather than replacing pre-set headers, and type the test fixture with satisfies so the per-call UnstableServeDirOptions casts are no longer needed. Addresses @bartlomieju's review feedback.
|
@bartlomieju updated in 6f87cb1 — added a doc note on both |
Closes #6723.
ServeDirOptions.headersandServeFileOptions.headersin@std/http/unstable-file-servernow acceptHeadersInitinstead ofstring[], so callers can pass aHeadersinstance, a record, or a list of tuples without hand-rollingname:valuestrings.The stable
serveDir/serveFilekeep their existingstring[]signature, so the file-server CLI behaviour is unchanged. The unstable wrapper stripsheadersbefore delegating to the stable implementation and applies them to the response itself (skipping redirect responses, matching the stable behaviour).