Fix cross-platform compatibility when bundling an app #109
Conversation
WalkthroughReplaces eager static parser paths across language packages with a lazy, cached resolver that prefers a prebuilt via Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as Consumer (require / import)
participant Module as language package (index.js)
participant Resolver as resolvePrebuild
participant FS as Filesystem
Consumer->>Module: access libraryPath
Module->>Resolver: resolvePrebuild(__dirname)
alt prebuild found
Resolver-->>Module: prebuiltPath
Module->>FS: stat(prebuiltPath)
FS-->>Module: exists
Module-->>Consumer: return prebuiltPath (cached)
else no prebuild
Resolver-->>Module: null
Module->>FS: stat(path.join(__dirname,'parser.so'))
alt local parser exists
FS-->>Module: exists
Module-->>Consumer: return local parser path (cached)
else none found
FS-->>Module: missing
Module-->>Consumer: throw Error("No parser found. Please ensure the parser is built or a prebuild is available.")
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
macabeus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/c/index.js (2)
27-27: Allow an override via env var for bundlers and custom layoutsSome bundlers or packaging setups may prefer to supply the exact native path. Add an opt-in env override without changing the public API.
+const overrideLibPath = process.env.AST_GREP_LANG_C_PARSER_PATH -const libPath = path.join(__dirname, getNativePath()) +const libPath = overrideLibPath || path.join(__dirname, getNativePath())
12-12: Minor style consistency: trailing semicolonsReturns on Line 12 and Line 18 end with semicolons while the rest do not. Align with the repository’s style (remove or add consistently).
- return 'prebuilds/prebuild-macOS-ARM64/parser.so'; + return 'prebuilds/prebuild-macOS-ARM64/parser.so' - return 'prebuilds/prebuild-Linux-X64/parser.so'; + return 'prebuilds/prebuild-Linux-X64/parser.so'Also applies to: 18-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/c/index.js(1 hunks)
🔇 Additional comments (1)
packages/c/index.js (1)
3-25: Confirm prebuild artifacts layoutI didn’t find a
packages/c/prebuildsdirectory or anyparser.*files in the source tree—those files are likely generated or published separately. Before applying the table-driven refactor forgetNativePath(), please:
- Inspect your built or published package (e.g. the installed
@…/parsermodule undernode_modules) and note the exact folder names (Windowsvs.windows,macOSvs.darwin, etc.), architecture suffixes (X64,x64,ARM64, etc.) and file extensions (.dll,.dylib,.so).- Ensure your
PLATFORMSandARCHESmappings inpackages/c/index.jsexactly match those artifact names.- Verify that
require(path)at runtime correctly resolves to the existing binary files.Without this check, changing to a generalized lookup risks breaking native library loading.
|
The resolution has already done in the postinstall script. https://github.com/ast-grep/langs/blob/main/scripts/setup/index.ts Because you are bundling the package using macos so the MacOS parser.so is copied. You can either try
To make this merge, it should include these
I am not sure if you want to pursue this approach since it is quite tiringsome. |
|
@HerringtonDarkholme Hmm... Got it. I think I can take it, yes! Just to make sure if I understand well the pending tasks:
By this way, we still keep the support for the platforms that we don't have a prebuilt file.
Hopefully, copilot will help me to update the logic for all the ~25 languages as soon as I have it working well for C 🙏 As soon as you confirm that my understanding is right, I start working on it. |
|
@HerringtonDarkholme I made the changes for |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
scripts/setup/index.ts (3)
37-48: Build may run in the wrong directory; set cwd and surface outputexecSync defaults to the current process cwd. In monorepos or when invoked from a different folder, this can run the wrong build script. Also, surfacing stdio helps diagnose build failures.
- execSync('npm run build') + execSync('npm run build', { cwd: dirname, stdio: 'inherit' })
64-79: Make resolvePrebuild resilient: handle alt dir patterns and file extensionsCurrent logic assumes prebuilds/prebuild-OS-ARCH/parser.so with capitalized OS/arch and .so on all platforms. To better match real-world layouts and Windows/macOS ext differences, try multiple directory patterns and extensions, and log raw platform/arch when not mapped.
-function resolvePrebuild(dir: string) { - const os = PLATFORM_MAP[process.platform] - const arch = ARCH_MAP[process.arch] - const prebuild = path.join( - dir, - 'prebuilds', - `prebuild-${os}-${arch}`, - 'parser.so', - ) - if (!os || !arch || !fs.existsSync(prebuild)) { - log(`no prebuild for ${os} ${arch}`) - return undefined - } - log(`found prebuild for ${os} ${arch}`) - return prebuild -} +function resolvePrebuild(dir: string) { + const rawOS = process.platform + const rawArch = process.arch + const os = PLATFORM_MAP[rawOS] ?? rawOS + const arch = ARCH_MAP[rawArch] ?? rawArch + + // Support both "prebuild-OS-ARCH" and "OS-ARCH" folder styles just in case. + const candidateDirs = [ + path.join(dir, 'prebuilds', `prebuild-${os}-${arch}`), + path.join(dir, 'prebuilds', `${os}-${arch}`), + path.join(dir, 'prebuilds', `${rawOS}-${rawArch}`), + ] + // Try common extensions per platform. Some repos ship .so across platforms; include that too. + const exts = + rawOS === 'win32' ? ['dll', 'so'] : + rawOS === 'darwin' ? ['dylib', 'so'] : + ['so'] + + for (const d of candidateDirs) { + for (const ext of exts) { + const p = path.join(d, `parser.${ext}`) + if (fs.existsSync(p)) { + log(`found prebuild for ${os} ${arch} at ${p}`) + return p + } + } + } + log(`no prebuild for ${rawOS} ${rawArch} under: ${candidateDirs.join(', ')}`) + return undefined +}
50-60: Fallback to raw process.platform/process.arch and/or detect prebuild dirsI ran your check — no prebuild directories or parser binaries were found in the repo.
- Location to change:
- scripts/setup/index.ts (around lines 50–60)
Suggested change (keep the explicit map but fall back to raw values and consider scanning prebuild dirs):
const PLATFORM_MAP: Record<string, string> = { darwin: 'macOS', linux: 'Linux', win32: 'Windows', } const ARCH_MAP: Record<string, string> = { x64: 'X64', arm64: 'ARM64', } // Use mapped value or fall back to the runtime value so unknown platforms/arches still work: const targetPlatform = PLATFORM_MAP[process.platform] ?? process.platform const targetArch = ARCH_MAP[process.arch] ?? process.arch // (Optional) Further harden by introspecting available prebuild directories at runtime // and selecting the best match (e.g., handle ia32, arm, musl, freebsd, etc.).Please apply the fallback and/or implement a simple prebuild-dir scan to avoid false "no prebuild" reports for unsupported-but-valid architectures or OS variants.
🧹 Nitpick comments (1)
scripts/setup/index.ts (1)
20-26: Early return on prebuild found is fine; include path to aid debuggingLog the resolved path so consumers know exactly which binary was used. The message grammar can also be tightened.
- log('prebuild found, do not need to build') + log('prebuild found at', prebuild, '- no build needed')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/c/index.js(1 hunks)scripts/setup/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/c/index.js
🔇 Additional comments (1)
scripts/setup/index.ts (1)
81-81: Exporting resolvePrebuild is a good moveThis enables runtime loaders (e.g., per-language packages) to share the same resolution logic.
|
Thanks! @macabeus I currently don't have time for a review but I will find some time later this weekend. Appreciate your help |
| return native; | ||
| } | ||
|
|
||
| throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); |
There was a problem hiding this comment.
this is a nice error message. if the user is on an unsupported platform and install the library using pnpm, this error message can direct them
|
Hi @macabeus this change is surprisingly short and beautiful. Please go ahead and I will merge after the other languages are done. |
|
@HerringtonDarkholme Done! |
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (18)
packages/yaml/index.js (1)
5-17: Mirror Swift feedback: error message and cross-platform fallbackSame comments apply here as in packages/swift/index.js about:
- Making the error include platform/arch and install hint.
- Considering .dll/.dylib fallback if postinstall doesn’t always emit parser.so.
- Potential centralization in @ast-grep/setup-lang.
packages/html/index.js (1)
5-17: Mirror Swift feedback: error message and cross-platform fallbackRecommend the same improvements as noted for packages/swift/index.js:
- More actionable error text.
- Optional .dll/.dylib fallback check.
- Consider centralizing the logic in @ast-grep/setup-lang.
packages/csharp/index.js (1)
5-17: Mirror Swift feedback: error message and cross-platform fallbackRecommend the same improvements (actionable error, optional .dll/.dylib fallback, centralizing logic).
packages/elixir/index.js (3)
16-17: Improve the error to guide users (see rust comment)Add platform/arch and remediation steps to the error string.
2-3: Ensure @ast-grep/setup-lang is listed as a runtime dependencyThis module now imports resolvePrebuild at runtime.
11-16: Align local-build fallback with platform-specific extension (see rust comment)Same concern as in packages/rust/index.js: use .dll/.dylib/.so based on process.platform so local builds on Windows/macOS are discovered.
packages/lua/index.js (3)
16-17: Make error messages actionableAdopt the enhanced message suggested in packages/rust/index.js to help users recover when neither prebuild nor local parser exists.
2-3: Dependency check: @ast-grep/setup-langEnsure this package declares @ast-grep/setup-lang in dependencies/optionalDependencies.
11-16: Use platform-appropriate extension for local fallback (dll/dylib/so)Replicate the platform-aware extension logic suggested in packages/rust/index.js to make the local-build fallback work across OSes.
packages/javascript/index.js (3)
16-17: Enhance error message with platform and guidanceAdopt the actionable error style proposed in packages/rust/index.js.
2-3: Declare @ast-grep/setup-lang as a runtime dependencyThis import needs to be present in package.json dependencies.
11-16: Platform-specific extension for local fallback is neededAs noted in other files, hardcoding parser.so breaks local fallback on Windows/macOS. Compute the extension.
packages/ruby/index.js (3)
16-17: Actionable error would improve UXInclude platform, arch, and recovery steps.
2-3: Runtime dep check: @ast-grep/setup-langPlease ensure dependency is declared.
11-16: Fix local fallback for Windows/macOSUse a platform-based extension to discover locally built parser.{dll|dylib|so}.
packages/markdown/index.js (1)
2-4: Dependency check mirrors css.Ensure @ast-grep/setup-lang is listed under dependencies for packages/markdown.
Same script as in packages/css applies; just change the path to packages/markdown/package.json.
packages/toml/index.js (1)
2-4: Dependency check mirrors css.Confirm @ast-grep/setup-lang is a runtime dependency of packages/toml.
Same validation approach as for packages/css/package.json.
packages/python/index.js (1)
2-4: Dependency check mirrors css.Verify packages/python includes @ast-grep/setup-lang in dependencies.
Use the same grep-based check against packages/python/package.json.
🧹 Nitpick comments (22)
README.md (1)
76-78: Clarify grammar and subject; use “prebuilt parser”Minor grammar/clarity fix to avoid ambiguity and match terminology elsewhere.
Apply this diff:
-If there is prebuild available, it does nothing. -If there is no prebuild available, it will build the dynamic library from the tree-sitter grammar files. +If a prebuilt parser is available, the postinstall script does nothing. +If no prebuilt parser is available, it will build the dynamic library from the tree-sitter grammar files.packages/swift/index.js (1)
7-16: Style nit: semicolon consistencyThis file mixes styles (top-level requires without semicolons; returns/assignments with semicolons). Consider removing semicolons here to match the existing style in this file and others.
Apply this diff:
- return prebuild; + return prebuild @@ - const native = path.join(__dirname, 'parser.so'); + const native = path.join(__dirname, 'parser.so') @@ - return native; + return native @@ - throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.') @@ - libPath = getLibPath(); + libPath = getLibPath() @@ - return libPath; + return libPathAlso applies to: 23-26
packages/yaml/index.js (1)
7-16: Semicolon style consistencyMatch the file’s prevailing no-semicolon style.
Apply this diff:
- return prebuild; + return prebuild @@ - const native = path.join(__dirname, 'parser.so'); + const native = path.join(__dirname, 'parser.so') @@ - return native; + return native @@ - throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.') @@ - libPath = getLibPath(); + libPath = getLibPath() @@ - return libPath; + return libPathAlso applies to: 23-26
packages/html/index.js (1)
7-16: Nit: semicolon consistencyAlign with the file’s no-semicolon style.
Apply this diff:
- return prebuild; + return prebuild @@ - const native = path.join(__dirname, 'parser.so'); + const native = path.join(__dirname, 'parser.so') @@ - return native; + return native @@ - throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.') @@ - libPath = getLibPath(); + libPath = getLibPath() @@ - return libPath; + return libPathAlso applies to: 23-26
packages/csharp/index.js (1)
7-16: Nit: semicolon consistencyKeep style consistent within the file.
Apply this diff:
- return prebuild; + return prebuild @@ - const native = path.join(__dirname, 'parser.so'); + const native = path.join(__dirname, 'parser.so') @@ - return native; + return native @@ - throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.') @@ - libPath = getLibPath(); + libPath = getLibPath() @@ - return libPath; + return libPathAlso applies to: 23-26
packages/scala/index.js (1)
16-16: Make the error more actionable (include platform/arch and package).Adding platform and package context helps users debug missing prebuilds/postinstall failures faster.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${path.basename(__dirname)}. Ensure postinstall ran or a matching prebuild exists.`);packages/tsx/index.js (1)
16-16: Improve error messaging with platform/arch context.- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${path.basename(__dirname)}. Ensure postinstall ran or a matching prebuild exists.`);packages/bash/index.js (1)
16-16: Add context to the error for easier debugging.- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${path.basename(__dirname)}. Ensure postinstall ran or a matching prebuild exists.`);packages/typescript/index.js (2)
16-16: Clarify error with platform/arch and package name.- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${path.basename(__dirname)}. Ensure postinstall ran or a matching prebuild exists.`);
5-17: Consider centralizing this resolver to remove duplication across ~25 packages.A single helper in @ast-grep/setup-lang (e.g., getNativePath(dir): string) that encapsulates:
- prebuild resolution,
- local parser.so fallback,
- standardized error,
would reduce maintenance and ensure consistent behavior across languages and templates.packages/java/index.js (1)
16-16: Enhance error message for quicker diagnostics.- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${path.basename(__dirname)}. Ensure postinstall ran or a matching prebuild exists.`);packages/haskell/index.js (2)
16-16: Make error actionable with platform/arch and tried pathsImprove diagnosability by including platform/arch and the attempted local path. This helps users/bundlers understand what failed.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch}. Searched prebuilds and "${native}". If you are bundling (e.g. VS Code), ensure postinstall ran to build a local parser or include matching prebuilds.`);
5-17: Reduce duplication by centralizing resolver logicThis resolver is duplicated across ~25 packages. Consider exposing a single helper from @ast-grep/setup-lang, e.g., resolveLibraryPath(__dirname), that implements:
- prebuild-first lookup,
- local parser fallback,
- standardized, actionable error message.
This will keep behavior consistent and make future changes one-liners per package.
packages/php/index.js (1)
16-16: Improve the error with platform/arch and next stepsProvide actionable context to users and bundlers.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch}. Searched prebuilds and "${native}". If you are bundling (e.g. VS Code), ensure postinstall ran to build a local parser or include matching prebuilds.`);packages/dart/index.js (1)
16-16: Enhance error message for debuggingAdd platform/arch and the attempted path to aid users.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch}. Searched prebuilds and "${native}". If you are bundling (e.g. VS Code), ensure postinstall ran to build a local parser or include matching prebuilds.`);packages/json/index.js (1)
16-16: Provide more context in the error messageSmall improvement for supportability.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch}. Searched prebuilds and "${native}". If you are bundling (e.g. VS Code), ensure postinstall ran to build a local parser or include matching prebuilds.`);packages/go/index.js (1)
16-16: Make the thrown error more actionableInclude platform/arch and attempted path.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch}. Searched prebuilds and "${native}". If you are bundling (e.g. VS Code), ensure postinstall ran to build a local parser or include matching prebuilds.`);packages/cpp/index.js (1)
16-16: Improve error message with actionable context (optional)Include platform/arch and attempted locations to aid debugging.
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + throw new Error(`No parser found for ${process.platform}-${process.arch} in ${__dirname}. +Tried prebuilds and ${native}. Ensure postinstall ran or build a parser.so for this platform.`);packages/kotlin/index.js (1)
5-17: Deduplicate getLibPath logic across packages (optional)This helper is copy-pasted across ~25 packages. Consider exposing a shared getLibPath(cwd) from @ast-grep/setup-lang to remove duplication and ensure consistent behavior/errors.
packages/rust/index.js (2)
16-17: Make the error actionable and self-identifyingInclude platform/arch and a suggestion to build or install prebuilds, so users know how to fix it in unsupported environments or when postinstall is skipped.
Apply this diff:
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); + const lang = path.basename(__dirname); + throw new Error( + `@ast-grep/lang-${lang}: no parser found for ${process.platform}-${process.arch}. ` + + `Try one of:\n` + + ` 1) npx @ast-grep/setup-lang postinstall\n` + + ` 2) reinstall to trigger postinstall build on this platform\n` + + ` 3) install a matching prebuilt artifact` + );
5-17: Reduce duplication: centralize path resolution in setup packageThis getLibPath is duplicated across ~25 packages. Consider exporting a resolveLibraryPath(__dirname) helper from @ast-grep/setup-lang that encapsulates:
- prebuild-first detection,
- platform-specific local filename,
- actionable error message.
This cuts maintenance overhead and keeps behavior consistent across languages.
packages/css/index.js (1)
5-17: Avoid duplicating resolver logic across ~25 packages.Consider hoisting this logic into @ast-grep/setup-lang (e.g., export a getNativePath(dirname): string) so each language can just call it. This reduces maintenance overhead and keeps behavior uniform.
Happy to sketch the shared helper API and a codemod to update all packages in one sweep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
README.md(1 hunks)packages/angular/index.js(1 hunks)packages/bash/index.js(1 hunks)packages/cpp/index.js(1 hunks)packages/csharp/index.js(1 hunks)packages/css/index.js(1 hunks)packages/dart/index.js(1 hunks)packages/elixir/index.js(1 hunks)packages/go/index.js(1 hunks)packages/haskell/index.js(1 hunks)packages/html/index.js(1 hunks)packages/java/index.js(1 hunks)packages/javascript/index.js(1 hunks)packages/json/index.js(1 hunks)packages/kotlin/index.js(1 hunks)packages/lua/index.js(1 hunks)packages/markdown/index.js(1 hunks)packages/php/index.js(1 hunks)packages/python/index.js(1 hunks)packages/ruby/index.js(1 hunks)packages/rust/index.js(1 hunks)packages/scala/index.js(1 hunks)packages/sql/index.js(1 hunks)packages/swift/index.js(1 hunks)packages/toml/index.js(1 hunks)packages/tsx/index.js(1 hunks)packages/typescript/index.js(1 hunks)packages/yaml/index.js(1 hunks)scripts/create-lang/template/index.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (20)
packages/rust/index.js (3)
packages/bash/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/kotlin/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)
packages/toml/index.js (2)
packages/bash/index.js (3)
prebuild(6-6)path(1-1)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/python/index.js (2)
packages/markdown/index.js (4)
prebuild(6-6)native(11-11)path(1-1)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/tsx/index.js (4)
packages/lua/index.js (4)
fs(2-2)prebuild(6-6)native(11-11)libPath(19-19)packages/tsx/nursery.js (2)
require(1-1)path(4-4)packages/tsx/postinstall.js (1)
require(1-1)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/dart/index.js (17)
packages/bash/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/angular/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/csharp/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/cpp/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/haskell/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/css/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/html/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/elixir/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/java/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/json/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/markdown/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/kotlin/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/php/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/dart/postinstall.js (1)
require(1-1)packages/dart/nursery.js (1)
require(1-1)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/yaml/index.js (1)
scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/css/index.js (1)
packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)
packages/javascript/index.js (1)
scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/markdown/index.js (2)
packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/php/index.js (4)
fs(2-2)prebuild(6-6)native(11-11)libPath(19-19)
packages/scala/index.js (2)
packages/kotlin/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)
packages/bash/index.js (1)
packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)
packages/swift/index.js (1)
scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/html/index.js (3)
packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/php/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
scripts/create-lang/template/index.js (1)
scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/cpp/index.js (3)
packages/json/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/cpp/postinstall.js (1)
require(1-1)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/elixir/index.js (3)
packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/php/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/java/index.js (2)
packages/javascript/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/lua/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)
packages/angular/index.js (4)
packages/html/index.js (6)
fs(2-2)require(3-3)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/markdown/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/kotlin/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/php/index.js (2)
packages/lua/index.js (4)
fs(2-2)prebuild(6-6)native(11-11)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
packages/typescript/index.js (3)
packages/kotlin/index.js (5)
fs(2-2)prebuild(6-6)native(11-11)path(1-1)libPath(19-19)packages/lua/index.js (4)
fs(2-2)prebuild(6-6)native(11-11)libPath(19-19)scripts/setup/index.ts (1)
resolvePrebuild(81-81)
🪛 LanguageTool
README.md
[grammar] ~76-~76: There might be a mistake here.
Context: ...e @ast-grep/lang-* packages. If there is prebuild available, it does nothing. If...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ... is prebuild available, it does nothing. If there is no prebuild available, it wi...
(QB_NEW_EN)
🪛 GitHub Actions: CI
packages/bash/index.js
[error] 6-6: TypeError: resolvePrebuild is not a function during test step 'node nursery.js test' (bash package).
packages/cpp/index.js
[error] 6-6: TypeError: resolvePrebuild is not a function during test step 'node nursery.js test' (cpp package).
packages/angular/index.js
[error] 6-6: TypeError: resolvePrebuild is not a function during test step 'node nursery.js test' (angular package).
🔇 Additional comments (42)
packages/swift/index.js (2)
21-27: Getter + memoization for libraryPath looks goodLazy resolution with caching avoids unnecessary I/O and plays well with bundlers that only access the property at runtime.
11-14: No change required — repository expects parser.soVerification: scripts/setup/index.ts explicitly lists 'parser.so' (line 71) and no parser.dll or parser.dylib were found under prebuilds/ or packages/. The hard-coded parser.so fallback in packages/swift/index.js matches the repo's setup, so the suggested OS-specific fallback is not required.
Files of interest:
- scripts/setup/index.ts:71 — contains 'parser.so'
- packages/swift/index.js (lines 11–14) — currently uses parser.so
packages/yaml/index.js (1)
21-27: LGTM: lazy getter + cached resolutionPattern matches the rest of the repo and avoids early path resolution during bundling.
packages/html/index.js (1)
21-27: Approach approvedThe lazy getter with memoization is appropriate and consistent with the cross-package pattern.
packages/csharp/index.js (1)
21-27: Getter + cache is solidMatches the repo’s pattern and defers I/O until actually needed.
packages/scala/index.js (1)
22-27: Lazy getter + caching LGTM.The lazy resolution via a getter and caching into libPath is clean and avoids build-time bundler capture.
packages/tsx/index.js (1)
22-27: Getter + memoization looks good.This avoids premature resolution during bundling.
packages/bash/index.js (1)
22-27: Lazy getter pattern approved.Once the guard is added, the runtime behavior is as intended.
packages/typescript/index.js (1)
22-27: Getter + memoization LGTM.packages/java/index.js (1)
22-27: Library path getter and caching are solid.packages/haskell/index.js (2)
5-17: LGTM: robust lazy resolution with prebuild-first fallback and cachingThe getter-based lazy resolution and prebuild-first strategy look correct and consistent with the PR’s goals. Caching avoids repeated fs checks.
Also applies to: 22-27
11-14: Confirmed — repo standardizes on parser.so; no OS-specific fallback neededRepo-wide search shows parser.so is used in package build scripts and index.js files; no occurrences of parser.dll or parser.dylib were found. Keep the current code as-is.
packages/php/index.js (3)
5-17: LGTM: prebuild-aware lazy resolution and caching are correctMatches the desired runtime detection behavior for bundled apps and avoids repeated fs checks.
Also applies to: 22-27
2-3: Dependency check for @ast-grep/setup-lang — OKConfirmed packages/php/package.json lists "@ast-grep/setup-lang" in dependencies at version "0.0.3"; no runtime dependency change needed.
- packages/php/package.json: dependencies["@ast-grep/setup-lang"] = "0.0.3"
11-14: Verify extension uniformity (parser.so vs OS-specific) — no action requiredI searched the repo for parser.(so|dll|dylib). Only parser.so references were found (build scripts and index.js files across packages use
tree-sitter build -o parser.so), so the check in packages/php/index.js is correct.Examples:
- scripts/setup/index.ts:71 —
'parser.so'- packages/php/index.js:11 —
const native = path.join(__dirname, 'parser.so');- many packages' package.json build scripts use
tree-sitter build -o parser.soSnippet in packages/php/index.js (unchanged and valid):
const native = path.join(__dirname, 'parser.so'); if (fs.existsSync(native)) { return native; }packages/dart/index.js (3)
5-17: LGTM: consistent lazy resolver with cachingThe implementation aligns with the cross-platform runtime detection requirements and mirrors other languages.
Also applies to: 22-27
2-3: Dependency verified — no action neededpackages/dart/package.json declares "@ast-grep/setup-lang": "0.0.3", so the runtime dependency is present.
- packages/dart/index.js — lines 2-3: requires '@ast-grep/setup-lang'
- packages/dart/package.json — dependencies include "@ast-grep/setup-lang": "0.0.3"
11-14: No action required — repo consistently uses parser.soI searched for parser.(so|dll|dylib) and only parser.so appears in build scripts and runtime loaders. Build commands use
tree-sitter build -o parser.soand index.js files load'parser.so'(e.g. packages/dart/package.json & packages/dart/index.js, packages/python/index.js, packages/yaml/package.json), so an OS-specific fallback is not needed.packages/json/index.js (3)
5-17: LGTM: prebuild-first lazy resolution with cacheMatches the repo-wide pattern and solves the bundling issue at runtime.
Also applies to: 22-27
2-3: @ast-grep/setup-lang dependency present — no action requiredVerified that packages/json/package.json declares @ast-grep/setup-lang at version 0.0.3.
- File: packages/json/package.json
- Dependency: "@ast-grep/setup-lang": "0.0.3"
11-14: No change required — repo consistently usesparser.soripgrep shows the repo consistently builds and loads a single filename:
parser.so. Examples:
- packages/*/index.js (e.g. packages/json/index.js, packages/typescript/index.js) — load path.join(__dirname, 'parser.so')
- packages/*/package.json — "build": "tree-sitter build -o parser.so"
- scripts/setup/index.ts — references 'parser.so'
Action: keep as-is. Add OS-specific fallbacks (e.g. .dll/.dylib) only if you intend to support platform-specific prebuilt binaries.
packages/go/index.js (3)
5-17: LGTM: coherent lazy resolution patternCorrectly prioritizes prebuilds, falls back to local parser, and caches the resolved path.
Also applies to: 22-27
2-3: Dependency present — no action requiredVerified packages/go/package.json includes @ast-grep/[email protected], so the runtime import in packages/go/index.js is satisfied.
- File checked: packages/go/package.json — @ast-grep/setup-lang: 0.0.3
- Import location: packages/go/index.js (uses resolvePrebuild from '@ast-grep/setup-lang')
11-14: No change required — repo consistently builds/loads parser.soGrepped the repo for parser.(so|dll|dylib). Only occurrences of "parser.so" were found (build outputs and index.js loaders); no .dll or .dylib files/refs exist. The current .so-only fallback is consistent across the codebase.
Examples:
- scripts/setup/index.ts
- packages/*/index.js (e.g., packages/go/index.js, packages/javascript/index.js)
- packages/*/package.json build scripts (tree-sitter build -o parser.so)
packages/rust/index.js (4)
5-9: Prebuild-first resolution is appropriate for bundled, multi-arch scenariosPreferring resolvePrebuild(__dirname) at runtime aligns with the PR objective and avoids bundlers picking an incompatible binary at build time. Good move.
22-27: Lazy getter + cache keeps module load side-effect freeUsing a getter to compute once and cache avoids early fs work and keeps bundling safer. Looks good.
11-16: Keep parser.so — the repo intentionally normalizes the native nameThe codebase/build scripts always use "parser.so" as the canonical artifact name, so computing platform-specific extensions would be incorrect.
- scripts/setup/index.ts — resolvePrebuild builds a prebuild path with 'parser.so' (line ~71).
- packages/*/package.json — many packages build with
tree-sitter build -o parser.so.- packages/*/index.js — language packages (e.g. packages/rust/index.js, packages/javascript/index.js, packages/python/index.js, etc.) check for
path.join(__dirname, 'parser.so').Do not apply the suggested diff; ignore this review suggestion.
Likely an incorrect or invalid review comment.
2-3: No action needed — @ast-grep/setup-lang is declared as a runtime dependencyVerified: all packages that import "@ast-grep/setup-lang" list it under dependencies (runtime), so consumers should not hit MODULE_NOT_FOUND.
File/context checked:
- packages/rust/index.js (lines 2-3)
const fs = require('node:fs')
const { resolvePrebuild } = require('@ast-grep/setup-lang')packages/elixir/index.js (2)
5-9: Prebuild-first resolution matches bundling needsThis mirrors the agreed approach and avoids bundler-time architecture pinning. Looks good.
22-27: Getter + cache pattern LGTMThe lazy resolution and caching are appropriate.
packages/lua/index.js (2)
5-9: Runtime prebuild resolution is correctGood shift to lazy, prebuild-first detection to support bundled multi-arch extensions.
22-27: Lazy getter pattern LGTMCaching after first resolution avoids repeated fs checks.
packages/javascript/index.js (2)
5-9: Prebuild-first resolution is appropriateMatches the desired runtime selection and unblocks cross-arch bundling.
22-27: Getter + cache LGTMNon-eager resolution is desirable here.
packages/ruby/index.js (2)
5-9: LGTM: prebuild-first dynamic resolutionConsistent with the broader approach and solves the original issue.
22-27: Lazy getter pattern LGTMThe caching behavior is correct and minimal.
packages/css/index.js (3)
22-27: LGTM: lazy getter and memoization are appropriate.The getter defers environment detection to runtime and caches the resolved path; this directly addresses bundler cross-arch issues.
2-4: Dependency present — no action required: @ast-grep/setup-lang is declared in packages/css/package.jsonVerified: packages/css/package.json references "@ast-grep/setup-lang". No change needed for this package.
11-14: Confirmed: repo consistently expects parser.so — please verify this matches your target platformsFindings (brief):
- All language packages use a fallback of path.join(__dirname, 'parser.so') (e.g., packages/css/index.js, packages/javascript/index.js).
- package.json build scripts across packages use: "tree-sitter build -o parser.so".
- scripts/setup/index.ts resolvePrebuild looks for prebuilds/.../parser.so.
Actionable note:
- If your build/postinstall guarantees a parser.so on every platform you support, no code change is needed.
- Otherwise, update resolvePrebuild/getLibPath to derive the filename by platform or fall back to searching for parser.* (safer for mixed/unsupported platforms).
packages/markdown/index.js (1)
22-27: LGTM: the lazy getter solution is consistent and correct.packages/toml/index.js (1)
22-27: LGTM: memoized getter is the right call here.packages/python/index.js (1)
22-27: LGTM: lazy property + cache matches the cross-platform goal.
| const fs = require('node:fs') | ||
| const { resolvePrebuild } = require('@ast-grep/setup-lang') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix CI: guard against missing resolvePrebuild export from @ast-grep/setup-lang
CI shows "TypeError: resolvePrebuild is not a function". Make the import resilient to older setup-lang builds by avoiding destructuring and falling back to a no-op when the export is absent.
Apply:
-const fs = require('node:fs')
-const { resolvePrebuild } = require('@ast-grep/setup-lang')
+const fs = require('node:fs')
+const setupLang = require('@ast-grep/setup-lang')
+const resolvePrebuild = typeof setupLang.resolvePrebuild === 'function' ? setupLang.resolvePrebuild : () => nullVerification (optional):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check that resolvePrebuild is exported in scripts/setup/index.ts"
rg -n "export\s+(function|const)\s+resolvePrebuild" scripts/setup/index.ts || {
echo "resolvePrebuild not exported from scripts/setup/index.ts"; exit 1; }
echo "Find any remaining destructured imports that can throw at runtime"
rg -n "const\s*\{\s*resolvePrebuild\s*\}\s*=\s*require\('@ast-grep/setup-lang'\)"Length of output: 367
Fix CI: guard against missing resolvePrebuild export from @ast-grep/setup-lang
CI fails with "TypeError: resolvePrebuild is not a function". Verification shows resolvePrebuild is not exported from scripts/setup/index.ts (rg reported "resolvePrebuild not exported from scripts/setup/index.ts"). Make the import resilient by avoiding destructured require and falling back to a no-op.
Files to change:
- packages/cpp/index.js — replace the destructured import (lines ~2-3) with a safe require:
-const fs = require('node:fs')
-const { resolvePrebuild } = require('@ast-grep/setup-lang')
+const fs = require('node:fs')
+const setupLang = require('@ast-grep/setup-lang')
+const resolvePrebuild = typeof setupLang.resolvePrebuild === 'function' ? setupLang.resolvePrebuild : () => null- scripts/setup/index.ts — currently does not export resolvePrebuild; either export it if intended to be a public API, or keep the consumer-side guard above (recommended).
📝 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.
| const fs = require('node:fs') | |
| const { resolvePrebuild } = require('@ast-grep/setup-lang') | |
| const fs = require('node:fs') | |
| const setupLang = require('@ast-grep/setup-lang') | |
| const resolvePrebuild = typeof setupLang.resolvePrebuild === 'function' ? setupLang.resolvePrebuild : () => null |
🤖 Prompt for AI Agents
In packages/cpp/index.js around lines 2 to 3, the destructured require of
resolvePrebuild can throw when @ast-grep/setup-lang doesn't export it; change
the import to require the module as a whole and guard by assigning
resolvePrebuild to the module's export if present and a function, otherwise fall
back to a no-op function. Update the top of the file to use that guarded
resolvePrebuild value so callers won't crash in CI; optionally add a brief
comment explaining the fallback. Ensure no other parts of the file assume
resolvePrebuild is always present without checking.
| return native; | ||
| } | ||
|
|
||
| throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the error to aid debugging across platforms and PNPM v10 installs
Make the error actionable by including platform/arch, the attempted local path, and a hint about PNPM’s postinstall allowlist.
Apply this diff:
- throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.');
+ throw new Error(`No parser found for ${process.platform}-${process.arch}. Tried prebuilds and ${native}. If using pnpm >= 10, ensure postinstall ran (see README for --allow-build usage).`);📝 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.
| throw new Error('No parser found. Please ensure the parser is built or a prebuild is available.'); | |
| throw new Error(`No parser found for ${process.platform}-${process.arch}. Tried prebuilds and ${native}. If using pnpm >= 10, ensure postinstall ran (see README for --allow-build usage).`); |
🤖 Prompt for AI Agents
In packages/swift/index.js around line 16, the thrown Error is too generic;
update it to include runtime details and actionable hints by constructing the
error message with platform and architecture (process.platform and
process.arch), the exact local path you attempted to load (build the path with
__dirname and the expected parser filename), and a short note suggesting PNPM
v10 users may need to add the postinstall script to PNPM’s allowlist (or check
that prebuilds exist). Replace the static message with one that concatenates
these values so the error shows platform/arch, the resolved path attempted, and
the PNPM postinstall hint.
| function getLibPath() { | ||
| const prebuild = resolvePrebuild(__dirname) | ||
| if (prebuild) { | ||
| return prebuild; | ||
| } |
There was a problem hiding this comment.
Avoid runtime TypeError when resolvePrebuild is absent.
Same concern as other packages: guard the call so we can fall back to the local parser without crashing when resolvePrebuild isn’t exported.
- const prebuild = resolvePrebuild(__dirname)
+ const prebuild = typeof resolvePrebuild === 'function' ? resolvePrebuild(__dirname) : undefined
if (prebuild) {
return prebuild;
}📝 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.
| function getLibPath() { | |
| const prebuild = resolvePrebuild(__dirname) | |
| if (prebuild) { | |
| return prebuild; | |
| } | |
| function getLibPath() { | |
| const prebuild = typeof resolvePrebuild === 'function' ? resolvePrebuild(__dirname) : undefined | |
| if (prebuild) { | |
| return prebuild; | |
| } |
🤖 Prompt for AI Agents
In packages/tsx/index.js around lines 5 to 9, the code calls
resolvePrebuild(__dirname) unguarded which will throw a runtime TypeError if
resolvePrebuild is not exported; modify the code to check that resolvePrebuild
is a function before calling it (e.g. typeof resolvePrebuild === 'function' or
optional chaining) and only call it when present, otherwise fall back to the
local parser path so the module doesn't crash when resolvePrebuild is absent.
|
I'm going to merge this first and the remaining issue is actually different issues |
|
released |
This PR follows up on the issue raised here: ast-grep/ast-grep#2146.
Currently, the packages assume that
./parser.sois compatible with the system architecture when running the app.This assumption breaks when the app is bundled to run on multiple platforms.
In my case, I’m using
@ast-grep/lang-cin a VS Code extension and need a bundle that works for both ARM and x86 architectures.As a solution, I updated
packages/c/index.jsto loadparser.sofrom theprebuildsfolder based onprocess.platformandprocess.arch, similar to howrequireNativein@ast-grep/napiworks.Notes:
This PR is an MVP to resolve my immediate issue and unblock release for some users.
It currently only addresses
@ast-grep/lang-c.The extra copy of parser.so should be removed in a proper implementation.
Intended as a proof-of-concept rather than a merge-ready fix.
Please let me know what you think about it before I spend more time working on it.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation