-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: support esm project to parse documentation #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import typescriptEslint from "@typescript-eslint/eslint-plugin"; | ||
| import typescriptParser from "@typescript-eslint/parser"; | ||
| import pluginPrettierRecommended from "eslint-plugin-prettier/recommended"; | ||
| import globals from "globals"; | ||
| import {join} from "node:path"; | ||
|
|
||
| export default [ | ||
| { | ||
| ignores: [ | ||
| "**/coverage", | ||
| "**/lib", | ||
| "**/dist", | ||
| "processes.config.js", | ||
| "**/snapshots", | ||
| "**/templates", | ||
| "**/docs/**", | ||
| "**/docs-references/**" | ||
| ] | ||
| }, | ||
| { | ||
| files: ["**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"], | ||
| languageOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module", | ||
| parser: typescriptParser, | ||
| parserOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module" | ||
| }, | ||
| globals: { | ||
| ...globals.node | ||
| } | ||
| }, | ||
| plugins: { | ||
| "@typescript-eslint": typescriptEslint | ||
| }, | ||
| 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 | ||
| } | ||
|
Comment on lines
+37
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Enable TypeScript ESLint rules for better code quality The project heavily uses TypeScript as evidenced by:
Consider enabling these essential rules with at least warning level (1):
The other disabled rules can remain at 0 if they conflict with your coding standards. 🔗 Analysis chainReview disabled TypeScript rules. Disabling all TypeScript rules (
🏁 Scripts executedThe 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 |
||
| }, | ||
| // { | ||
| // files: ["**/*.spec.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}"], // or any other pattern | ||
| // ignores: ["docs/**", "docs-references/**"], | ||
| // plugins: { | ||
| // vitest | ||
| // }, | ||
| // rules: { | ||
| // ...vitest.configs.recommended.rules, // you can also use vitest.configs.all.rules to enable all rules | ||
| // "vitest/consistent-test-it": ["error", {fn: "it", withinDescribe: "it"}], | ||
| // "vitest/no-alias-methods": "error", | ||
| // "vitest/expect-expect": "off" | ||
| // } | ||
| // }, | ||
| pluginPrettierRecommended | ||
| ]; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,10 @@ | ||||||||||||||||||||||||||||||
| const {context} = require("../context"); | ||||||||||||||||||||||||||||||
| const path = require("path"); | ||||||||||||||||||||||||||||||
| const fs = require("fs"); | ||||||||||||||||||||||||||||||
| const fs = require("fs/promises"); | ||||||||||||||||||||||||||||||
| const fsSync = require("fs"); | ||||||||||||||||||||||||||||||
| const normalizePath = require("normalize-path"); | ||||||||||||||||||||||||||||||
| const readPkgUp = require("read-pkg-up"); | ||||||||||||||||||||||||||||||
| const {logger} = require("../context/context"); | ||||||||||||||||||||||||||||||
| const mapExported = new Map(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class DocFile { | ||||||||||||||||||||||||||||||
|
|
@@ -13,7 +15,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| get path() { | ||||||||||||||||||||||||||||||
|
|
@@ -50,24 +52,26 @@ class DocFile { | |||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| requireModule() { | ||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace sync existence check with async implementation. The mix of -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
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 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}); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+72
to
74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance error handling with specific error types. The current error handling swallows all errors and only logs them. Consider:
- } 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
Suggested change
Comment on lines
+55
to
74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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 (
🔗 Analysis chainVerify 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 executedThe 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 |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,21 +49,24 @@ module.exports = { | |
| * | ||
| * @param patterns | ||
| */ | ||
| scanFiles(patterns) { | ||
| async scanFiles(patterns) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider converting other scanning methods to async. While the async conversion of 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);
|
||
| context.logger("Scan folders '" + chalk.cyan(JSON.stringify(patterns)) + "'"); | ||
|
|
||
| let symbolsSize = 0; | ||
| const files = await globby(patterns); | ||
|
|
||
| globby.sync(patterns).forEach((file) => { | ||
| 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); | ||
| } | ||
| }); | ||
| } | ||
|
Comment on lines
+58
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling and reporting. While the async/await conversion is correct, the error handling could be improved to:
+ 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`
+ );
|
||
|
|
||
| context.logger(`${chalk.green(symbolsSize)} scanned symbols`); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider separating TypeScript and JavaScript patterns.
For better maintainability, consider splitting the file patterns:
📝 Committable suggestion