feat: support gen ts defines on esm project #115
Conversation
WalkthroughThis pull request updates multiple files to enhance Egg.js support. The Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant SaveEggInfo as save_egg_info.mjs
participant Loader
participant FS as FileSystem
CLI->>SaveEggInfo: Provide baseDir, frameworkName, saveEggInfoPath
SaveEggInfo->>SaveEggInfo: Log baseDir and compute frameworkPath
SaveEggInfo->>Loader: Initialize loader for framework
Loader-->>SaveEggInfo: Return plugins and configuration (or warning on error)
SaveEggInfo->>FS: Write eggInfo JSON to file
SaveEggInfo->>CLI: Output keys of loaded plugins
sequenceDiagram
participant Main as main() in eggInfo.ts
participant PkgInfo as PackageInfo
participant ESM as ESM Handler
participant Loader
Main->>PkgInfo: Retrieve package info
alt Package type is 'module'
Main->>ESM: Construct ESM script path and execute via execFileSync
ESM-->>Main: ESM script completed
Main->>Logger: Log warning about ESM script usage
else Package type is not 'module'
Main->>Main: Continue with default processing
end
Main->>Loader: Call getFrameworkPath and then getLoader to handle framework loading
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
test/utils.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (7)
✨ Finishing Touches
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. 🪧 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 (
|
There was a problem hiding this comment.
PR Overview
This PR adds support for generating TypeScript definitions on ESM projects by introducing an ESM-based egg info script and updating related loader functions. Key changes include:
- A new file (save_egg_info.mjs) for saving egg info using ESM modules.
- A refactored constructor in Watcher to decouple property initialization.
- Enhanced eggInfo.ts to support ESM projects by splitting framework path resolution and invoking an external script via execFileSync.
Reviewed Changes
| File | Description |
|---|---|
| save_egg_info.mjs | New script that gathers and writes egg info using ESM imports. |
| src/watcher.ts | Refactored the constructor to manually assign the helper property. |
| src/scripts/eggInfo.ts | Added ESM support and refactored loader functions (getFrameworkPath and getLoader) to improve clarity. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/watcher.ts:42
- [nitpick] Consider using a parameter property in the constructor (e.g., using 'constructor(public helper: TsHelper)') to streamline the initialization of 'helper', unless explicit assignment is necessary for clarity.
helper: TsHelper;
src/scripts/eggInfo.ts:35
- Consider adding error handling around the execFileSync call to catch and log errors from the spawned process, which will improve robustness in case the external script fails.
execFileSync(process.execPath, [ '--no-warnings', '--loader', esmLoader, saveEggInfoPath, cwd, framework, eggInfoPath ], {
![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 (2)
save_egg_info.mjs (1)
7-7: Consider adding input validation for command-line argumentsThe script expects exactly three command-line arguments without any validation. Consider adding basic validation to ensure all required arguments are provided and provide meaningful error messages if they're missing.
-const [ baseDir, frameworkName, saveEggInfoPath ] = process.argv.slice(2); +const [ baseDir, frameworkName, saveEggInfoPath ] = process.argv.slice(2); + +if (!baseDir || !frameworkName || !saveEggInfoPath) { + console.error('[egg-ts-helper] Error: Missing required arguments. Usage: node save_egg_info.mjs baseDir frameworkName saveEggInfoPath'); + process.exit(1); +}src/scripts/eggInfo.ts (1)
28-31: Improve code using optional chaining for framework determinationThe framework determination can be improved by using optional chaining to access the nested
egg.frameworkproperty.- const framework = (pkg.egg || {}).framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg'; + const framework = pkg.egg?.framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg';🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(3 hunks)save_egg_info.mjs(1 hunks)src/scripts/eggInfo.ts(5 hunks)src/watcher.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/scripts/eggInfo.ts
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (12)
package.json (3)
13-14: Good addition of the new save_egg_info.mjs to files arrayAdding
save_egg_info.mjsto the files array ensures that this new file will be included when the package is published to npm.
39-39: Version pinning for @eggjs/utils looks appropriateThe addition of the
@eggjs/utilsdependency is necessary for the new ESM support functionality, and the version requirement of^4.3.0follows semver conventions.
64-64: Minor version update for egg dependencyUpdating the
eggdependency from^4.0.7to^4.0.9is a minor version bump that should be compatible.src/watcher.ts (1)
42-44: Improved class structure by explicitly declaring the helper memberThis change improves the class structure by explicitly declaring the
helperproperty as a class member rather than implicitly creating it through the constructor parameter. This is more consistent with TypeScript best practices and makes the code more maintainable.save_egg_info.mjs (3)
1-4: Good module imports with proper ESM syntaxThe ESM import syntax is correctly used with named imports for the required dependencies, including the new
getLoaderfunction from@eggjs/utils.
19-25: Good error handling for loadPlugin operationThe error handling for the
loadPluginoperation is well-implemented. It catches any exceptions that might occur during the operation, logs them as warnings, and allows the script to continue execution.
27-33: Good error handling for loadConfig operationSimilar to the
loadPluginerror handling, theloadConfigerror handling is well-implemented with proper warning logs that include the error details and context information.src/scripts/eggInfo.ts (5)
7-9: Good addition of necessary imports for ESM supportThe addition of
execFileSyncandpathToFileURLimports is necessary for the ESM module support functionality.
31-42: Well-implemented ESM supportThe addition of ESM support is well-implemented. The code correctly:
- Detects if the package type is 'module'
- Uses the appropriate script path
- Sets up the Node.js ESM loader properly
- Passes the required environment variables and arguments to the script
This is critical for supporting generation of TypeScript definitions in ESM projects.
90-101: Good refactoring of getFrameworkPath functionThe function
getFrameworkPathis a good extraction of the framework path resolution logic from the originalgetLoaderfunction. It has clean error handling and proper fallback paths.
103-105: Well-structured loader functionThe new
getLoaderfunction correctly reuses thegetFrameworkPathfunction to get the framework path before proceeding with the loader logic. This modular approach improves code organization and maintainability.
130-131: Simplified error handling in try-catch blockThe error handling in the
findEggCorefunction has been simplified by removing the unused catch parameter. This is a good practice when the error details aren't needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils.ts (1)
264-264:❓ Verification inconclusive
Adds .js extension to TypeScript imports for ESM compatibility
This change modifies the import string format for TypeScript files by appending the
.jsextension to the import path. This is a necessary adjustment for proper ECMAScript Modules (ESM) compatibility, as ESM requires file extensions in import statements, unlike CommonJS.This change correctly addresses the requirements for ESM projects. When TypeScript files are transpiled to JavaScript in an ESM environment, the import statements need to reference the
.jsfiles (not the original.tsfiles). Without this change, ESM module resolution would fail at runtime because it couldn't find the modules without the extension.Let's verify that this change doesn't break any existing imports in the codebase:
🏁 Script executed:
#!/bin/bash # Check for existing import statements that might be affected by this change rg "from '[^']+'" --type ts # Look for any ESM-related configuration or mention in the codebase rg "type\s*:\s*['\"](module|commonjs)['\"]" --type jsonLength of output: 15309
ESM Import Adjustment – Manual Confirmation Recommended
The change in
src/utils.ts(line 264) to append a.jsextension on TypeScript imports is implemented as expected for ESM compatibility. Our initial searches confirm that the modified import pattern appears correctly in the codebase. However, the absence of any explicit ESM configuration (e.g.,"type": "module"or"commonjs") in JSON files means that further manual verification is advisable to ensure this adjustment aligns with the project's intended module system.
- The new snippet in
src/utils.tsnow generates the string:which is correct for ESM environments when transpiling TypeScript.const fromStr = isTS ? `from '${importPath}.js'` : `= require('${importPath}')`;rgfound the expected usage of import statements across the codebase, and no conflicting patterns were detected.- Action: Please manually verify the project’s module configuration (for example, in
package.jsonor tsconfig settings) to ensure that adding the.jsextension meets the overall ESM requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/scripts/eggInfo.ts (1)
28-30: Use optional chaining for cleaner code.The static analysis tool correctly identified that the property access can be simplified.
- const framework = (pkg.egg || {}).framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg'; + const framework = pkg.egg?.framework || process.env.ETS_SCRIPT_FRAMEWORK || 'egg';🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/fixtures/app-esm/app/extend/context.ts (1)
3-15: Consider adding TypeScript interfaces for better type safety.The current implementation works but would benefit from a more structured approach by implementing a proper interface for the context extension.
import { Context } from 'egg'; +interface ExtendContext { + ctx: Context; + isProd: boolean; + isAjax: boolean; +} -const extendContext = { +const extendContext: ExtendContext = { get ctx(): Context { return this as any as Context; }, get isProd(): boolean { return this.ctx.app.config.env === 'prod'; }, get isAjax(): boolean { return this.ctx.get('X-Requested-With') === 'XMLHttpRequest'; }, }; export default extendContext;test/fixtures/app-esm/app/extend/application.ts (4)
11-14: Fix string concatenation in log message.The string concatenation lacks proper spacing, which will result in unexpected output like "123is Cool" instead of "123 is Cool".
isCool() { const val = '123'; - console.info(val + 'is Cool'); + console.info(val + ' is Cool'); },
16-22: Consolidate duplicate functions or differentiate their implementations.The arrow functions 'test-gg' and 'test-ggs' have identical implementations, which indicates potential code duplication.
Either consolidate them into a single method, or differentiate their implementations to reflect their distinct purposes:
'test-gg': () => { console.info('ko'); }, 'test-ggs': () => { - console.info('ko'); + console.info('ko with different behavior'); },
29-30: Simplify variable assignment.The declaration and assignment of
secondExportAppcan be combined into a single statement for readability.-let secondExportApp: any; -secondExportApp = exportApp; +const secondExportApp: any = exportApp;
32-34: Add return type and improve logging.The
abcfunction should have a return type defined and the console message could be more descriptive.-export function abc() { - console.info('sss'); +export function abc(): void { + console.info('abc function called'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
package.json(3 hunks)save_egg_info.mjs(1 hunks)src/scripts/eggInfo.ts(4 hunks)test/fixtures/app-esm/app/controller/home.ts(1 hunks)test/fixtures/app-esm/app/extend/agent.ts(1 hunks)test/fixtures/app-esm/app/extend/application.ts(1 hunks)test/fixtures/app-esm/app/extend/context.ts(1 hunks)test/fixtures/app-esm/app/extend/helper.ts(1 hunks)test/fixtures/app-esm/config/config.ts(1 hunks)test/fixtures/app-esm/config/plugin.local.ts(1 hunks)test/fixtures/app-esm/config/plugin.ts(1 hunks)test/fixtures/app-esm/package.json(1 hunks)test/generators/config.test.ts(4 hunks)test/index.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- test/fixtures/app-esm/app/extend/agent.ts
- test/fixtures/app-esm/package.json
- test/fixtures/app-esm/config/plugin.ts
- test/fixtures/app-esm/config/plugin.local.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- save_egg_info.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
src/scripts/eggInfo.ts
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (18)
src/scripts/eggInfo.ts (5)
7-8: Appropriate imports added for ESM support.The new imports
execFileSyncandpathToFileURLare correctly added to support the execution of ESM modules, which aligns with the PR objective of supporting TypeScript definitions in ESM projects.
31-44: Well-implemented ESM support logic.The conditional block for handling ESM packages is a good addition. It:
- Correctly identifies ESM packages via
pkg.type === 'module'- Uses the appropriate Node.js APIs to execute the ESM script
- Passes all necessary parameters to the script
- Sets up the environment variables correctly
The implementation ensures compatibility with ESM projects while maintaining the existing functionality for CommonJS projects.
92-104: Good extraction of framework path resolution.The function rename from
getLoadertogetFrameworkPathimproves code clarity by accurately describing what the function does. The implementation correctly resolves the framework path using Node.js's resolution algorithm first and falls back to a direct path if needed.
106-127: Clean refactoring of the loader initialization.The new
getLoaderfunction now properly uses the extractedgetFrameworkPathfunction, maintaining the original functionality while making the code more modular and easier to understand.
132-134: Simplified error handling.The error handling in the
findEggCorefunction has been simplified by removing the unused catch parameter. This is a good practice as it avoids unused variables.test/fixtures/app-esm/config/config.ts (1)
1-5: Appropriate configuration structure for ESM module.The configuration file exports a default function returning a config object, which is the correct pattern for Egg.js configurations. The
keysproperty is essential for security in Egg.js applications.test/fixtures/app-esm/app/controller/home.ts (1)
1-5: Controller implementation follows Egg.js conventions.The Home controller class with an async index method follows the standard Egg.js controller pattern. This is a good example of the expected structure for ESM-based Egg.js applications.
test/fixtures/app-esm/app/extend/helper.ts (1)
1-9: Helper extension correctly implemented for ESM.The helper extension follows the Egg.js pattern of exporting an object with utility methods. Both
isCoolandisNotCoolmethods are properly defined.test/fixtures/app-esm/app/extend/context.ts (3)
1-6: Type casting approach needs improvement.The use of
this as any as Contextbypasses TypeScript's type checking, which can lead to runtime errors if the actual type doesn't match the expected one. While this pattern is common in Egg.js extensions, it would be better to use a more type-safe approach.- get ctx(): Context { - return this as any as Context; + get ctx(): Context { + // Explicitly cast to Context type to maintain type safety + return this as Context;
8-10: LGTM: Clean implementation for checking production environment.The
isProdgetter correctly checks if the application environment is set to 'prod' by accessing the app's configuration.
12-14: LGTM: Proper implementation for AJAX request detection.The
isAjaxgetter correctly identifies AJAX requests by checking the 'X-Requested-With' header against the standard value 'XMLHttpRequest'.test/index.test.ts (1)
435-450: LGTM: Well-structured test for ESM application.The test case is correctly structured to verify TypeScript definition generation in an ESM environment. It creates the helper with appropriate parameters and checks for the existence of expected definition files. The use of
it.skipis appropriate if this test is experimental or waiting for additional implementation.test/generators/config.test.ts (6)
17-17: LGTM: Updated import statement for ESM compatibility.The assert statement has been correctly updated to check for
.jsextension in import paths, which is required for ESM compatibility.
28-30: LGTM: Properly updated import assertions for multiple config files.The assertions are correctly updated to check for
.jsextensions in all import paths, maintaining consistency across the test suite.
42-43: LGTM: Updated import assertions with correct extensions.The assertions correctly check for
.jsextension in the import path for the default config, and appropriately assert that the local config is not imported.
51-53: LGTM: Consistent update of import path assertions.The assertions have been properly updated to check for
.jsextensions in all import paths, maintaining consistency with the ESM module standard.
67-68: LGTM: Updated import assertions for the file removal test case.The assertions correctly check for
.jsextensions in import paths, ensuring consistent testing of ESM compatibility.
75-75: LGTM: Updated import assertion for config.ts file test.The assertion has been properly updated to check for the
.jsextension in the import path, ensuring ESM compatibility.
[skip ci] ## [3.1.0](v3.0.0...v3.1.0) (2025-03-05) ### Features * support gen ts defines on esm project ([#115](#115)) ([5c25621](5c25621))
eggjs/utils#43
Summary by CodeRabbit
New Features
Bug Fixes
Refactor