feat: support esm project to parse documentation #30
Conversation
Romakita
force-pushed
the
feat-add-esm-support-import
branch
from
7682333 to
3c52df0
Compare
WalkthroughThe changes involve the removal of several configuration files and updates to the ESLint setup, including the introduction of a new Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
packages/ts-doc/src/models/DocFile.js (1)
3-4: Consider consolidating fs operations in future updates.While maintaining both sync and async fs operations is valid during the transition, consider moving entirely to async operations in a future update for consistency.
package.json (1)
46-66: Dependencies well-aligned with ESM support objective.The comprehensive update of the development toolchain provides a solid foundation for ESM support:
- ESLint v9 with native ESM support
- TypeScript 5.6.3 with enhanced ESM capabilities
- Modern versions of all related tools
Consider adding
"type": "module"to package.json to fully embrace ESM.eslint.config.mjs (1)
9-18: Consider adding more common ignore patterns.The ignore patterns cover most cases, but you might want to consider adding:
**/node_modules**/.nyc_output**/.yarnpackages/ts-doc/src/models/DocSymbol.spec.js (1)
42-44: Consider enhancing the mock implementationThe
importModulemock returns an empty object, which might not provide sufficient test coverage. Consider adding more realistic mock data based on actual module structure.packages/ts-doc/src/models/DocSymbol.js (2)
Line range hint
162-192: Add error handling and parameter validation.The async implementation should include proper error handling and parameter validation to ensure robustness.
Apply this diff to improve error handling:
async setDocFile(docFile) { + if (!docFile) { + throw new Error('DocFile is required'); + } + this.docFile = docFile; if (this.relativePath.match("decorators") && this.symbolType === "function") { this.symbolType = "decorator"; this.symbolLabel = context.symbolTypes["decorator"].label; this.symbolCode = context.symbolTypes["decorator"].code; } if (this.relativePath.match("services") && this.symbolType === "function") { this.symbolType = "service"; this.symbolLabel = context.symbolTypes["service"].label; this.symbolCode = context.symbolTypes["service"].code; } if (this.private === undefined) { if (this.symbolType !== "interface") { this.private = true; - const exported = await this.docFile.importModule(); + try { + const exported = await this.docFile.importModule(); - if (exported) { - const symbolPrivate = exported[this.symbolName.trim()]; + if (exported) { + const symbolPrivate = exported[this.symbolName.trim()]; - this.private = !symbolPrivate; - } + this.private = !symbolPrivate; + } - if (this.private) { - this.labels.push({key: "private", value: "private"}); + if (this.private) { + this.labels.push({key: "private", value: "private"}); + } + } catch (error) { + console.warn(`Failed to import module for ${this.symbolName}: ${error.message}`); + // Fallback to private if import fails + this.private = true; + this.labels.push({key: "private", value: "private"}); } } } }
Line range hint
162-192: Consider refactoring for better separation of concerns.The method currently handles multiple responsibilities: setting the doc file, determining symbol type, and managing privacy. Consider splitting these into separate methods for better maintainability.
Consider refactoring like this:
async setDocFile(docFile) { if (!docFile) { throw new Error('DocFile is required'); } this.docFile = docFile; + await this.determineSymbolType(); + await this.determinePrivacy(); + } + + determineSymbolType() { if (this.relativePath.match("decorators") && this.symbolType === "function") { this.symbolType = "decorator"; this.symbolLabel = context.symbolTypes["decorator"].label; this.symbolCode = context.symbolTypes["decorator"].code; } if (this.relativePath.match("services") && this.symbolType === "function") { this.symbolType = "service"; this.symbolLabel = context.symbolTypes["service"].label; this.symbolCode = context.symbolTypes["service"].code; } + } + + async determinePrivacy() { if (this.private === undefined) { if (this.symbolType !== "interface") { this.private = true; try { const exported = await this.docFile.importModule(); if (exported) { const symbolPrivate = exported[this.symbolName.trim()]; this.private = !symbolPrivate; } if (this.private) { this.labels.push({key: "private", value: "private"}); } } catch (error) { console.warn(`Failed to import module for ${this.symbolName}: ${error.message}`); this.private = true; this.labels.push({key: "private", value: "private"}); } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
- .eslintignore (0 hunks)
- .eslintrc (0 hunks)
- .github/workflows/build.yml (3 hunks)
- .mocharc.js (1 hunks)
- eslint.config.mjs (1 hunks)
- package.json (2 hunks)
- packages/ts-doc/src/highlight/index.js (1 hunks)
- packages/ts-doc/src/models/DocFile.js (3 hunks)
- packages/ts-doc/src/models/DocSymbol.js (2 hunks)
- packages/ts-doc/src/models/DocSymbol.spec.js (3 hunks)
- packages/ts-doc/src/parsers/DocParser.js (1 hunks)
- packages/ts-doc/src/scan/scan.js (1 hunks)
💤 Files with no reviewable changes (2)
- .eslintignore
- .eslintrc
✅ Files skipped from review due to trivial changes (2)
- .mocharc.js
- packages/ts-doc/src/highlight/index.js
🔇 Additional comments (14)
.github/workflows/build.yml (2)
34-39: LGTM! Action versions and configuration are well updated.Good improvements:
- Updated to latest action versions
- Added caching for better performance
- Proper secret management
Just a reminder: Ensure the repository secrets (GH_TOKEN and NPM_TOKEN) are up to date and have the minimum required permissions.
#!/bin/bash # Check if the workflow has the required permissions in repository settings gh api repos/:owner/:repo/actions/permissions --jq '.enabled_repositories, .allowed_actions'Also applies to: 48-48
17-22: Consider testing against multiple Node.js versions.While the action version updates are good improvements, testing against a single Node.js version (20.x) might miss compatibility issues with other versions that your users might be using. Consider keeping a matrix strategy for critical Node.js versions.
packages/ts-doc/src/models/DocFile.js (1)
67-69: Consider implementing a cache cleanup mechanism.The
mapExportedcache grows indefinitely which could lead to memory issues. Consider implementing a cleanup mechanism or using a proper caching solution:Consider using a proper caching solution like:
const moduleCache = new Map(); // Cleanup old entries periodically or use LRU cache setInterval(() => moduleCache.clear(), 3600000); // Clear every hourpackage.json (3)
46-47: Major ESLint upgrade with enhanced capabilities.The ESLint ecosystem upgrade brings significant improvements:
- ESLint v9 with better ESM support
- TypeScript integration via @typescript-eslint
- Import sorting and Vitest testing support
However, the major version jump from ESLint v7 to v9 may introduce breaking changes.
#!/bin/bash # Description: Check for potential ESLint configuration issues # Test: Look for old ESLint config files that might conflict echo "Checking for old ESLint config files..." fd -g '.eslintrc*' # Test: Check for any ESLint disable comments that might need review echo -e "\nChecking for eslint-disable comments that might need review..." rg -i "eslint-disable"Also applies to: 52-56
62-62: Review Prettier formatting changes and TypeScript configuration.The upgrade to Prettier v3 may cause significant formatting changes. Additionally, ensure TypeScript is properly configured for ESM support.
#!/bin/bash # Description: Check for TypeScript and Prettier configuration # Test: Look for TypeScript config echo "Checking for TypeScript config..." fd -g 'tsconfig*.json' # Test: Look for Prettier config echo -e "\nChecking for Prettier config..." fd -g '.prettier*'Also applies to: 66-66
13-14: Verify ESLint configuration coverage.The simplified lint commands suggest reliance on ESLint config for scope definition. Ensure the new
eslint.config.mjsproperly includes all necessary files and extensions.✅ Verification successful
ESLint configuration is properly set up for the project scope
The simplified lint commands in package.json are justified as the
eslint.config.mjsfile properly defines:
- Coverage for all necessary file types (
.js,.mjs,.cjs,.jsx,.ts,.tsx)- Appropriate ignores for build artifacts and generated files
- TypeScript support with proper parser configuration
- Project-specific rule customizations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ESLint configuration coverage # Test: Check for ESLint config and its content echo "Checking for ESLint config file..." fd -e mjs -e js -e cjs -e json -g '*eslint*config*' # Test: List all JS/TS files that should be linted echo -e "\nFiles that should be covered by linting:" fd -e js -e mjs -e ts -e tsxLength of output: 481
Script:
#!/bin/bash # Let's try a more comprehensive search for ESLint configuration echo "Searching for ESLint config files (including hidden)..." fd -H -e mjs -e js -e cjs -e json -g '*eslint*config*' fd -H -g '.eslintrc*' echo -e "\nChecking package.json for ESLint config..." cat package.json | grep -A 10 '"eslintConfig"' echo -e "\nChecking source files that need linting..." fd -t f -e js -e mjs -e ts -e tsx -e jsx echo -e "\nChecking if there's any ESLint configuration in package.json..." cat package.json | jq '.eslintConfig'Length of output: 15970
Script:
#!/bin/bash # Let's examine the ESLint configuration file content echo "Checking ESLint config content..." cat eslint.config.mjs echo -e "\nChecking for any ESLint overrides in package.json..." cat package.json | jq '.overrides'Length of output: 2171
eslint.config.mjs (3)
1-6: LGTM! Clean and well-organized imports.The imports are properly structured and use the recommended
node:prefix for Node.js built-in modules.
62-62: Verify Prettier configuration exists.The ESLint configuration includes Prettier integration. Ensure that a Prettier configuration file exists.
#!/bin/bash # Check for Prettier configuration files fd -g '.prettierrc*' if [ $? -ne 0 ]; then echo "No Prettier configuration file found. Consider adding one for consistent formatting." fi
49-61: Verify if Vitest configuration is needed.The commented Vitest configuration suggests that the project might use Vitest for testing. If so, this configuration should be uncommented and properly configured.
#!/bin/bash # Check if project uses Vitest if rg -q "vitest" package.json; then echo "Vitest dependency found. Consider uncommenting and configuring the Vitest rules." fipackages/ts-doc/src/models/DocSymbol.spec.js (3)
7-7: LGTM: Proper async/await usageThe addition of the async keyword is appropriate for handling the asynchronous
setDocFileoperation.
Line range hint
47-95: LGTM: Well-structured test suiteThe test suite comprehensively covers the DocSymbol functionality with clear, focused test cases. The unchanged assertions indicate good interface stability despite the async implementation changes.
Line range hint
29-45: Verify the impact of requireModule → importModule renameThe rename from
requireModuletoimportModulesuggests a transition towards ESM. Let's verify this change is consistent across the codebase.✅ Verification successful
Let me verify if this is a complete migration by checking the git history for any recent changes related to this rename.
Based on the git history and previous search results, I can now generate the final response:
The rename from
requireModuletoimportModuleis complete and consistentThe migration from
requireModuletoimportModulehas been properly implemented across all relevant files in the codebase:
packages/ts-doc/src/models/DocFile.js: Main implementationpackages/ts-doc/src/models/DocSymbol.js: Usage in symbol resolutionpackages/ts-doc/src/models/DocSymbol.spec.js: Test implementationThe git history confirms this was an intentional rename with all instances updated, and there are no remaining usages of
requireModulein the current codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of requireModule to ensure complete migration echo "Checking for remaining requireModule usage..." rg "requireModule" -l # Search for new importModule usage to verify consistent implementation echo "Checking importModule implementation across files..." rg "importModule" -A 3Length of output: 1081
Script:
#!/bin/bash # Check git history for requireModule to importModule changes git log -p | grep -E '[-+].*require(Module|module)|[-+].*import(Module|module)' -A 2 -B 2Length of output: 863
packages/ts-doc/src/parsers/DocParser.js (1)
39-42: Verify concurrent access safety.The code modifies shared state (
context.symbolsanddocFile.symbols) in an async loop. This could lead to race conditions ifparse()is called concurrently on the same docFile.Let's check for concurrent usage:
Consider one of these approaches to prevent race conditions:
- Add a mutex/lock mechanism
- Use immutable data structures
- Document that the method is not thread-safe
packages/ts-doc/src/models/DocSymbol.js (1)
Line range hint
162-192: Verify the impact of async changes across the codebase.The conversion to async might require updates in calling code.
Let's verify the usage of this method:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the usage of
setDocFile.All calls to
setDocFileare properly awaitedThe method is called in two locations and both correctly use the
awaitkeyword:
packages/ts-doc/src/models/DocSymbol.spec.js:await this.docSymbol.setDocFile({packages/ts-doc/src/parsers/DocParser.js:await symbol.setDocFile(docFile);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for setDocFile calls to ensure they're properly awaited rg "setDocFile\(" -A 2Length of output: 807
packages/ts-doc/src/scan/scan.js
Outdated
| async scanFiles(patterns) { | ||
| context.logger("Scan folders '" + chalk.cyan(JSON.stringify(patterns)) + "'"); | ||
|
|
||
| let symbolsSize = 0; | ||
| const files = globby.sync(patterns); |
There was a problem hiding this comment.
Replace globby.sync() with async alternative.
Using synchronous file system operations in an async function can block the event loop. Consider using the Promise-based API from globby instead.
- const files = globby.sync(patterns);
+ const files = await globby(patterns);📝 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.
| async scanFiles(patterns) { | |
| context.logger("Scan folders '" + chalk.cyan(JSON.stringify(patterns)) + "'"); | |
| let symbolsSize = 0; | |
| const files = globby.sync(patterns); | |
| async scanFiles(patterns) { | |
| context.logger("Scan folders '" + chalk.cyan(JSON.stringify(patterns)) + "'"); | |
| let symbolsSize = 0; | |
| const files = await globby(patterns); |
| for (const file of files) { | ||
| try { | ||
| DocParser.parse(new DocFile(file)).forEach((symbol) => { | ||
| const symbols = await DocParser.parse(new DocFile(file)); | ||
|
|
||
| symbols.forEach((symbol) => { | ||
| context.logger(`Scanned symbol '${chalk.cyan(symbol.symbolName)}'`); | ||
| symbolsSize++; | ||
| }); | ||
| } catch (er) { | ||
| context.logger.error(chalk.red(er), er.stack); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and reporting.
While the async/await conversion is correct, the error handling could be improved to:
- Track failed files separately
- Provide more accurate final statistics
+ let failedFiles = 0;
for (const file of files) {
try {
const symbols = await DocParser.parse(new DocFile(file));
symbols.forEach((symbol) => {
context.logger(`Scanned symbol '${chalk.cyan(symbol.symbolName)}'`);
symbolsSize++;
});
} catch (er) {
context.logger.error(chalk.red(er), er.stack);
+ context.logger.error(`Failed to parse file: ${chalk.red(file)}`);
+ failedFiles++;
}
}
- context.logger(`${chalk.green(symbolsSize)} scanned symbols`);
+ context.logger(
+ `${chalk.green(symbolsSize)} scanned symbols, ` +
+ `${chalk.red(failedFiles)} files failed`
+ );Committable suggestion was skipped due to low confidence.
| @@ -13,7 +14,7 @@ class DocFile { | |||
| constructor(file) { | |||
| this.file = normalizePath(file); | |||
| this.symbols = new Map(); | |||
| this.contents = fs.readFileSync(file).toString(); | |||
| this.contents = fsSync.readFileSync(file).toString(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing lazy loading for file contents.
Reading file contents synchronously in the constructor could block execution for large files. Consider implementing lazy loading where contents are read only when needed:
constructor(file) {
this.file = normalizePath(file);
this.symbols = new Map();
- this.contents = fsSync.readFileSync(file).toString();
+ this._contents = null;
}
+
+get contents() {
+ if (!this._contents) {
+ this._contents = fsSync.readFileSync(this.file).toString();
+ }
+ return this._contents;
+}📝 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.
| this.contents = fsSync.readFileSync(file).toString(); | |
| constructor(file) { | |
| this.file = normalizePath(file); | |
| this.symbols = new Map(); | |
| this._contents = null; | |
| } | |
| get contents() { | |
| if (!this._contents) { | |
| this._contents = fsSync.readFileSync(this.file).toString(); | |
| } | |
| return this._contents; | |
| } |
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using async file existence check.
Using synchronous existsSync with async readFile creates an inconsistent pattern. Consider using async operations throughout:
-if (fsSync.existsSync(path.join(modulePath, "package.json"))) {
- const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"}));
+try {
+ const pkgContent = await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"});
+ const pkg = JSON.parse(pkgContent);
file = path.join(modulePath, pkg.main);
+} catch (error) {
+ if (error.code !== 'ENOENT') {
+ throw error;
+ }
}📝 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.
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | |
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | |
| try { | |
| const pkgContent = await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"}); | |
| const pkg = JSON.parse(pkgContent); | |
| file = path.join(modulePath, pkg.main); | |
| } catch (error) { | |
| if (error.code !== 'ENOENT') { | |
| throw error; | |
| } | |
| } |
| async importModule() { | ||
| const {modulePath} = this.module; | ||
| let file = path.join(modulePath, "index.js"); | ||
|
|
||
| if (fs.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = require(path.join(modulePath, "package.json")); | ||
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | ||
| file = path.join(modulePath, pkg.main); | ||
| } | ||
|
|
||
| if (mapExported.has(file)) { | ||
| return mapExported.get(file); | ||
| } | ||
|
|
||
| if (fs.existsSync(file)) { | ||
| mapExported.set(file, require(file)); | ||
| if (fsSync.existsSync(file)) { | ||
| mapExported.set(file, await import(file)); | ||
| return mapExported.get(file); | ||
| } |
There was a problem hiding this comment.
Add error handling for file operations.
The method lacks try-catch blocks for file operations which could fail. Consider adding proper error handling:
async importModule() {
const {modulePath} = this.module;
let file = path.join(modulePath, "index.js");
+ try {
if (fsSync.existsSync(path.join(modulePath, "package.json"))) {
const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"}));
file = path.join(modulePath, pkg.main);
}
if (mapExported.has(file)) {
return mapExported.get(file);
}
if (fsSync.existsSync(file)) {
mapExported.set(file, await import(file));
return mapExported.get(file);
}
return undefined;
+ } catch (error) {
+ throw new Error(`Failed to import module from ${file}: ${error.message}`);
+ }
}📝 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.
| async importModule() { | |
| const {modulePath} = this.module; | |
| let file = path.join(modulePath, "index.js"); | |
| if (fs.existsSync(path.join(modulePath, "package.json"))) { | |
| const pkg = require(path.join(modulePath, "package.json")); | |
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | |
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | |
| file = path.join(modulePath, pkg.main); | |
| } | |
| if (mapExported.has(file)) { | |
| return mapExported.get(file); | |
| } | |
| if (fs.existsSync(file)) { | |
| mapExported.set(file, require(file)); | |
| if (fsSync.existsSync(file)) { | |
| mapExported.set(file, await import(file)); | |
| return mapExported.get(file); | |
| } | |
| async importModule() { | |
| const {modulePath} = this.module; | |
| let file = path.join(modulePath, "index.js"); | |
| try { | |
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | |
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | |
| file = path.join(modulePath, pkg.main); | |
| } | |
| if (mapExported.has(file)) { | |
| return mapExported.get(file); | |
| } | |
| if (fsSync.existsSync(file)) { | |
| mapExported.set(file, await import(file)); | |
| return mapExported.get(file); | |
| } | |
| return undefined; | |
| } catch (error) { | |
| throw new Error(`Failed to import module from ${file}: ${error.message}`); | |
| } | |
| } |
eslint.config.mjs
Outdated
| parserOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module", | ||
| tsconfigRootDir: join(import.meta.dirname, "tsconfig.eslint.json") | ||
| }, |
There was a problem hiding this comment.
Fix potential tsconfig path resolution issue.
The current path resolution might not work correctly. Consider using the following approach:
- tsconfigRootDir: join(import.meta.dirname, "tsconfig.eslint.json")
+ project: "./tsconfig.eslint.json"📝 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.
| parserOptions: { | |
| ecmaVersion: "latest", | |
| sourceType: "module", | |
| tsconfigRootDir: join(import.meta.dirname, "tsconfig.eslint.json") | |
| }, | |
| parserOptions: { | |
| ecmaVersion: "latest", | |
| sourceType: "module", | |
| project: "./tsconfig.eslint.json" | |
| }, |
| ] | ||
| }, | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating TypeScript and JavaScript patterns.
For better maintainability, consider splitting the file patterns:
- files: ["**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"],
+ files: [
+ "**/*.{js,mjs,cjs,jsx,mjsx}", // JavaScript files
+ "**/*.{ts,tsx,mtsx}" // TypeScript files
+ ],📝 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.
| files: ["**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"], | |
| files: [ | |
| "**/*.{js,mjs,cjs,jsx,mjsx}", // JavaScript files | |
| "**/*.{ts,tsx,mtsx}" // TypeScript files | |
| ], |
| rules: { | ||
| "@typescript-eslint/ban-ts-comment": 0, | ||
| "@typescript-eslint/camelcase": 0, | ||
| "@typescript-eslint/no-inferrable-types": 0, | ||
| "@typescript-eslint/explicit-function-return-type": 0, | ||
| "@typescript-eslint/explicit-module-boundary-types": 0, | ||
| "@typescript-eslint/no-unused-vars": 0, | ||
| "@typescript-eslint/no-explicit-any": 0, | ||
| "@typescript-eslint/no-non-null-assertion": 0 | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Enable TypeScript ESLint rules for better code quality
The project heavily uses TypeScript as evidenced by:
- Numerous
.d.tsfiles in the codebase - TypeScript dependencies in package.json
- TypeScript parser and plugin configuration in ESLint
Consider enabling these essential rules with at least warning level (1):
@typescript-eslint/no-explicit-any: Prevents the use of unsafeanytype@typescript-eslint/no-unused-vars: Catches dead code and typos@typescript-eslint/explicit-module-boundary-types: Ensures better API documentation@typescript-eslint/no-non-null-assertion: Prevents potential runtime errors
The other disabled rules can remain at 0 if they conflict with your coding standards.
🔗 Analysis chain
Review disabled TypeScript rules.
Disabling all TypeScript rules (0) might lead to code quality issues. Consider:
- Enabling essential rules like
no-explicit-anyandno-unused-vars - Using warning level (
1) instead of completely disabling rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any TypeScript files in the project
fd -e ts -e tsx
# If TypeScript files exist, we should have stricter rules
if [ $? -eq 0 ]; then
echo "TypeScript files found. Consider enabling essential TypeScript rules."
fi
Length of output: 485
Script:
#!/bin/bash
# Let's check for TypeScript files more thoroughly
rg --type-list | grep -i typescript
echo "=== Checking for TypeScript files ==="
find . -type f -name "*.ts" -o -name "*.tsx" 2>/dev/null
echo -e "\n=== Checking package.json for TypeScript ==="
rg "typescript" package.json
echo -e "\n=== Checking for tsconfig.json ==="
find . -name "tsconfig*.json" 2>/dev/null
echo -e "\n=== Checking current ESLint configuration ==="
cat eslint.config.mjs
Length of output: 9609
| static async parse(docFile) { | ||
| const {symbols} = new DocParser(docFile.contents).parse(); | ||
|
|
||
| symbols.forEach((symbol) => { | ||
| symbol.setDocFile(docFile); | ||
| symbol = context.symbols.push(symbol); | ||
| docFile.symbols.set(symbol.symbolName, symbol); | ||
| }); | ||
| for (const [, symbol] of symbols) { | ||
| await symbol.setDocFile(docFile); | ||
| const newSymbol = context.symbols.push(symbol); | ||
| docFile.symbols.set(newSymbol.symbolName, newSymbol); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for async operations.
While the async implementation is correct, the method lacks proper error handling for potential failures in setDocFile or context operations.
Consider wrapping the async operations in a try-catch block:
static async parse(docFile) {
const {symbols} = new DocParser(docFile.contents).parse();
for (const [, symbol] of symbols) {
+ try {
await symbol.setDocFile(docFile);
const newSymbol = context.symbols.push(symbol);
docFile.symbols.set(newSymbol.symbolName, newSymbol);
+ } catch (error) {
+ console.error(`Failed to process symbol: ${error.message}`);
+ throw error;
+ }
}
return docFile.symbols;
}📝 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.
| static async parse(docFile) { | |
| const {symbols} = new DocParser(docFile.contents).parse(); | |
| symbols.forEach((symbol) => { | |
| symbol.setDocFile(docFile); | |
| symbol = context.symbols.push(symbol); | |
| docFile.symbols.set(symbol.symbolName, symbol); | |
| }); | |
| for (const [, symbol] of symbols) { | |
| await symbol.setDocFile(docFile); | |
| const newSymbol = context.symbols.push(symbol); | |
| docFile.symbols.set(newSymbol.symbolName, newSymbol); | |
| } | |
| static async parse(docFile) { | |
| const {symbols} = new DocParser(docFile.contents).parse(); | |
| for (const [, symbol] of symbols) { | |
| try { | |
| await symbol.setDocFile(docFile); | |
| const newSymbol = context.symbols.push(symbol); | |
| docFile.symbols.set(newSymbol.symbolName, newSymbol); | |
| } catch (error) { | |
| console.error(`Failed to process symbol: ${error.message}`); | |
| throw error; | |
| } | |
| } | |
| return docFile.symbols; | |
| } |
🛠️ Refactor suggestion
Consider batching symbol processing for performance.
The current implementation processes symbols sequentially. For large documentation files, this could be inefficient.
Consider using Promise.all for parallel processing:
static async parse(docFile) {
const {symbols} = new DocParser(docFile.contents).parse();
- for (const [, symbol] of symbols) {
- await symbol.setDocFile(docFile);
- const newSymbol = context.symbols.push(symbol);
- docFile.symbols.set(newSymbol.symbolName, newSymbol);
- }
+ await Promise.all(
+ Array.from(symbols.values()).map(async (symbol) => {
+ await symbol.setDocFile(docFile);
+ const newSymbol = context.symbols.push(symbol);
+ docFile.symbols.set(newSymbol.symbolName, newSymbol);
+ })
+ );
return docFile.symbols;
}📝 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.
| static async parse(docFile) { | |
| const {symbols} = new DocParser(docFile.contents).parse(); | |
| symbols.forEach((symbol) => { | |
| symbol.setDocFile(docFile); | |
| symbol = context.symbols.push(symbol); | |
| docFile.symbols.set(symbol.symbolName, symbol); | |
| }); | |
| for (const [, symbol] of symbols) { | |
| await symbol.setDocFile(docFile); | |
| const newSymbol = context.symbols.push(symbol); | |
| docFile.symbols.set(newSymbol.symbolName, newSymbol); | |
| } | |
| static async parse(docFile) { | |
| const {symbols} = new DocParser(docFile.contents).parse(); | |
| await Promise.all( | |
| Array.from(symbols.values()).map(async (symbol) => { | |
| await symbol.setDocFile(docFile); | |
| const newSymbol = context.symbols.push(symbol); | |
| docFile.symbols.set(newSymbol.symbolName, newSymbol); | |
| }) | |
| ); | |
| return docFile.symbols; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/ts-doc/src/models/DocFile.js (1)
64-71: Consider using a more robust caching mechanism.The global
mapExportedMap could lead to memory leaks and makes testing difficult. Consider:
- Moving cache to class level
- Adding cache invalidation
- Implementing a proper cache interface
class DocFile { + static #moduleCache = new Map(); + + static clearCache() { + this.#moduleCache.clear(); + } async importModule() { // ... - if (mapExported.has(file)) { - return mapExported.get(file); + if (DocFile.#moduleCache.has(file)) { + return DocFile.#moduleCache.get(file); } if (fsSync.existsSync(file)) { - mapExported.set(file, await import(file)); - return mapExported.get(file); + const module = await import(file); + DocFile.#moduleCache.set(file, module); + return module; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- eslint.config.mjs (1 hunks)
- packages/ts-doc/src/models/DocFile.js (3 hunks)
- packages/ts-doc/src/parsers/DocParser.js (2 hunks)
- packages/ts-doc/src/scan/scan.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eslint.config.mjs
- packages/ts-doc/src/parsers/DocParser.js
🔇 Additional comments (4)
packages/ts-doc/src/scan/scan.js (2)
56-56: LGTM! Async globby implementation.The conversion from
globby.syncto asyncglobbyis correct and addresses the event loop blocking concern.
58-69: 🛠️ Refactor suggestionEnhance error handling with file tracking.
While the async conversion is correct, consider implementing more robust error handling to track failed files and provide better feedback.
Apply this enhancement:
+ let failedFiles = 0; for (const file of files) { try { const symbols = await DocParser.parse(new DocFile(file)); symbols.forEach((symbol) => { context.logger(`Scanned symbol '${chalk.cyan(symbol.symbolName)}'`); symbolsSize++; }); } catch (er) { context.logger.error(chalk.red(er), er.stack); + context.logger.error(`Failed to parse file: ${chalk.red(file)}`); + failedFiles++; } } - context.logger(`${chalk.green(symbolsSize)} scanned symbols`); + context.logger( + `${chalk.green(symbolsSize)} scanned symbols, ` + + `${chalk.red(failedFiles)} files failed` + );packages/ts-doc/src/models/DocFile.js (2)
3-4: LGTM! Good separation of sync/async fs operations.The split between sync and async file system operations is well-structured, and the logger addition supports proper error handling.
Also applies to: 7-7
18-18: Previous review comment about lazy loading is still applicable.The synchronous file reading in the constructor could still benefit from lazy loading implementation as suggested in the previous review.
| @@ -49,21 +49,24 @@ module.exports = { | |||
| * | |||
| * @param patterns | |||
| */ | |||
| scanFiles(patterns) { | |||
| async scanFiles(patterns) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider converting other scanning methods to async.
While the async conversion of scanFiles is correct, consider converting scanComponents and scanTemplate methods to async as well for consistency, since they also use globby.
Apply this pattern to other methods:
- scanComponents(directory) {
+ async scanComponents(directory) {
context.logger("Scan components '" + chalk.cyan(directory) + "'");
- const files = globby.sync(path.join(directory, "**/*.ejs"));
+ const files = await globby(path.join(directory, "**/*.ejs"));
- scanTemplate(templatePattern) {
+ async scanTemplate(templatePattern) {
context.logger("Scan template directory '" + chalk.cyan(templatePattern) + "'");
- let files = globby.sync(templatePattern);
+ let files = await globby(templatePattern);Committable suggestion was skipped due to low confidence.
| } catch (er) { | ||
| logger.error("Fail to import module", {file, modulePath}); | ||
| } |
There was a problem hiding this comment.
Enhance error handling with specific error types.
The current error handling swallows all errors and only logs them. Consider:
- Creating specific error types
- Providing more context in error messages
- Allowing error propagation for critical failures
- } catch (er) {
- logger.error("Fail to import module", {file, modulePath});
+ } catch (error) {
+ logger.error("Failed to import module", {
+ file,
+ modulePath,
+ errorCode: error.code,
+ errorMessage: error.message
+ });
+ if (error.code !== 'MODULE_NOT_FOUND') {
+ throw new Error(`Failed to import module at ${file}: ${error.message}`);
+ }
}📝 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.
| } catch (er) { | |
| logger.error("Fail to import module", {file, modulePath}); | |
| } | |
| } catch (error) { | |
| logger.error("Failed to import module", { | |
| file, | |
| modulePath, | |
| errorCode: error.code, | |
| errorMessage: error.message | |
| }); | |
| if (error.code !== 'MODULE_NOT_FOUND') { | |
| throw new Error(`Failed to import module at ${file}: ${error.message}`); | |
| } | |
| } |
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | ||
| file = path.join(modulePath, pkg.main); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sync existence check with async implementation.
The mix of existsSync with async readFile creates an inconsistent pattern. Consider using fs.access or catching ENOENT from readFile.
-if (fsSync.existsSync(path.join(modulePath, "package.json"))) {
- const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"}));
- file = path.join(modulePath, pkg.main);
+try {
+ const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"}));
+ file = path.join(modulePath, pkg.main);
+} catch (error) {
+ if (error.code !== 'ENOENT') {
+ throw error;
+ }
+}📝 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.
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | |
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | |
| file = path.join(modulePath, pkg.main); | |
| } | |
| try { | |
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | |
| file = path.join(modulePath, pkg.main); | |
| } catch (error) { | |
| if (error.code !== 'ENOENT') { | |
| throw error; | |
| } | |
| } |
| async importModule() { | ||
| const {modulePath} = this.module; | ||
| let file = path.join(modulePath, "index.js"); | ||
| try { | ||
| if (fsSync.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = JSON.parse(await fs.readFile(path.join(modulePath, "package.json"), {encoding: "utf-8"})); | ||
| file = path.join(modulePath, pkg.main); | ||
| } | ||
|
|
||
| if (fs.existsSync(path.join(modulePath, "package.json"))) { | ||
| const pkg = require(path.join(modulePath, "package.json")); | ||
| file = path.join(modulePath, pkg.main); | ||
| } | ||
|
|
||
| if (mapExported.has(file)) { | ||
| return mapExported.get(file); | ||
| } | ||
| if (mapExported.has(file)) { | ||
| return mapExported.get(file); | ||
| } | ||
|
|
||
| if (fs.existsSync(file)) { | ||
| mapExported.set(file, require(file)); | ||
| return mapExported.get(file); | ||
| if (fsSync.existsSync(file)) { | ||
| mapExported.set(file, await import(file)); | ||
| return mapExported.get(file); | ||
| } | ||
| } catch (er) { | ||
| logger.error("Fail to import module", {file, modulePath}); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
ESM changes require additional configuration before implementation
The codebase currently uses CommonJS (require/module.exports) throughout, and there's no ESM configuration in package.json. Switching to import statements without proper setup will cause compatibility issues:
packages/ts-doc/src/scan/scan.jsandpackages/ts-doc/src/models/DocFile.spec.jsuse CommonJS requirespackages/ts-doc/src/models/DocFile.jsexports using CommonJS- Package.json lacks
"type": "module"field and ESM-related configurations
🔗 Analysis chain
Verify the impact of ESM changes across the codebase.
The switch from require to import might affect other parts of the codebase that depend on this module.
Let me gather more information about the ESM compatibility by checking the package.json and any other ESM-related configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for requires of this module
rg "require.*DocFile" -A 2
# Search for potential CommonJS interop issues
rg "module\.exports.*=.*DocFile" -A 2
Length of output: 576
Script:
#!/bin/bash
# Check package.json for type and exports configuration
cat packages/ts-doc/package.json
# Look for any ESM-specific configurations or imports
rg "type.*=.*module" packages/ts-doc/
rg "import.*from" packages/ts-doc/src/
Length of output: 2515
|
🎉 This PR is included in version 4.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes
package.jsonfor better clarity and functionality.Documentation
Chores