feat: improve type generation#1662
Conversation
WalkthroughThis pull request focuses on restructuring type exports in the Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Learnings (2)crates/napi/scripts/generate-types.ts (2)crates/napi/index.d.ts (2)🔇 Additional comments (7)crates/napi/scripts/generate-types.ts (4)
The implementation correctly exports node types as TypeScript type definitions, which aligns with the expected behavior for type generation. Line range hint The changes effectively simplify the type structure while maintaining type safety:
Line range hint The implementation correctly uses type-only imports and follows TypeScript best practices.
The error handling is well-implemented:
crates/napi/index.d.ts (3)Line range hint The changes correctly simplify the return types while maintaining type safety, aligning with the type generation improvements. Line range hint The changes maintain consistency with other namespaces while simplifying the type definitions. Line range hint The changes maintain consistency and proper type definitions across all language namespaces (JSX, TypeScript, TSX, and CSS), following the same pattern of type simplification while preserving type safety. 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1662 +/- ##
=======================================
Coverage 87.11% 87.11%
=======================================
Files 96 96
Lines 15458 15458
=======================================
Hits 13467 13467
Misses 1991 1991 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/napi/index.d.ts(2 hunks)crates/napi/package.json(2 hunks)crates/napi/scripts/generate-types.ts(4 hunks)
🔇 Additional comments (2)
crates/napi/scripts/generate-types.ts (1)
141-141: 🛠️ Refactor suggestion
Update inserted export statements to match corrected syntax
At line 141, the insertedText includes the exportStatements. Ensure that the export statements use the correct syntax for re-exporting constants:
Revise the insertedText as follows to reflect the changes:
{
startPos: 0,
endPos: 0,
- insertedText: exportStatements.join("\n") + "\n",
+ insertedText: exportStatements.join("\n") + "\n",
}After applying the previous fix, the exportStatements variable will contain the correct export syntax.
Likely invalid or redundant comment.
crates/napi/package.json (1)
45-47: Verify the execution order of scripts involving type generation
The build:debug and prepublishOnly scripts have been updated to include yarn typegen:
"build:debug": "napi build --no-const-enum --platform && yarn typegen",
"prepublishOnly": "yarn typegen && napi prepublish -t npm --skip-gh-release",Ensure that yarn typegen runs after the build process if it depends on build artifacts. Verify that the type generation works as expected in both debug and prepublish scenarios.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Herrington Darkholme <2883231+HerringtonDarkholme@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/napi/scripts/generate-types.ts (1)
Line range hint
16-54: Consider architectural improvements for robustnessThe type generation implementation could benefit from the following improvements:
- Add request timeouts for network calls to prevent hanging:
const timeout = 30000; // 30 seconds const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeout); const nodeTypesResponse = await fetch(url, { signal: controller.signal }); clearTimeout(timeoutId);
- Consider parallelizing type generation for better performance:
await Promise.all( Object.entries(languagesNodeTypesUrls).map(([lang, urlTemplate]) => generateLangNodeType(lang, urlTemplate).catch(e => { console.error(`Error generating types for ${lang}:`, e); }) ) );
- Add retry logic for network failures:
const retry = async (fn: () => Promise<any>, attempts = 3) => { for (let i = 0; i < attempts; i++) { try { return await fn(); } catch (e) { if (i === attempts - 1) throw e; await new Promise(resolve => setTimeout(resolve, 1000 * Math.pow(2, i))); } } };Would you like me to provide a complete implementation of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/napi/scripts/generate-types.ts(4 hunks)
🔇 Additional comments (3)
crates/napi/scripts/generate-types.ts (3)
162-165: LGTM: Proper error handling implemented
The error handling implementation for the main function looks good. It properly catches errors, logs them, and exits with a non-zero status code on failure.
49-49:
Invalid TypeScript type assignment with JSON object
The current implementation attempts to assign a JSON object to a type alias, which is incorrect in TypeScript.
Apply this fix to correctly define the type:
-export type ${lang}NodeTypesMap = ${JSON.stringify(nodeTypeMap, null, 2)};
+export const ${lang}NodeTypesMap = ${JSON.stringify(nodeTypeMap, null, 2)} as const;
+export type ${lang}NodeTypesMapType = typeof ${lang}NodeTypesMap;128-129:
Incorrect re-export of types from modules
This issue is related to the previous type export problem. Once the node types are exported as constants, these re-exports need to be adjusted accordingly.
Apply this fix to correctly re-export the types:
const exportStatements = Object.keys(languagesNodeTypesUrls)
- .map((lang) => `export type { ${lang}NodeTypesMap } from "./types/${lang}-node-types";`)
+ .map((lang) => [
+ `export { ${lang}NodeTypesMap } from "./types/${lang}-node-types";`,
+ `export type { ${lang}NodeTypesMapType } from "./types/${lang}-node-types";`
+ ].join('\n'))There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/napi/index.d.ts (1)
Line range hint
188-206: Consider retaining specific node type information in function return types for enhanced type safety.The return types of functions like
parse,parseAsync, andfindInFileshave been generalized toSgRootandSgNode[]across various language namespaces (html,js,jsx,ts,tsx,css). While this simplifies the function signatures, it may reduce type safety and make it more challenging for users to work with language-specific node types.Consider parameterizing these functions with generic types or returning more specific types to retain detailed type information, which can help users leverage type checking and IntelliSense features in their development environment.
Also applies to: 210-228, 232-250, 254-272, 276-294, 298-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/napi/__test__/index.spec.ts(1 hunks)crates/napi/index.d.ts(8 hunks)crates/napi/scripts/constants.ts(1 hunks)crates/napi/scripts/generate-types.ts(5 hunks)crates/napi/scripts/rules.ts(1 hunks)crates/napi/types/node-types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/napi/scripts/rules.ts
- crates/napi/types/node-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/napi/scripts/generate-types.ts
🧰 Additional context used
📓 Learnings (1)
crates/napi/index.d.ts (2)
Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1662
File: crates/napi/index.d.ts:2-23
Timestamp: 2024-12-14T18:56:38.743Z
Learning: In TypeScript declaration files (`.d.ts`), constants cannot be exported; only types and declarations can be exported.
Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1662
File: crates/napi/scripts/generate-types.ts:128-129
Timestamp: 2024-12-14T18:57:47.685Z
Learning: In `crates/napi/scripts/generate-types.ts`, when re-exporting modules that only export types (e.g., `./types/${lang}-node-types`), use `export type { ... }` to correctly re-export the types in TypeScript.
🔇 Additional comments (2)
crates/napi/index.d.ts (1)
2-23: Exporting types correctly with export type statements.
The use of export type { ... } statements to re-export types from individual language node types is appropriate in TypeScript declaration files (.d.ts). This ensures that only type information is exported, adhering to TypeScript best practices.
crates/napi/scripts/constants.ts (1)
84-84: Verify that all references to languageLibs have been updated or removed.
The languageLibs constant has been removed. Please ensure that any references to languageLibs elsewhere in the codebase are also removed or updated to prevent potential runtime errors or broken functionality.
You can run the following script to search for any remaining references:
✅ Verification successful
No references to languageLibs found in the codebase
The comprehensive search across the codebase shows no remaining references to languageLibs. This indicates that all references have been properly removed along with the constant itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to 'languageLibs' in the codebase.
rg 'languageLibs' --type ts --type js
Length of output: 37
Script:
#!/bin/bash
# Let's try a more comprehensive search without type filters
# as the file might be in a different location or have a different extension
# First, let's use ripgrep without type filters
rg "languageLibs" -l
# Also search using ast-grep to catch any variable declarations or usages
ast-grep --pattern 'const languageLibs = $_'
# And search for any potential imports that might reference it
ast-grep --pattern 'import { $$$languageLibs$$$ } from $_'
Length of output: 128
| const byKind = sg.root().find(js.kind("template_string")) | ||
| t.is(node!.id(), byKind!.id()) | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
Replace row with line in range positions to match the expected API properties.
In the test case starting at line 397, the start and end positions use row instead of line. The API expects the property line for specifying positions. Using row may lead to incorrect behavior or test failures.
Apply this diff to correct the property names:
- range: {
- start: { row: 0, column: 16 },
- end: { row: 4, column: 1 },
- }
+ range: {
+ start: { line: 0, column: 16 },
+ end: { line: 4, column: 1 },
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I will address it later. This is an inconsistent naming in ast-grep node's interface and rule. Node has the naming line instead of row. And we should keep consistent naming here. cc @mohebifar
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai please open a follow-up GitHub issue for this inconsistency.
There was a problem hiding this comment.
@HerringtonDarkholme, I've created a GitHub issue to track this inconsistency: #1663.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Inconsistent naming: 'line' vs 'row' in ast-grep node's interface and rule" \
--body "There is an inconsistency between the use of \`line\` and \`row\` in the ast-grep node's interface and rule parameters. The node uses \`line\`, while the rule uses \`row\`, which may lead to confusion. This inconsistency should be addressed.
**References:**
- PR: https://github.com/ast-grep/ast-grep/pull/1662
- Comment: https://github.com/ast-grep/ast-grep/pull/1662#discussion_r1885373639
Reported by: @HerringtonDarkholme" \
--assignee HerringtonDarkholme
Length of output: 629
Summary by CodeRabbit
New Features
Bug Fixes
SgNodeclass for improved type definitions and functionality.Chores
build:debugscript in the package configuration for easier execution.prepublishOnlyscript to streamline the build process.