feat: pick dns cache commits to v4#5841
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a lazy transparent proxy for HttpClient creation, composed configurable dispatcher interceptors at HttpClient construction, and extended types, tests, and fixtures to validate proxy semantics and interceptor behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as EggApplicationCore
participant Proxy as HttpClient Proxy
participant HC as HttpClient
participant Disp as Dispatcher
participant Intercept as Interceptor Chain
participant Net as Network
App->>Proxy: access httpClient (first time)
Proxy->>Proxy: detect property/method access
Proxy->>HC: call createReal() -> createHttpClient()
HC->>Disp: read current dispatcher
HC->>Intercept: Dispatcher.compose(config.interceptors)
HC->>Disp: setDispatcher(composed)
HC-->>Proxy: return proxied HttpClient
App->>Proxy: httpClient.request(opts)
Proxy->>HC: forward request(opts)
HC->>Disp: dispatch(opts, handler)
Disp->>Intercept: run interceptors (inject headers)
Intercept->>Net: perform network request
Net-->>Intercept: response
Intercept-->>HC: response
HC-->>App: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a createTransparentProxy utility to enable lazy initialization of the HttpClient, allowing plugins to modify configurations during lifecycle hooks before the client is instantiated. It also adds support for request interceptors via Dispatcher.compose(). Feedback was provided regarding the use of a module-level variable for state in a test fixture, which could lead to race conditions in concurrent environments.
| let rpcIdCounter = 0; | ||
|
|
||
| exports.httpclient = { | ||
| interceptors: [ | ||
| // Tracer interceptor: injects trace headers into every request | ||
| (dispatch) => { | ||
| return (opts, handler) => { | ||
| opts.headers = opts.headers || {}; | ||
| opts.headers['x-trace-id'] = 'trace-123'; | ||
| rpcIdCounter++; | ||
| opts.headers['x-rpc-id'] = `rpc-${rpcIdCounter}`; |
There was a problem hiding this comment.
Using a module-level variable rpcIdCounter for state can be problematic in a concurrent environment, as it creates shared mutable state. While this might be acceptable for this specific test which runs requests sequentially, it's a pattern that can lead to race conditions in a real application. To promote better practices even in tests, consider managing this state without relying on a shared module-level variable. For example, you could use a factory function that creates an interceptor with its own encapsulated counter.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5841 +/- ##
==========================================
+ Coverage 85.24% 85.29% +0.05%
==========================================
Files 666 666
Lines 13171 13204 +33
Branches 1522 1529 +7
==========================================
+ Hits 11227 11262 +35
+ Misses 1813 1811 -2
Partials 131 131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/egg/test/lib/core/utils.test.ts (1)
7-7: Minor inconsistency: describe name references.jsbut file is.ts.The describe block name references
utils.test.jsbut the actual file isutils.test.ts.♻️ Suggested fix
-describe('test/lib/core/utils.test.js', () => { +describe('test/lib/core/utils.test.ts', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/lib/core/utils.test.ts` at line 7, The describe block string is inconsistent with the TypeScript file name; update the test suite title used in the describe(...) invocation (currently "test/lib/core/utils.test.js") to reference the correct file name "test/lib/core/utils.test.ts" so the description matches the actual file and test conventions.packages/egg/test/lib/core/httpclient_interceptor.test.ts (2)
21-26: Consider promisifying server close for reliable cleanup.
http.Server.close()is callback-based and doesn't return a Promise. In rare cases, the server might not fully close beforeapp.close()runs or before the next test suite starts.♻️ Suggested improvement
afterAll(async () => { if (serverInfo?.server?.listening) { - serverInfo.server.close(); + await new Promise<void>(resolve => serverInfo.server.close(() => resolve())); } await app.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/lib/core/httpclient_interceptor.test.ts` around lines 21 - 26, The afterAll cleanup uses the callback-based serverInfo.server.close() which may not finish before awaiting app.close(); wrap serverInfo.server.close() in a Promise (or use util.promisify) and await it before calling await app.close() to ensure the HTTP server is fully closed; update the afterAll block to await the promised server close (referencing serverInfo.server.close and app.close) so teardown is deterministic.
66-71: Same server close pattern - apply consistent fix.Same callback-based close pattern as the first describe block.
♻️ Suggested improvement
afterAll(async () => { if (serverInfo?.server?.listening) { - serverInfo.server.close(); + await new Promise<void>(resolve => serverInfo.server.close(() => resolve())); } await app.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/lib/core/httpclient_interceptor.test.ts` around lines 66 - 71, In the afterAll block, the server is closed via the callback-based serverInfo.server.close(); make this consistent with the first describe: when serverInfo?.server?.listening, await closing the server by wrapping serverInfo.server.close in a Promise (e.g., await new Promise(resolve => serverInfo.server.close(() => resolve()))), then await app.close(); update the afterAll that contains serverInfo.server.close to use this same Promise-wrapped await pattern so the server fully closes before app.close() runs.packages/egg/test/lib/core/httpclient_proxy.test.ts (2)
21-28: Consider promisifying server close for reliable cleanup.Same pattern as other test files -
http.Server.close()is callback-based. Also,afterEach(mm.restore)is a good practice for ensuring mock cleanup.♻️ Suggested improvement
afterAll(async () => { if (serverInfo?.server?.listening) { - serverInfo.server.close(); + await new Promise<void>(resolve => serverInfo.server.close(() => resolve())); } await app.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/lib/core/httpclient_proxy.test.ts` around lines 21 - 28, The test teardown should await the server close to ensure reliable cleanup: replace the callback-based call to serverInfo.server.close() in the afterAll block with a promisified wait (e.g. await a Promise that resolves when serverInfo.server.close's callback runs) so the process waits for the server to actually close; keep the afterEach(mm.restore) mock restore as-is to ensure mocks are cleaned up.
80-117: Redundantmm.restore()call inside test.Line 95 calls
mm.restore()manually, butafterEach(mm.restore)on line 28 already handles cleanup. The manual call is harmless but redundant.♻️ Optional: Remove redundant restore
If the intent is to test behavior after restore within the same test, keep it. Otherwise, this can be removed since
afterEachhandles cleanup:assert.deepEqual(res.data, { mocked: true }); - // Restore - mm.restore(); - // After restore, should use real HttpClient againHowever, if you need to verify post-restore behavior in the same test (as the current code does), keep the manual call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/lib/core/httpclient_proxy.test.ts` around lines 80 - 117, The test "should support mm() mock on httpClient (egg-mock compatibility)" contains a redundant manual mm.restore() call (mm.restore) because afterEach(mm.restore) already cleans up mocks; remove the explicit mm.restore() in that test unless you intentionally want to verify behavior after restore (in which case keep it and add a comment clarifying that intent), referencing the test name and the mm.restore() invocation to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/egg/src/lib/types.ts`:
- Around line 71-88: The interceptors property is typed as
Dispatcher.DispatcherComposeInterceptor which doesn't exist; change its type to
the correct undici type (DispatcherComposeInterceptor) or import and use that
type directly. Update the declaration of interceptors?:
Dispatcher.DispatcherComposeInterceptor[] to use DispatcherComposeInterceptor[]
(or import { DispatcherComposeInterceptor } from 'undici' and use that), or
alternatively declare it as (dispatch: Dispatcher['dispatch']) =>
Dispatcher['dispatch'][] if you prefer inline typing; ensure references to
Dispatcher.DispatcherComposeInterceptor are removed and replaced with
DispatcherComposeInterceptor or the equivalent imported/inline type so
TypeScript compiles.
---
Nitpick comments:
In `@packages/egg/test/lib/core/httpclient_interceptor.test.ts`:
- Around line 21-26: The afterAll cleanup uses the callback-based
serverInfo.server.close() which may not finish before awaiting app.close(); wrap
serverInfo.server.close() in a Promise (or use util.promisify) and await it
before calling await app.close() to ensure the HTTP server is fully closed;
update the afterAll block to await the promised server close (referencing
serverInfo.server.close and app.close) so teardown is deterministic.
- Around line 66-71: In the afterAll block, the server is closed via the
callback-based serverInfo.server.close(); make this consistent with the first
describe: when serverInfo?.server?.listening, await closing the server by
wrapping serverInfo.server.close in a Promise (e.g., await new Promise(resolve
=> serverInfo.server.close(() => resolve()))), then await app.close(); update
the afterAll that contains serverInfo.server.close to use this same
Promise-wrapped await pattern so the server fully closes before app.close()
runs.
In `@packages/egg/test/lib/core/httpclient_proxy.test.ts`:
- Around line 21-28: The test teardown should await the server close to ensure
reliable cleanup: replace the callback-based call to serverInfo.server.close()
in the afterAll block with a promisified wait (e.g. await a Promise that
resolves when serverInfo.server.close's callback runs) so the process waits for
the server to actually close; keep the afterEach(mm.restore) mock restore as-is
to ensure mocks are cleaned up.
- Around line 80-117: The test "should support mm() mock on httpClient (egg-mock
compatibility)" contains a redundant manual mm.restore() call (mm.restore)
because afterEach(mm.restore) already cleans up mocks; remove the explicit
mm.restore() in that test unless you intentionally want to verify behavior after
restore (in which case keep it and add a comment clarifying that intent),
referencing the test name and the mm.restore() invocation to locate the change.
In `@packages/egg/test/lib/core/utils.test.ts`:
- Line 7: The describe block string is inconsistent with the TypeScript file
name; update the test suite title used in the describe(...) invocation
(currently "test/lib/core/utils.test.js") to reference the correct file name
"test/lib/core/utils.test.ts" so the description matches the actual file and
test conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3c59b7c-7705-4ba1-b701-24f2f34b86e1
⛔ Files ignored due to path filters (2)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/egg/src/index.tspackages/egg/src/lib/core/httpclient.tspackages/egg/src/lib/core/utils.tspackages/egg/src/lib/egg.tspackages/egg/src/lib/types.tspackages/egg/test/fixtures/apps/httpclient-interceptor/app/router.jspackages/egg/test/fixtures/apps/httpclient-interceptor/config/config.default.jspackages/egg/test/fixtures/apps/httpclient-interceptor/package.jsonpackages/egg/test/lib/core/httpclient_interceptor.test.tspackages/egg/test/lib/core/httpclient_proxy.test.tspackages/egg/test/lib/core/utils.test.ts
| /** | ||
| * Interceptors for request composition, applied via `Dispatcher.compose()`. | ||
| * Each interceptor receives a `dispatch` function and returns a new `dispatch` function. | ||
| * | ||
| * @example | ||
| * ```ts | ||
| * // config.default.ts | ||
| * config.httpclient = { | ||
| * interceptors: [ | ||
| * (dispatch) => (opts, handler) => { | ||
| * opts.headers = { ...opts.headers, 'x-trace-id': generateTraceId() }; | ||
| * return dispatch(opts, handler); | ||
| * }, | ||
| * ], | ||
| * }; | ||
| * ``` | ||
| */ | ||
| interceptors?: Dispatcher.DispatcherComposeInterceptor[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
urllib npm package Dispatcher.DispatcherComposeInterceptor type
💡 Result:
No such type "Dispatcher.DispatcherComposeInterceptor" exists in the urllib npm package or its underlying undici dependency. Urllib (v4.9.0, latest as of 2025-12-13) is a high-level HTTP client built on undici that exports undici's Dispatcher types like Agent, ProxyAgent, MockAgent, but does not define custom types like DispatcherComposeInterceptor. Undici (v7.1.1 dependency) has Dispatcher.compose method which accepts interceptors of type DispatcherComposeInterceptor (in newer versions) or DispatchInterceptor (older deprecated versions). This type is: type DispatcherComposeInterceptor = (dispatch: Dispatcher['dispatch']) => Dispatcher['dispatch']; It takes the base dispatch function and returns a new dispatch-like function for chaining interceptors (e.g., retry, redirect). Example usage from undici docs: const { Agent, interceptors } = require('undici'); const agent = new Agent.compose( interceptors.retry, interceptors.redirect ); Urllib usage example with custom dispatcher: import { ProxyAgent, request } from 'urllib'; const proxyAgent = new ProxyAgent('http://proxy:8080'); const response = await request('https://example.com', { dispatcher: proxyAgent }); To use compose interceptors in urllib, pass a composed undici Dispatcher via options.dispatcher.
Citations:
- 1: https://www.npmjs.com/package/urllib
- 2: [TS] Signature method of Interceptor using Dispatcher#compose nodejs/undici#2982
- 3: https://github.com/node-modules/urllib
- 4: https://undici-docs.vramana.dev/docs/api/Dispatcher
- 5: https://github.com/nodejs/undici/blob/main/docs/docs/api/Dispatcher.md
- 6: https://undici-docs.vramana.dev/docs/api/DispatchInterceptor
- 7: https://tessl.io/registry/tessl/npm-undici/7.15.0/files/docs/interceptors.md
🏁 Script executed:
cat -n packages/egg/src/lib/types.ts | head -100Repository: eggjs/egg
Length of output: 4100
🏁 Script executed:
rg "Dispatcher" packages/egg/src/lib/types.ts -B 5 -A 5Repository: eggjs/egg
Length of output: 1194
🏁 Script executed:
rg "import.*Dispatcher" packages/egg/src/lib/ -t tsRepository: eggjs/egg
Length of output: 124
🏁 Script executed:
cat packages/egg/package.json | grep -A 5 -B 5 "urllib\|undici"Repository: eggjs/egg
Length of output: 1058
🏁 Script executed:
rg "DispatcherComposeInterceptor" packages/ -t tsRepository: eggjs/egg
Length of output: 142
🏁 Script executed:
find packages -name "*.d.ts" -type f | xargs rg "DispatcherComposeInterceptor" 2>/dev/null || echo "No .d.ts files found with this type"Repository: eggjs/egg
Length of output: 88
🏁 Script executed:
# Check urllib package.json to see what version is used
rg "urllib" packages/egg/package.json -A 2 -B 2Repository: eggjs/egg
Length of output: 514
🏁 Script executed:
# Check the root pnpm-workspace.yaml or package.json for version catalog
cat pnpm-workspace.yaml 2>/dev/null || cat package.json | grep -A 50 "catalog"Repository: eggjs/egg
Length of output: 5462
🏁 Script executed:
# Look for urllib version in root catalog
rg "urllib.*:" . --type yaml --type json | head -20Repository: eggjs/egg
Length of output: 859
🏁 Script executed:
# Check if there's a monorepo-level package.json that defines catalog versions
cat package.json | jq '.catalog' 2>/dev/null || grep -A 100 '"catalog"' package.json | head -50Repository: eggjs/egg
Length of output: 57
Type reference Dispatcher.DispatcherComposeInterceptor does not exist in urllib.
The type Dispatcher.DispatcherComposeInterceptor is not exported from urllib v4.8.2 or its underlying undici dependency. The actual type from undici is DispatcherComposeInterceptor (without the Dispatcher namespace), defined as (dispatch: Dispatcher['dispatch']) => Dispatcher['dispatch']. This code will fail TypeScript compilation. The type annotation should be corrected to reference the proper type from undici, or import DispatcherComposeInterceptor directly rather than accessing it as a namespace member.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/src/lib/types.ts` around lines 71 - 88, The interceptors
property is typed as Dispatcher.DispatcherComposeInterceptor which doesn't
exist; change its type to the correct undici type (DispatcherComposeInterceptor)
or import and use that type directly. Update the declaration of interceptors?:
Dispatcher.DispatcherComposeInterceptor[] to use DispatcherComposeInterceptor[]
(or import { DispatcherComposeInterceptor } from 'undici' and use that), or
alternatively declare it as (dispatch: Dispatcher['dispatch']) =>
Dispatcher['dispatch'][] if you prefer inline typing; ensure references to
Dispatcher.DispatcherComposeInterceptor are removed and replaced with
DispatcherComposeInterceptor or the equivalent imported/inline type so
TypeScript compiles.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
2 participants
PR Description
This PR ports the complete custom DNS lookup and httpclient proxy/interceptor module design from 3.x to the next branch.
Key changes:
Implements createTransparentProxy utility for lazy, mm()-compatible httpClient instantiation.
Refactors app.httpClient to use the proxy, enabling plugins to modify config before real client creation.
Adds support for config.httpclient.interceptors (undici dispatcher interceptors) and applies them in httpclient constructor.
Provides full unit tests for proxy and interceptor logic.
Ensures all changes are TypeScript, ESM, and monorepo compatible.
All tests pass except for known unrelated flaky cases.
Summary by CodeRabbit
New Features
Tests