feat: forward target URL as the 4th argument in error events#135
feat: forward target URL as the 4th argument in error events#135bjohansebas wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR enhances the proxy's error event emission to include the proxied target URL as a 4th argument, enabling error listeners to identify which upstream target caused the failure. The implementation updates the middleware error callback signature and the server error emission path, with tests verifying the behavior in both direct proxy errors and middleware exception scenarios. ChangesError Event Target URL Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
225-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMiddleware error callbacks must pass the URL as the 4th argument on all code paths.
The callback on line 138 correctly passes all 4 arguments:
callback(err, req, res, url). However, the socket error handlers at lines 160 and 169 invoke the callback with only 3 arguments:callback(err, req, socket). Since the callback signature hasurl?as optional, those invocations will receiveundefinedfor the url parameter, which will causeurlto be undefined when the error event is emitted (line 240 in server.ts). Either pass the url/target to the callback on all paths, or reconsider the error handling flow for socket-level errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 225 - 245, The middleware error callback passed into server._getPasses(type) must always be invoked with four arguments (error, req, resOrSocket, url); update the socket-level error handlers that currently call callback(err, req, socket) to pass the url/target as the fourth parameter (e.g., the same url/target used when invoking pass or requestOptions.target) so that the downstream logic that checks server.listenerCount("error") and calls server.emit("error", error, req, res, url) always receives a defined url; ensure all code paths that call the callback (including socket error handlers) use the same callback signature.
🧹 Nitpick comments (1)
test/index.test.ts (1)
185-253: 💤 Low valueRecommended: extract shared setup helper for the two
middleware pass exceptionstests.The target-server and proxy-server scaffolding (lines 186‑230) is a near-verbatim copy of the test at lines 112‑158. A small helper would shrink the file and reduce drift between the two cases. Non-blocking — the assertions and finally-block cleanup are correct as-is.
♻️ Sketch of a shared helper
async function setupProxy(throwing: () => never) { const target = await listen((_req, res) => res.end("ok")); // reuse `listen` from top of file const proxy = createProxyServer({ target: target.url }); proxy.before("web", "", throwing); const proxyServer = await listen((req, res) => { void proxy.web(req, res); }); return { target, proxy, proxyServer }; }Each test then becomes setup + the unique assertion + finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/index.test.ts` around lines 185 - 253, The test duplicates server/proxy setup in two places; extract a shared helper (e.g., setupProxy or reuse the existing listen helper) that creates the target HTTP server (res.end("ok")), calls createProxyServer({ target: target.url }), registers the before("web", ...) handler via proxy.before, and starts the proxyServer that calls proxy.web for incoming requests, returning { target, proxy, proxyServer } so both tests can call the helper and keep their unique assertions and cleanup in the finally block; locate references to createProxyServer, proxy.before, proxy.web and the current target/proxy/proxyServer variables to implement this refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server.ts`:
- Around line 225-245: The middleware error callback passed into
server._getPasses(type) must always be invoked with four arguments (error, req,
resOrSocket, url); update the socket-level error handlers that currently call
callback(err, req, socket) to pass the url/target as the fourth parameter (e.g.,
the same url/target used when invoking pass or requestOptions.target) so that
the downstream logic that checks server.listenerCount("error") and calls
server.emit("error", error, req, res, url) always receives a defined url; ensure
all code paths that call the callback (including socket error handlers) use the
same callback signature.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 185-253: The test duplicates server/proxy setup in two places;
extract a shared helper (e.g., setupProxy or reuse the existing listen helper)
that creates the target HTTP server (res.end("ok")), calls createProxyServer({
target: target.url }), registers the before("web", ...) handler via
proxy.before, and starts the proxyServer that calls proxy.web for incoming
requests, returning { target, proxy, proxyServer } so both tests can call the
helper and keep their unique assertions and cleanup in the finally block; locate
references to createProxyServer, proxy.before, proxy.web and the current
target/proxy/proxyServer variables to implement this refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83e08256-7de1-4caf-bf18-4e8796faefb9
📒 Files selected for processing (3)
src/server.tstest/http-proxy.test.tstest/index.test.ts
1 participant
The
errorevent listener on aProxyServeronly receives(err, req, res), the 4th argument (thetargetURL the proxy was trying to reach) is alwaysundefinedhttpxy/src/server.ts
Line 15 in 9dc2425
This breaks consumers that rely on it for logging or error handling. The most visible case is
http-proxy-middleware@4, whose defaultloggerPluginproduces messages like:instead of showing the actual target (
http://unknown:1234/).This matches the original
http-proxy@1.18.1behaviour:his helps resolve webpack-dev-server being able to update http-proxy-middleware, which now uses this package webpack/webpack-dev-server#5663
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests