feat: enable custom lookup #5751
Conversation
📝 WalkthroughWalkthroughThis PR adds support for custom DNS lookup functions in HttpClient configuration. It introduces a LookupFunction type to HttpClientConfig, updates the HttpClient constructor to safely handle missing config and inherit lookup settings, replaces legacy DNS caching configuration with a custom lookup implementation in test fixtures, and adds comprehensive tests with a new local server utility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @Dipper30, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the HTTP client by enabling custom DNS lookup functionality. This allows for greater control over how hostnames are resolved, which is particularly beneficial for advanced network configurations, testing environments, and local development. The changes involve modifying the HTTP client's configuration handling, updating relevant type definitions, and a comprehensive refactoring of the associated test suite to validate the new feature's behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5751 +/- ##
==========================================
+ Coverage 87.40% 87.60% +0.20%
==========================================
Files 561 563 +2
Lines 10932 10931 -1
Branches 1238 1241 +3
==========================================
+ Hits 9555 9576 +21
+ Misses 1293 1271 -22
Partials 84 84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 enables the use of a custom lookup function in httpclient, which is a great move towards using standard Node.js features and simplifying the codebase by removing the previous custom DNS caching logic. The implementation is straightforward and the related type definitions and test cases have been updated accordingly.
I've found a couple of minor issues in the test files: a duplicated test case and a potential race condition in a new test utility. Please see my comments for details.
| it('should safeCurl also work', async () => { | ||
| // will resolve custom-localhost to 127.0.0.1 | ||
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | ||
| assert(res.status === 200); | ||
| }); |
There was a problem hiding this comment.
This test case is a duplicate of the one from lines 38-42. It should be removed to avoid redundancy and keep the test suite clean.
| export async function startNewLocalServer(ip = '127.0.0.1'): Promise<{ | ||
| url: string; | ||
| server: http.Server; | ||
| }> { | ||
| let localServer: http.Server; | ||
| return new Promise((resolve, reject) => { | ||
| const app = new Koa(); | ||
| app.use(async (ctx) => { | ||
| if (ctx.path === '/get_headers') { | ||
| ctx.body = { | ||
| headers: ctx.request.headers, | ||
| host: ctx.request.headers.host, | ||
| }; | ||
| return; | ||
| } | ||
| ctx.body = JSON.stringify(`${ctx.method} ${ctx.path}`); | ||
| }); | ||
| localServer = http.createServer(app.callback()); | ||
| const serverCallback = () => { | ||
| const addressRes = localServer.address(); | ||
| const port = addressRes && typeof addressRes === 'object' ? addressRes.port : addressRes; | ||
| const url = `http://${ip}:` + port; | ||
| return resolve({ url, server: localServer }); | ||
| }; | ||
| localServer.listen(0, serverCallback); | ||
| localServer.on('error', (e: any) => { | ||
| if (e.code === 'EADDRINUSE') { | ||
| setTimeout(() => { | ||
| localServer.close(); | ||
| localServer.listen(0, serverCallback); | ||
| }, 1000); | ||
| } else { | ||
| reject(e); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The EADDRINUSE error handling in startNewLocalServer contains a race condition. localServer.close() is asynchronous, but it's not awaited before localServer.listen() is called again inside a setTimeout.
Additionally, since listen(0) is used to pick a random available port, an EADDRINUSE error is highly unlikely.
I suggest refactoring this function to use async/await and simplifying the error handling, similar to the existing startLocalServer function in this file. This makes the code cleaner and avoids the potential race condition.
export async function startNewLocalServer(ip = '127.0.0.1'): Promise<{
url: string;
server: http.Server;
}> {
const app = new Koa();
app.use(async ctx => {
if (ctx.path === '/get_headers') {
ctx.body = {
headers: ctx.request.headers,
host: ctx.request.headers.host,
};
return;
}
ctx.body = JSON.stringify(`${ctx.method} ${ctx.path}`);
});
const localServer = http.createServer(app.callback());
localServer.listen(0);
await once(localServer, 'listening');
const address = localServer.address() as AddressInfo;
const url = `http://${ip}:${address.port}`;
return { url, server: localServer };
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/egg/src/lib/types.ts (1)
69-69: Consider adding JSDoc documentation for the newlookupproperty.Other properties in
HttpClientConfighave comments. For consistency and discoverability, consider documenting this option.🔎 Suggested documentation
allowH2?: boolean; + /** + * Custom DNS lookup function for resolving hostnames + */ lookup?: LookupFunction;packages/egg/test/utils.ts (1)
157-173: Consider consolidating with existingstartLocalServerto reduce duplication.There's already a
startLocalServerfunction (lines 80-125) with similar functionality. The main differences are:
startNewLocalServerreturns the server instance for caller-managed cleanup/get_headersreturns different response formatConsider whether these could be unified or if the differences warrant a clearer naming distinction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/egg/src/lib/core/httpclient.tspackages/egg/src/lib/types.tspackages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.jspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/test/utils.ts
🧰 Additional context used
📓 Path-based instructions (10)
packages/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/*.ts: Use TypeScript throughout all packages in the Eggjs monorepo
Use strict TypeScript mode in all packages
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/src/lib/types.tspackages/egg/test/utils.ts
packages/egg/src/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Work primarily in 'packages/egg/src/' for core framework features, organizing into lib/, app/extend/, config/, and lib/loader/ directories
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/src/lib/types.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use
.tssources over CommonJS and prefer ESM for exportsUse ESM (ES Modules) syntax with 'import' and 'export' statements - add 'type: module' to package.json for CommonJS to ESM migration
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.jspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/src/lib/types.tspackages/egg/test/utils.ts
{packages,plugins}/**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Name files in lowercase with hyphens (e.g.
loader-context.ts)
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.jspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/src/lib/types.tspackages/egg/test/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Name classes in PascalCase
Name functions and variables in camelCase
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/src/lib/types.tspackages/egg/test/utils.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: All exported functions and methods must have explicit return type annotations to support TypeScript's isolatedDeclarations flag
Avoid using computed property names with symbols in class declarations - use override methods instead for custom property definitions
Use explicit type annotations for class properties and add 'unique symbol' type for exported symbols to support isolatedDeclarations
Enable strict mode in all TypeScript packages and use explicit return types for public APIs
Prefer interfaces over type aliases for object shapes in TypeScript
Use readonly modifiers where appropriate in TypeScript class properties and interface definitions
Avoid using 'any' type in TypeScript - use 'unknown' when type is truly unknown
Use BaseContextClass as the base class for context-aware components like services and subscriptions in Egg applications
Monitor memory usage and implement proper cleanup in lifecycle hooks to avoid memory leaks in long-running processes
Avoid global state in worker processes - use application context or request context for state management
Files:
packages/egg/src/lib/core/httpclient.tspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/src/lib/types.tspackages/egg/test/utils.ts
packages/egg/test/fixtures/apps/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create test fixtures in 'packages/egg/test/fixtures/apps/' for scenario testing
Files:
packages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.js
**/test/fixtures/**
📄 CodeRabbit inference engine (AGENTS.md)
Place reusable test data under
test/fixtures/
Files:
packages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.js
packages/**/test/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/**/test/**/*.test.ts: Use Vitest as the standard test runner for all packages; test files follow pattern 'test/**/*.test.ts'
Use 'import { describe, it } from "vitest"' for test functions in Vitest test files
Use Node.js built-in 'assert' module for assertions in tests
Files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Configure Vitest to discover
**/test/**/*.test.tswithin each package and mirror this pattern when adding test suites
**/test/**/*.test.ts: Test files in Vitest-based packages must follow the naming pattern 'test/**/*.test.ts' and import test functions from vitest
Configure test files to use Node.js built-in 'assert' module for assertions in Vitest tests
Import test functions (describe, it, expect, beforeEach, afterEach) from 'vitest' package in test files
Files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
🧠 Learnings (13)
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to **/test/**/*.test.ts : Import test functions (describe, it, expect, beforeEach, afterEach) from 'vitest' package in test files
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-11-27T14:37:59.711Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T14:37:59.711Z
Learning: Applies to packages/**/test/**/*.test.ts : Use 'import { describe, it } from "vitest"' for test functions in Vitest test files
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to **/test/**/*.test.ts : Configure test files to use Node.js built-in 'assert' module for assertions in Vitest tests
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-11-27T14:37:59.711Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T14:37:59.711Z
Learning: Applies to packages/**/test/**/*.test.ts : Use Node.js built-in 'assert' module for assertions in tests
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-20T09:24:59.960Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-20T09:24:59.960Z
Learning: Add regression cases that exercise both CommonJS and TypeScript example apps when features affect HTTP or process orchestration
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to **/test/**/*.test.ts : Test files in Vitest-based packages must follow the naming pattern 'test/**/*.test.ts' and import test functions from vitest
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to **vitest.config.ts : All plugin Vitest configurations should use a vitest.config.ts file for test setup and configuration
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: All new packages MUST use Vitest for testing instead of other test runners. Exception: egg-bin and cookies packages use Mocha for consistency with their testing patterns.
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T03:05:13.313Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: tegg/CLAUDE.md:0-0
Timestamp: 2025-12-21T03:05:13.313Z
Learning: Applies to tegg/**/__tests__/**/*.test.{ts,tsx,js} : Use Vitest for testing all tegg packages
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-11-27T14:37:59.711Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T14:37:59.711Z
Learning: Applies to packages/**/test/**/*.test.ts : Use Vitest as the standard test runner for all packages; test files follow pattern 'test/**/*.test.ts'
Applied to files:
packages/egg/test/lib/core/dnscache_httpclient.test.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to plugins/*/src/types.ts : All module augmentation in plugin types.ts files must use 'declare module "egg"' syntax to extend Egg interfaces
Applied to files:
packages/egg/src/lib/types.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to plugins/*/src/types.ts : All Egg framework plugins MUST define a 'src/types.ts' file that extends the Egg module declarations using 'declare module "egg"' and defines typed configuration
Applied to files:
packages/egg/src/lib/types.ts
📚 Learning: 2025-12-21T14:11:29.307Z
Learnt from: CR
Repo: eggjs/egg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T14:11:29.307Z
Learning: Applies to packages/egg/src/lib/start.ts : Implement graceful shutdown in application startup to support zero-downtime deployments
Applied to files:
packages/egg/test/utils.ts
🧬 Code graph analysis (3)
packages/egg/src/lib/core/httpclient.ts (6)
packages/core/src/egg.ts (1)
config(315-317)packages/core/src/loader/egg_loader.ts (2)
config(1027-1031)app(157-159)packages/egg/src/lib/egg.ts (1)
config(562-564)packages/egg/src/lib/core/base_hook_class.ts (2)
config(20-22)app(24-27)packages/egg/test/utils.ts (1)
app(31-36)packages/core/src/lifecycle.ts (1)
app(128-130)
packages/egg/test/lib/core/dnscache_httpclient.test.ts (1)
packages/egg/test/utils.ts (4)
app(31-36)MockApplication(24-24)createApp(38-38)startNewLocalServer(157-193)
packages/egg/test/utils.ts (4)
packages/egg/src/app/extend/context.ts (2)
ip(283-285)ip(287-289)packages/egg/src/app/extend/request.ts (2)
ip(139-148)ip(161-163)plugins/view/src/lib/view_manager.ts (1)
resolve(101-115)packages/core/src/loader/egg_loader.ts (1)
app(157-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: Test (macos-latest, 20, 3/3)
- GitHub Check: Test (ubuntu-latest, 24, 1/3)
- GitHub Check: Test (ubuntu-latest, 20, 1/3)
- GitHub Check: Test (ubuntu-latest, 24, 3/3)
- GitHub Check: Test (windows-latest, 20, 2/3)
- GitHub Check: Test (windows-latest, 24, 3/3)
- GitHub Check: Test (macos-latest, 22, 3/3)
- GitHub Check: Test (ubuntu-latest, 20, 3/3)
- GitHub Check: Test (ubuntu-latest, 22, 1/3)
- GitHub Check: Test (macos-latest, 22, 2/3)
- GitHub Check: Test (ubuntu-latest, 22, 2/3)
- GitHub Check: Test bin (ubuntu-latest, 24, 0/3)
- GitHub Check: Test bin (windows-latest, 24, 2/3)
- GitHub Check: Test bin (windows-latest, 24, 0/3)
- GitHub Check: Test bin (ubuntu-latest, 24, 2/3)
- GitHub Check: Test bin (ubuntu-latest, 24, 1/3)
- GitHub Check: Test bin (windows-latest, 24, 1/3)
- GitHub Check: typecheck
🔇 Additional comments (3)
packages/egg/src/lib/types.ts (1)
1-1: LGTM!The import of
LookupFunctionfromnode:netis the correct standard Node.js type for custom DNS resolution callbacks.packages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.js (1)
4-21: LGTM - appropriate custom lookup implementation for testing.The lookup function correctly implements the Node.js
dns.lookupcallback signature, handling both single andallresult modes. Hardcoding resolution to127.0.0.1for non-IP hostnames is suitable for this test fixture.packages/egg/test/lib/core/dnscache_httpclient.test.ts (1)
1-6: LGTM on imports and test structure.The test correctly uses Vitest imports (
describe,it,beforeAll,afterAll) and Node.js built-inassertmodule as per the coding guidelines.
| constructor(app: EggApplicationCore, options: HttpClientOptions = {}) { | ||
| normalizeConfig(app); | ||
| const config = app.config.httpclient; | ||
| const config = app.config.httpclient || {}; | ||
| options.lookup = options.lookup ?? config.lookup; |
There was a problem hiding this comment.
Potential runtime error in normalizeConfig when httpclient config is undefined.
The fallback to {} on line 28 is good, but normalizeConfig(app) is called before this assignment on line 27. Inside normalizeConfig (lines 62-66), app.config.httpclient is accessed directly without a fallback, which will cause a TypeError when accessing config.request?.timeout if config is undefined.
🔎 Proposed fix
function normalizeConfig(app: EggApplicationCore) {
- const config = app.config.httpclient;
+ const config = app.config.httpclient ?? {};
if (typeof config.request?.timeout === 'string') {
config.request.timeout = ms(config.request.timeout);
}
}📝 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.
| constructor(app: EggApplicationCore, options: HttpClientOptions = {}) { | |
| normalizeConfig(app); | |
| const config = app.config.httpclient; | |
| const config = app.config.httpclient || {}; | |
| options.lookup = options.lookup ?? config.lookup; | |
| function normalizeConfig(app: EggApplicationCore) { | |
| const config = app.config.httpclient ?? {}; | |
| if (typeof config.request?.timeout === 'string') { | |
| config.request.timeout = ms(config.request.timeout); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/egg/src/lib/core/httpclient.ts around lines 26-29,
normalizeConfig(app) is called before ensuring app.config.httpclient is defined,
causing a TypeError when normalizeConfig reads config.request?.timeout; either
move the const config = app.config.httpclient || {} and options.lookup
assignment above the normalizeConfig(app) call so normalizeConfig sees a defined
config, or modify normalizeConfig to accept a config parameter or use safe
guards (optional chaining / default {}) when reading app.config.httpclient;
implement one of these fixes and remove the unsafe direct access to
app.config.httpclient inside normalizeConfig.
| afterAll(() => { | ||
| if (serverInfo?.server?.listening) { | ||
| serverInfo.server.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing app.close() in afterAll cleanup.
The application instance should be properly closed to release resources and avoid potential test interference. Based on coding guidelines, proper cleanup in lifecycle hooks is important to avoid memory leaks.
🔎 Proposed fix
afterAll(() => {
if (serverInfo?.server?.listening) {
serverInfo.server.close();
}
+ return app.close();
});📝 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.
| afterAll(() => { | |
| if (serverInfo?.server?.listening) { | |
| serverInfo.server.close(); | |
| } | |
| }); | |
| afterAll(() => { | |
| if (serverInfo?.server?.listening) { | |
| serverInfo.server.close(); | |
| } | |
| return app.close(); | |
| }); |
🤖 Prompt for AI Agents
In packages/egg/test/lib/core/dnscache_httpclient.test.ts around lines 20 to 24,
the afterAll cleanup closes the server but does not close the application
instance; update the afterAll hook to also close the app by checking for the app
variable (e.g., if (app && app.close)) and calling/awaiting app.close() so the
application is properly shut down and resources released after tests.
| it('should safeCurl also work', async () => { | ||
| // will resolve custom-localhost to 127.0.0.1 | ||
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | ||
| assert(res.status === 200); | ||
| }); |
There was a problem hiding this comment.
Duplicate test case - remove or rename.
This test (lines 44-48) is an exact duplicate of the test at lines 38-42. Both are named "should safeCurl also work" with identical implementations.
🔎 Proposed fix - remove duplicate
it('should safeCurl also work', async () => {
// will resolve custom-localhost to 127.0.0.1
const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' });
assert(res.status === 200);
});
-
- it('should safeCurl also work', async () => {
- // will resolve custom-localhost to 127.0.0.1
- const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' });
- assert(res.status === 200);
- });📝 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.
| it('should safeCurl also work', async () => { | |
| // will resolve custom-localhost to 127.0.0.1 | |
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | |
| assert(res.status === 200); | |
| }); | |
| it('should safeCurl also work', async () => { | |
| // will resolve custom-localhost to 127.0.0.1 | |
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | |
| assert(res.status === 200); | |
| }); |
🤖 Prompt for AI Agents
In packages/egg/test/lib/core/dnscache_httpclient.test.ts around lines 44-48,
there is an exact duplicate test of the one at lines 38-42 (both named "should
safeCurl also work"); remove the duplicate test or rename it if a distinct
behavior is intended — delete the redundant it(...) block at 44-48 so only one
test with that name and implementation remains, or change the duplicate's
description and implementation to cover the different case you actually want to
test.
| it('should server down and catch error', async () => { | ||
| try { | ||
| if (serverInfo?.server?.listening) await serverInfo.server.close(); | ||
| // will resolve to 127.0.0.1 | ||
| const res = await app.curl(url + '/get_headers', { dataType: 'json' }); | ||
| assert(res.status !== 200); | ||
| } catch (err: any) { | ||
| assert(err); | ||
| assert(err.message.includes('ECONNREFUSED')); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test logic issues in "server down" test.
server.close()uses callback-style API;awaitdoesn't wait for closure to complete- The try-catch logic is unclear: if the request succeeds, it asserts
status !== 200, which would fail for an actual successful response
🔎 Proposed fix
it('should server down and catch error', async () => {
+ const { promisify } = await import('node:util');
try {
- if (serverInfo?.server?.listening) await serverInfo.server.close();
+ if (serverInfo?.server?.listening) {
+ await promisify(serverInfo.server.close.bind(serverInfo.server))();
+ }
// will resolve to 127.0.0.1
const res = await app.curl(url + '/get_headers', { dataType: 'json' });
- assert(res.status !== 200);
+ assert.fail('Expected request to fail after server shutdown');
} catch (err: any) {
assert(err);
assert(err.message.includes('ECONNREFUSED'));
}
});📝 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.
| it('should server down and catch error', async () => { | |
| try { | |
| if (serverInfo?.server?.listening) await serverInfo.server.close(); | |
| // will resolve to 127.0.0.1 | |
| const res = await app.curl(url + '/get_headers', { dataType: 'json' }); | |
| assert(res.status !== 200); | |
| } catch (err: any) { | |
| assert(err); | |
| assert(err.message.includes('ECONNREFUSED')); | |
| } | |
| }); | |
| it('should server down and catch error', async () => { | |
| const { promisify } = await import('node:util'); | |
| try { | |
| if (serverInfo?.server?.listening) { | |
| await promisify(serverInfo.server.close.bind(serverInfo.server))(); | |
| } | |
| // will resolve to 127.0.0.1 | |
| const res = await app.curl(url + '/get_headers', { dataType: 'json' }); | |
| assert.fail('Expected request to fail after server shutdown'); | |
| } catch (err: any) { | |
| assert(err); | |
| assert(err.message.includes('ECONNREFUSED')); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/egg/test/lib/core/dnscache_httpclient.test.ts around lines 50-60,
the "server down" test incorrectly awaits a callback-style server.close() and
allows a successful HTTP response path that asserts status !== 200; instead,
first properly wait for server shutdown by wrapping serverInfo.server.close in a
Promise (or use server.closeSync if available) so closure completes before
making the request, then call app.curl expecting it to throw; remove the flaky
assert(res.status !== 200) and instead assert failure if the request does not
throw (e.g., throw a test error after the call), and in the catch assert the
error exists and its message includes 'ECONNREFUSED'.
| localServer.on('error', (e: any) => { | ||
| if (e.code === 'EADDRINUSE') { | ||
| setTimeout(() => { | ||
| localServer.close(); | ||
| localServer.listen(0, serverCallback); | ||
| }, 1000); | ||
| } else { | ||
| reject(e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
EADDRINUSE retry logic is flawed - closed server cannot be restarted.
After localServer.close() is called, the same server instance cannot be used to listen() again. A new server instance needs to be created for a retry attempt. Additionally, with port 0 (ephemeral port), EADDRINUSE is extremely unlikely since the OS assigns an available port.
🔎 Proposed fix - simplify by removing unlikely retry logic
localServer.listen(0, serverCallback);
- localServer.on('error', (e: any) => {
- if (e.code === 'EADDRINUSE') {
- setTimeout(() => {
- localServer.close();
- localServer.listen(0, serverCallback);
- }, 1000);
- } else {
- reject(e);
- }
- });
+ localServer.on('error', reject);📝 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.
| localServer.on('error', (e: any) => { | |
| if (e.code === 'EADDRINUSE') { | |
| setTimeout(() => { | |
| localServer.close(); | |
| localServer.listen(0, serverCallback); | |
| }, 1000); | |
| } else { | |
| reject(e); | |
| } | |
| }); | |
| localServer.listen(0, serverCallback); | |
| localServer.on('error', reject); |
🤖 Prompt for AI Agents
In packages/egg/test/utils.ts around lines 182 to 191, the EADDRINUSE retry
logic is incorrect because closing the existing server and attempting to reuse
it with listen() is invalid and, for port 0, unnecessary; remove the
special-case retry branch and treat any server 'error' by rejecting the promise
(or, if desired, create a brand-new server instance before retrying), so delete
the setTimeout/close/listen block and simply call reject(e) for all errors.
There was a problem hiding this comment.
Pull request overview
This PR adds support for custom DNS lookup functions in the Eggjs HTTP client, allowing applications to provide their own hostname resolution logic instead of relying solely on the default DNS lookup mechanism.
- Adds a
lookupproperty to theHttpClientConfigtype definition - Integrates the custom lookup function into the HttpClient constructor
- Replaces skipped DNS cache tests with new tests that verify custom lookup functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/egg/src/lib/types.ts | Imports LookupFunction from Node.js net module and adds it as an optional property to HttpClientConfig |
| packages/egg/src/lib/core/httpclient.ts | Updates HttpClient constructor to merge custom lookup from config into options before passing to parent class |
| packages/egg/test/utils.ts | Adds startNewLocalServer utility function to create a local test HTTP server with custom endpoints |
| packages/egg/test/lib/core/dnscache_httpclient.test.ts | Replaces previously skipped tests with new tests that verify custom DNS lookup functionality using a mock resolver |
| packages/egg/test/fixtures/apps/dnscache_httpclient/config/config.default.js | Configures a custom lookup function that resolves all non-IP hostnames to 127.0.0.1 for testing purposes |
| it('should safeCurl also work', async () => { | ||
| // will resolve custom-localhost to 127.0.0.1 | ||
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | ||
| assert(res.status === 200); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Duplicate test case detected. Both test cases on lines 38-42 and 44-48 have identical descriptions "should safeCurl also work" and identical test logic. This creates confusion and is redundant. Consider removing one of these duplicate tests or differentiating them if they're intended to test different scenarios.
| it('should safeCurl also work', async () => { | |
| // will resolve custom-localhost to 127.0.0.1 | |
| const res = await app.safeCurl(url + '/get_headers', { dataType: 'json' }); | |
| assert(res.status === 200); | |
| }); |
| try { | ||
| if (serverInfo?.server?.listening) await serverInfo.server.close(); | ||
| // will resolve to 127.0.0.1 | ||
| const res = await app.curl(url + '/get_headers', { dataType: 'json' }); | ||
| assert(res.status !== 200); | ||
| } catch (err: any) { | ||
| assert(err); | ||
| assert(err.message.includes('ECONNREFUSED')); | ||
| } |
There was a problem hiding this comment.
The test logic appears flawed. After closing the server on line 52, the test expects a non-200 status or catches an ECONNREFUSED error. However, the assertion assert(res.status !== 200) on line 55 will never be reached if the connection is refused, as the curl call will throw an error immediately. The try-catch block makes the status check unreachable. Consider restructuring this test to only check for the expected error, or use a pattern that properly handles both success and error cases.
| try { | |
| if (serverInfo?.server?.listening) await serverInfo.server.close(); | |
| // will resolve to 127.0.0.1 | |
| const res = await app.curl(url + '/get_headers', { dataType: 'json' }); | |
| assert(res.status !== 200); | |
| } catch (err: any) { | |
| assert(err); | |
| assert(err.message.includes('ECONNREFUSED')); | |
| } | |
| if (serverInfo?.server?.listening) await serverInfo.server.close(); | |
| // will resolve to 127.0.0.1 but should fail because server is down | |
| await assert.rejects( | |
| () => app.curl(url + '/get_headers', { dataType: 'json' }), | |
| (err: any) => { | |
| assert(err); | |
| assert(err.message.includes('ECONNREFUSED')); | |
| return true; | |
| }, | |
| ); |
| server: http.Server; | ||
| }> { | ||
| let localServer: http.Server; | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
The return type Promise should have a more specific reject type. The Promise is initialized on line 162 but the error handling on lines 182-191 can reject with various error types (line 189 rejects with 'e' of type 'any'). Consider using a more specific error type or at least Error instead of the implicit unknown rejection type.
| return resolve({ url, server: localServer }); | ||
| }; | ||
| localServer.listen(0, serverCallback); | ||
| localServer.on('error', (e: any) => { |
There was a problem hiding this comment.
The error parameter has type 'any' which bypasses TypeScript's type safety. Consider using a more specific type such as NodeJS.ErrnoException or at minimum Error, as the code checks for e.code === 'EADDRINUSE' which is a property of NodeJS.ErrnoException.
| localServer.on('error', (e: any) => { | |
| localServer.on('error', (e: NodeJS.ErrnoException) => { |
2 participants
pick from #5749
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.