fix: various fix for napi build #1666
Conversation
WalkthroughThis pull request focuses on refactoring the TypeScript type generation and build scripts for 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)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)crates/napi/scripts/constants.ts (1)
The URL update improves consistency by using the official tree-sitter-grammars organization and supports versioning through tags. Let's verify the repository and its tags: ✅ Verification successfulRepository and tags verified successfully The tree-sitter-lua repository in tree-sitter-grammars organization:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the Lua grammar repository exists and has proper tags
# Check if the repository exists and fetch its tags
gh api \
-H "Accept: application/vnd.github+json" \
repos/tree-sitter-grammars/tree-sitter-lua/tags --jq '.[].name'
# Verify node-types.json exists in the expected path
curl -s -o /dev/null -w "%{http_code}" \
"https://raw.githubusercontent.com/tree-sitter-grammars/tree-sitter-lua/master/src/node-types.json"
Length of output: 406 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 #1666 +/- ##
=======================================
Coverage 87.13% 87.13%
=======================================
Files 96 96
Lines 15476 15476
=======================================
Hits 13485 13485
Misses 1991 1991 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
crates/napi/package.json (1)
54-54: Update @ast-grep/napi devDependency versionThe package version is 0.31.1, but the devDependency @ast-grep/napi is still at 0.31.0. Consider updating the devDependency version to match.
- "@ast-grep/napi": "0.31.0", + "@ast-grep/napi": "0.31.1",crates/napi/scripts/generateTypes.ts (1)
51-54: Consider adding cleanup on errorThe error handling could benefit from cleanup operations before exiting.
generateLangNodeTypes().catch((error) => { console.error('Error:', error); + // Attempt to clean up any partially generated files + console.log('Cleaning up...'); process.exit(1); });crates/napi/scripts/updateIndexDts.ts (1)
93-96: Improve error message detailThe error handling could provide more context about what failed during the update process.
updateIndexDts().catch((error) => { - console.error("Error in updateIndexDts:", error); + console.error("Failed to update index.d.ts definitions:", error); + console.error("Stack trace:", error.stack); process.exit(1); })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/napi/index.d.ts(1 hunks)crates/napi/package.json(1 hunks)crates/napi/scripts/generateTypes.ts(1 hunks)crates/napi/scripts/updateIndexDts.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/napi/index.d.ts
🧰 Additional context used
📓 Learnings (1)
crates/napi/scripts/generateTypes.ts (2)
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.
Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1662
File: crates/napi/scripts/generate-types.ts:49-49
Timestamp: 2024-12-14T18:57:37.142Z
Learning: In `crates/napi/scripts/generate-types.ts`, it's expected to assign the output of `JSON.stringify(nodeTypeMap, null, 2)` directly to a TypeScript type alias using `export type` syntax.
🔇 Additional comments (3)
crates/napi/package.json (2)
50-51: LGTM! Improved organization of type generation scripts
The separation of type generation (generateTypes.ts) and type definition updates (updateIndexDts.ts) shows good separation of concerns. This should make the type generation workflow more maintainable.
✅ Verification successful
LGTM! Clean separation of type generation concerns
The scripts show a well-structured separation of concerns:
generateTypes.tsfetches and generates node type definitions from tree-sitter language repositoriesupdateIndexDts.tsupdates type declarations in index.d.ts to handle generic type parameters
There are no circular dependencies between the scripts, and they serve distinct purposes in the type generation workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the relationship between type generation scripts
# Check for any circular dependencies or incorrect imports
# Check the content of both scripts
rg -A 5 "import.*from" scripts/generateTypes.ts scripts/updateIndexDts.ts
# Look for any shared functionality between the scripts
ast-grep --pattern 'function $name($$$) { $$$ }'
Length of output: 257
Script:
#!/bin/bash
# Let's first find the actual location of these scripts
fd -t f "generateTypes.ts|updateIndexDts.ts"
# Then check the package.json to understand the script setup
fd -t f "package.json" | xargs cat
Length of output: 13183
Script:
#!/bin/bash
# Now that we found the correct paths, let's examine the content of these scripts
cat crates/napi/scripts/generateTypes.ts crates/napi/scripts/updateIndexDts.ts
# Also check for any imports between these files
rg -A 5 "import.*from" crates/napi/scripts/generateTypes.ts crates/napi/scripts/updateIndexDts.ts
Length of output: 7077
45-46: Verify the impact of post-build type modifications
The addition of yarn patchDts to both build commands suggests type definition modifications after the build. Let's verify that this doesn't affect the build artifacts unexpectedly.
✅ Verification successful
Build process changes are safe and well-implemented
The patchDts script is a necessary post-build step that updates type definitions to ensure proper TypeScript type support. After examining the implementation:
-
updateIndexDts.tsperforms controlled modifications toindex.d.tsto:- Add required type imports
- Update class type parameters for better type inference
- Modify method signatures for improved type safety
-
The script runs after both debug and release builds to maintain consistent type definitions, and its changes are deterministic and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the changes made by patchDts script
# Check the content and structure of the type definition files
# Check if index.d.ts exists and is modified by the script
fd -e d.ts -x stat -c "%n: %y" {}
# Look for any potential error patterns in the build script
rg -A 3 "patchDts" scripts/
Length of output: 186
Script:
#!/bin/bash
# Let's try a different approach to verify the patchDts script and its impact
# First, check if the patchDts script exists in package.json
rg "patchDts" crates/napi/package.json -A 2
# Look for any TypeScript-related build or patch scripts
rg "\"scripts\":" crates/napi/package.json -A 15
# Check for any .d.ts files in the repository
fd ".d.ts$"
# Look for any TypeScript configuration
fd "tsconfig.json"
Length of output: 1196
Script:
#!/bin/bash
# Let's examine the TypeScript scripts that handle type definitions
# Check the content of the update script
rg -l "updateIndexDts.ts" crates/napi/
cat crates/napi/scripts/updateIndexDts.ts
# Check the content of the type generation script
cat crates/napi/scripts/generateTypes.ts
# Look at the current state of index.d.ts
cat crates/napi/index.d.ts
Length of output: 16484
crates/napi/scripts/updateIndexDts.ts (1)
Line range hint 32-41: Verify AST node existence before operations
The code assumes certain AST nodes exist with ! assertions. Add explicit checks to handle cases where nodes might be missing.
const sgNodeClass = root.find({
// @ts-expect-error temporary error
rule: createMatchClassDeclarationRule("SgNode"),
});
+
+if (!sgNodeClass) {
+ throw new Error("Failed to find SgNode class declaration in AST");
+}
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor