feat: support file returning async functions when loading it #272
Conversation
WalkthroughThe pull request introduces enhancements to file loading mechanisms, focusing on improving support for asynchronous modules and promises. The changes primarily involve updating the Changes
Sequence DiagramsequenceDiagram
participant Loader as EggLoader
participant Module as File Module
participant Promise as Promise Handler
Loader->>Module: Load File
alt Is Promise
Module-->>Promise: Await Promise
Promise-->>Loader: Resolve Module
else Is Regular Module
Module-->>Loader: Return Module
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
test/fixtures/loadfile/es-module-default-async.js (2)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is redundant in ES modules as they are automatically in strict mode.
-"use strict"; exports.__esModule = true;🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
4-12: Consider reducing the timeout duration for faster testsThe 500ms timeout in the test fixture might slow down the test suite unnecessarily. Consider reducing this value to improve test execution time while still demonstrating the async behavior.
fn() { return new Promise((res) => { setTimeout(() => { res({ clients: 'Test Config' }); - }, 500); + }, 10); // Reduced timeout for faster tests }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
test/fixtures/loadfile/es-module-default-async.js(1 hunks)test/loader/load_file.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/loadfile/es-module-default-async.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
package.json (1)
Line range hint
91-111: Well-structured dual module format supportThe exports field configuration is well-structured to support both ESM and CommonJS modules, which is essential for:
- Compatibility with different module systems
- Proper type definitions in both formats
- Source file access for debugging
This setup aligns with the PR's need to support async function loading in both module formats.
Consider documenting the module format support in the README, especially regarding:
- How async functions are handled in both ESM and CommonJS contexts
- Any limitations or considerations when using async configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (2)
package.json (2)
Line range hint 6-8: Verify Node.js version requirement aligns with async function support needs
The upgrade to Node.js >= 18.19.0 is appropriate as it ensures robust support for:
- ESM modules
- Top-level await
- Enhanced async/await capabilities
This aligns well with the PR's objective of supporting async function returns in configuration files.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough evidence to verify the Node.js version requirement:
Node.js version requirement is appropriate for async function support
The codebase shows extensive use of async/await functionality and ESM modules:
- Multiple async function declarations in core loaders (
src/loader/file_loader.ts,src/loader/egg_loader.ts) - Support for loading async functions from configuration files (
test/loader/load_file.test.ts) - ESM module handling with top-level await (
src/utils/index.ts)
Node.js 18.19.0+ provides stable support for these features:
- Top-level await in ESM modules
- Enhanced async/await capabilities
- Stable ESM implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Node.js version compatibility with async features
# Check for any potential compatibility issues in the codebase
# Search for top-level await usage
rg "await\s+(?!.*\{)" --type ts
# Search for async function exports
rg "export\s+(?:default\s+)?async\s+function" --type ts
Length of output: 319
Script:
#!/bin/bash
# Let's try a simpler approach to verify async usage and Node.js compatibility
# Search for async function declarations and exports
rg "async function" --type ts -A 2
# Search for top-level await usage (simplified pattern)
rg "await " --type ts -A 2
# Check for any ESM-specific syntax
rg "export |import " --type ts
Length of output: 67804
18-18: Clarify the purpose of the -p flag in test script
The addition of the -p flag to egg-bin test command needs documentation. Please clarify if this is related to:
- Parallel test execution
- Process isolation for async tests
- Other test runner configurations
|
所有测试用例补充 + fix
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
test/fixtures/loadfile-esm/es-module-default-async.js (1)
1-1: Remove redundant "use strict" directiveES modules are automatically in strict mode, making this directive unnecessary.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile-esm/es-module-default-promise.js (1)
1-1: Remove redundant "use strict" directiveES modules are automatically in strict mode, making this directive unnecessary.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/es-module-default-async.js (1)
1-1: Remove redundant "use strict" directiveEven in CommonJS-style modules that use ES module features, the strict mode directive is redundant when __esModule is true.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/es-module-default-null.js (1)
1-1: Remove redundant "use strict" directiveThe "use strict" directive is unnecessary in ES modules as they are automatically in strict mode.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/loader/load_file.test.ts (1)
52-74: Consider adding error handling test casesWhile the happy path tests are comprehensive, consider adding test cases for:
- Rejected promises
- Timeout scenarios
- Invalid async function implementations
Example test cases:
it('should handle rejected promises in ES modules', async () => { app = createApp('load_file'); await assert.rejects(async () => { await app.loader.loadFile(getFilepath('load_file/es-module-default-async-error.js')); }, /Expected error/); }); it('should handle timeout in async functions', async () => { app = createApp('load_file'); await assert.rejects(async () => { await app.loader.loadFile(getFilepath('load_file/es-module-default-async-timeout.js')); }, /Timeout/); });test/utils/index.test.ts (2)
60-68: Enhance promise type checkingThe current check
typeof result().then === 'function'could be improved for more robust promise detection.Consider using:
-assert(typeof result().then === 'function'); +assert(result() instanceof Promise);
123-131: Add value assertion after promise resolutionThe tests verify that a promise is returned but don't verify the resolved value.
Consider adding:
it('should resolve with correct value from async function', async () => { const result = await utils.loadFile(path.join(baseDir, 'es-module-default-async.js')); const value = await result(); assert.deepEqual(value, { clients: 'Test Config' }); });test/fixtures/load_file/es-module-default.js (1)
3-5: Add documentation for test fixture purposeSince this is a test fixture, adding a brief comment explaining its purpose would improve maintainability.
+// Test fixture for ES module default export loading exports["default"] = { fn() {} };test/fixtures/load_file/async.js (1)
3-5: Consider expanding test data to match real-world scenariosSince this fixture tests async config loading (particularly for MySQL configs as mentioned in PR objectives), consider using more realistic test data.
module.exports = async () => { - return { clients: "Test Config" }; + return { + clients: { + mysql: { + host: 'test-host', + port: 3306, + user: 'test-user', + password: 'test-password', + database: 'test-db' + } + } + }; };test/fixtures/load_file/es-module-default-promise.js (1)
1-1: Remove redundant 'use strict' directiveES modules are automatically in strict mode, making this directive unnecessary.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile/es-module-default-promise.js (1)
3-5: LGTM! Consider enhancing test clarityThe implementation correctly demonstrates a Promise-returning function for testing async module loading. However, to make the test case more explicit about its purpose, consider adding a small delay to better simulate real-world async scenarios.
exports["default"] = function () { - return Promise.resolve({ clients: "Test Config" }); + return new Promise(resolve => { + setTimeout(() => { + resolve({ clients: "Test Config" }); + }, 10); + }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/loader/egg_loader.ts(2 hunks)test/fixtures/load_file/async.js(1 hunks)test/fixtures/load_file/es-module-default-async.js(1 hunks)test/fixtures/load_file/es-module-default-null.js(1 hunks)test/fixtures/load_file/es-module-default-promise.js(1 hunks)test/fixtures/load_file/es-module-default.js(1 hunks)test/fixtures/load_file/promise_function.js(1 hunks)test/fixtures/loadfile-esm/es-module-default-async.js(1 hunks)test/fixtures/loadfile-esm/es-module-default-promise.js(1 hunks)test/fixtures/loadfile/es-module-default-async.js(1 hunks)test/fixtures/loadfile/es-module-default-promise.js(1 hunks)test/loader/load_file.test.ts(1 hunks)test/utils/index.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/load_file/promise_function.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/loader/egg_loader.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/fixtures/load_file/es-module-default-promise.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/es-module-default-async.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile-esm/es-module-default-async.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile-esm/es-module-default-promise.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile/es-module-default-promise.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/async.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/es-module-default-null.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/load_file/es-module-default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile/es-module-default-async.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (7)
test/fixtures/loadfile-esm/es-module-default-async.js (1)
2-4: LGTM! Clean and focused test fixture.
The async function implementation correctly demonstrates the use case of returning a configuration object asynchronously.
test/fixtures/loadfile-esm/es-module-default-promise.js (1)
2-4: LGTM! Good coverage of Promise-based returns.
The direct Promise.resolve() implementation provides good test coverage alongside the async/await pattern in the previous fixture.
test/fixtures/load_file/es-module-default-async.js (2)
2-5: LGTM! Good CommonJS compatibility test.
The implementation correctly demonstrates async function support in a CommonJS-style module, providing good coverage alongside the ES module fixtures.
1-5: Verify test coverage across module patterns
The test fixtures provide good coverage of different module patterns:
- ES modules with async/await
- ES modules with direct Promise returns
- CommonJS-style modules with async functions
Let's verify that these are all the module patterns we need to test:
✅ Verification successful
Test coverage for module patterns is comprehensive
The test fixtures cover all essential async/Promise module patterns:
- ES modules with async/await (
es-module-default-async.js) - ES modules with Promise returns (
es-module-default-promise.js) - CommonJS with Promise returns (
promise_function.js) - CommonJS with async/await (
async.js)
The patterns include variations in:
- Module systems (ES modules vs CommonJS)
- Async handling (async/await vs Promise.resolve)
- Function syntax (arrow functions vs regular functions)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other async/promise-related test fixtures we might have missed
fd -e js -e ts . test/fixtures/ -x grep -l "async\|Promise\|resolve"
Length of output: 4922
Script:
#!/bin/bash
# Let's check the actual content of load_file test fixtures to understand the module patterns
echo "=== ES Module Default Async ==="
cat test/fixtures/load_file/es-module-default-async.js
echo -e "\n=== ES Module Default Promise ==="
cat test/fixtures/load_file/es-module-default-promise.js
echo -e "\n=== Promise Function ==="
cat test/fixtures/load_file/promise_function.js
echo -e "\n=== Async Function ==="
cat test/fixtures/load_file/async.js
Length of output: 915
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
test/fixtures/loadfile/es-module-default-async.js (1)
3-5: LGTM! Implementation aligns with PR objectives.
The async function implementation correctly demonstrates the use case of returning configuration asynchronously, which is essential for the PR's goal of supporting dynamic config loading.
test/loader/load_file.test.ts (1)
40-50: LGTM! Good test coverage for CJS modules
The tests effectively verify:
- Functions returning promises
- Async functions
test/fixtures/load_file/es-module-default-promise.js (1)
3-5: LGTM! Clean implementation of async config loading
The implementation correctly demonstrates the async config loading use case mentioned in the PR objectives. The function returns a Promise that resolves to a configuration object, which is exactly what's needed for loading async configurations (e.g., from remote config centers).
[skip ci] ## [6.2.0](v6.1.0...v6.2.0) (2024-12-18) ### Features * support file returning async functions when loading it ([#272](#272)) ([cb48d0e](cb48d0e))
Use Case:
dynamically load mysql connection configuration from a remote config center to keep it secure.
Summary by CodeRabbit
New Features
Bug Fixes
Tests