fix: require support paths #118
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Line
participant AgentWorker as Agent Worker
participant AppWorker as App Worker
participant ModuleLoader as Module Loader
CLI->>AgentWorker: Start with options
AgentWorker->>ModuleLoader: Import modules with baseDir
ModuleLoader-->>AgentWorker: Resolve and load modules
CLI->>AppWorker: Start with options
AppWorker->>ModuleLoader: Import modules with baseDir
ModuleLoader-->>AppWorker: Resolve and load modules
Poem
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: 1
🧹 Nitpick comments (1)
src/app_worker.ts (1)
30-32: Add error handling for module importsWhile the module resolution path configuration is correct, consider adding try-catch error handling to provide better error messages when modules can't be resolved from the specified baseDir.
if (options.require) { // inject for (const mod of options.require) { + try { await importModule(mod, { paths: [ options.baseDir ], }); + } catch (err) { + consoleLogger.error('[app_worker] Failed to load required module %s: %s', mod, err.message); + throw err; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.json(1 hunks)src/agent_worker.ts(2 hunks)src/app_worker.ts(3 hunks)src/utils/options.ts(1 hunks)test/agent_worker.test.ts(1 hunks)test/fixtures/apps/agent-debug-port/agent.js(0 hunks)test/fixtures/apps/agent-debug-port/inject1.js(1 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/apps/agent-debug-port/agent.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/apps/agent-debug-port/inject1.js
🔇 Additional comments (7)
src/agent_worker.ts (3)
21-21: Well-defined optional property baseDir
This property addition is straightforward, allowing dynamic resolution of modules with a base directory.
27-31: Asynchronous module imports with baseDir
The loop correctly imports modules from the options.require array. Using importModule with paths: [ options.baseDir ] ensures modules are resolved relative to baseDir. This change enables more flexible module resolution.
- Potential Improvement: Add a defensive check to confirm that
options.baseDiris defined or a valid path before importing.
42-44: Importing the framework with baseDir
The same approach is used when importing the framework. Ensure baseDir is always set correctly or fallback to a default if it’s undefined. Otherwise, runtime errors may occur if baseDir is missing.
src/utils/options.ts (1)
109-111: Explicitly setting paths in importModule
The addition of paths: [ options.baseDir! ] clarifies module resolution. This is beneficial for monorepo setups or custom directory structures.
test/agent_worker.test.ts (1)
18-21: Testing module injection
Adding { require: [ './inject1.js' ] } and asserting the stdout message is a good way to ensure custom modules are loaded. This test case effectively verifies that the code paths for baseDir and require are exercised.
package.json (1)
36-36: Dependency update to @eggjs/utils v4.2.1
Upgrading to ^4.2.1 can bring new features or fixes. Double-check any breaking changes or deprecations in the changelog.
src/app_worker.ts (1)
18-18: LGTM: Well-placed baseDir property addition
The addition of the mandatory baseDir property is appropriate for specifying module resolution paths.
| const { Application } = await importModule(options.framework, { | ||
| paths: [ options.baseDir ], | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Framework resolution is safe, but error handling can be improved
Based on the codebase analysis:
- The framework import pattern is consistent across the codebase (
app_worker.ts,agent_worker.ts, andutils/options.ts). - Error handling exists at the application initialization level but not specifically for framework imports.
- The codebase follows a pattern of using
consoleLogger.errorfor error logging before throwing.
Here's a refined suggestion for error handling:
- const { Application } = await importModule(options.framework, {
- paths: [ options.baseDir ],
- });
+ try {
+ const { Application } = await importModule(options.framework, {
+ paths: [ options.baseDir ],
+ });
+ if (!Application) {
+ throw new Error(`Framework ${options.framework} does not export an Application class`);
+ }
+ } catch (err) {
+ consoleLogger.error('[app_worker] Failed to load framework: %s', err.message);
+ throw err;
+ }🔗 Analysis chain
Verify framework resolution and enhance error handling
The framework import configuration looks correct, but let's ensure it doesn't break framework discovery and add specific error handling.
Let's verify the framework resolution:
Consider adding specific error handling:
- const { Application } = await importModule(options.framework, {
- paths: [ options.baseDir ],
- });
+ let Application;
+ try {
+ const framework = await importModule(options.framework, {
+ paths: [ options.baseDir ],
+ });
+ Application = framework.Application;
+ if (!Application) {
+ throw new Error(`Framework ${options.framework} does not export an Application class`);
+ }
+ } catch (err) {
+ consoleLogger.error('[app_worker] Failed to load framework %s: %s', options.framework, err.message);
+ throw err;
+ }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the framework can be resolved from baseDir
ast-grep --pattern 'new $_(options)' | rg -A 2 'Application'
Length of output: 117
Script:
#!/bin/bash
# Let's check how importModule is used for framework loading across the codebase
rg "importModule.*framework" -B 2 -A 2
Length of output: 799
Script:
#!/bin/bash
# Check the framework loading patterns and error handling
ast-grep --pattern 'try {
$$$
const $_ = await importModule($_, $_);
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Check if there are any framework-specific error handlers
rg "Failed to load framework" -B 2 -A 2
Length of output: 41
Script:
#!/bin/bash
# Let's check for any existing error handling patterns in the codebase
rg "catch.*err" -B 2 -A 2
Length of output: 4186
Script:
#!/bin/bash
# Check how errors are logged in the codebase
rg "consoleLogger.error" -B 2 -A 2
Length of output: 1828
Script:
#!/bin/bash
# Check the importModule implementation
ast-grep --pattern 'async function importModule($_) {
$$$
}'
Length of output: 64
[skip ci] ## [3.0.1](v3.0.0...v3.0.1) (2024-12-30) ### Bug Fixes * require support paths ([#118](#118)) ([b74872c](b74872c))
Summary by CodeRabbit
Release Notes
Dependencies
@eggjs/utilspackage to version 4.2.1Enhancements
baseDiroptionTesting
Deprecation Notice
keyandcert