Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 49fc520 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe middleware now returns multipart Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Multipart range responses are a new public behavior. On filesystems without stream support, a file that disappears after it is checked can make the request fail without a proper error response. Response-data callbacks can receive a different stream type than the documented one. The test for the 200-range limit does not actually exercise that limit. These issues should be fixed before merging. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main changes: adding multipart byte-range request support. It aligns with the changed files and commit message. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (2 skipped: 2 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ast-grep (0.45.3)
-
test/middleware.test.js
-
ast-grep timed out on this file
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4f7e441b-2183-4068-9aa9-0c9bd97ef753
📒 Files selected for processing (6)
.changeset/multipart-range-requests.md.cspell.jsonsrc/middleware.jssrc/utils.jstest/middleware.test.jstypes/utils.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| } | ||
|
|
||
| if (shouldUseMultipart) { | ||
| const contentType = context.mimeTypes.contentType(filename); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use the selected response Content-Type for each part.
If headers overrides Content-Type, or mime.default supplies it for an unknown extension, this lookup does not return the Content-Type selected earlier in processRequest. The multipart parts can therefore identify the file with a different type or omit its type. Read the selected response header before setting the outer multipart Content-Type, and pass that value to createMultipartBody. RFC 9110 says each part should carry the Content-Type that the corresponding 200 response would have carried. (rfc-editor.org)
|
|
||
| if (shouldUseMultipart) { | ||
| const contentType = context.mimeTypes.contentType(filename); | ||
| const multipartResult = createMultipartBody({ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle synchronous multipart read failures.
If a filesystem has no createReadStream and the file disappears after stat, createMultipartBody throws from readFileSync. This call is outside the try that handles regular file-read failures. processRequest then rejects without sending the normal error response. Catch this call and pass the error to errorHandler, as the regular read path does.
| req, | ||
| res, | ||
| bufferOrStream, | ||
| /** @type {Buffer | ReadStream} */ (bufferOrStream), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the callback declaration and nearby usage without executing repository code.
rg -n -C 5 'ModifyResponseData|modifyResponseData|type ResponseData' types src README.mdRepository: webpack/webpack-dev-middleware
Length of output: 41759
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- diff ---'
git diff --no-ext-diff --unified=30 8a7af5e362f56610ace8d7dd0a4d277b4952c2a7 49fc520b8a20f25187096d5b0fdf45a8fe1119c4
printf '%s\n' '--- middleware imports and range path ---'
sed -n '1,120p' src/middleware.js
sed -n '930,1175p' src/middleware.js
printf '%s\n' '--- public callback declarations ---'
sed -n '1,100p' src/index.js
sed -n '160,205p' types/index.d.ts
printf '%s\n' '--- README callback documentation ---'
sed -n '318,345p' README.mdRepository: webpack/webpack-dev-middleware
Length of output: 44636
Expose multipart response data as Readable.
createMultipartBody returns Readable.from(body()), and the multipart branch passes that value to modifyResponseData. The callback contract still declares Buffer | ReadStream. A callback that uses a ReadStream-specific property can fail for multipart responses.
Update ResponseData, ModifyResponseData, and the README to use generic Readable. Keep Buffer; fs.ReadStream remains supported because it is a Readable.
🐛 Suggested fix
- * @property {Buffer | ReadStream} data data
+ * @property {Buffer | Readable} data data
...
- * @param {Buffer | ReadStream} data data
+ * @param {Buffer | Readable} data dataApply the equivalent changes in types/index.d.ts, import or define Readable from node:stream, and update the README from ReadStream to Readable.
| for (let i = 0; i < 201; i++) { | ||
| ranges.push(`bytes=${i * 2}-${i * 2 + 1}`); | ||
| } | ||
| const response = await req | ||
| .get("/bundle.js") | ||
| .set("Range", ranges.join(", ")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Generate 201 separate ranges for the cap test.
ranges.join(", ") repeats bytes= after the first entry, so range-parser discards those entries. Changing only that prefix is insufficient: the generated ranges are adjacent, and combine: true merges them. Use one bytes= prefix and leave a gap between ranges. The current test cannot reach the 201-range fallback and can fail its 200 assertion. (github.com)
Proposed test input
for (let i = 0; i < 201; i++) {
- ranges.push(`bytes=${i * 2}-${i * 2 + 1}`);
+ ranges.push(`${i * 3}-${i * 3 + 1}`);
}
const response = await req
.get("/bundle.js")
- .set("Range", ranges.join(", "));
+ .set("Range", `bytes=${ranges.join(",")}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 0; i < 201; i++) { | |
| ranges.push(`bytes=${i * 2}-${i * 2 + 1}`); | |
| } | |
| const response = await req | |
| .get("/bundle.js") | |
| .set("Range", ranges.join(", ")); | |
| for (let i = 0; i < 201; i++) { | |
| ranges.push(`${i * 3}-${i * 3 + 1}`); | |
| } | |
| const response = await req | |
| .get("/bundle.js") | |
| .set("Range", `bytes=${ranges.join(",")}`); |
1 participant
Summary
Refs #2454 (RFC: regroup the options), specifically the “Names worth fixing while we are here” section.
This PR renames two client options that are confusing when used together:
reload→reloadOnFailedUpdateurlPrefix→urlParamPrefixThe first one is especially confusing because
reloadandliveReloadsound like they do almost the same thing, but they actually have different triggers.liveReloadreloads the page after a successful build, whilereloadreloads the page when a hot update cannot be applied. Renaming it toreloadOnFailedUpdatemakes the behavior much clearer.Similarly,
urlPrefixis a bit vague because it doesn't make it clear that the option controls the prefix used for the URL query parameters generated by the client.urlParamPrefixdescribes that more accurately.Both changes are backward-compatible aliases, following the same approach already used for
cache/mimeand previously forhot.progress.How the aliases work
... deprecated and will be removed in the next major release. Use '<new>' instead.// TODO in the next major releasemarker.instance.context.optionsis publicly reachable.For RFC open question 1, I went with warning immediately, following the existing behavior of the
cache/mimeoption grouping. If maintainers prefer warnings only when the next major release happens, I'm happy to change this.For the other open question around
build.fileSystem, I interpreted it as shorthand for the existingoutputFileSystem. However, this PR doesn't make any changes to thebuildgroup or thehot.clientfold. Those are more design-heavy changes, so I've left them for the maintainers to decide based on the RFC's open questions.What kind of change does this PR introduce?
feat— renames two options while keeping the old names working as backward-compatible aliases.The changes cover:
src/options.jsonsrc/options.check.jsHotOptions/HotClientOptionsJSDoc typedefs.cspell.jsonDid you add tests for your changes?
Yes.
I added/updated tests covering:
The full
npm testsuite is also passing, including linting, schema checks, types, and e2e tests.Does this PR introduce a breaking change?
No.
Existing configurations using
reloadorurlPrefixwill continue to work. They will only receive a deprecation warning.The old names are planned to be removed in the next major release.
Documentation
The README has already been updated to use:
reloadOnFailedUpdateas the primary name, withreloadmarked as deprecatedurlParamPrefixas the primary name, withurlPrefixmarked as deprecatedNothing else should be needed when this PR is merged. The old names can be removed in the next-major migration notes alongside
cache/mime.Use of AI
Yes. I used Claude to help research the RFC and existing repository conventions, think through the implementation approach, draft parts of this PR description, and put together the verification checklist.
I reviewed the complete diff myself, ran the full test suite locally, and I'm responsible for the changes. I have also read the webpack AI policy.
Summary by CodeRabbit
206 Partial Contentresponses inmultipart/byterangesformat. Satisfiable ranges are served even when the request also includes unsatisfiable ranges; requests with no satisfiable ranges return416.