fix(appkit): ensure @ast-grep/napi binding at install (napi-postinstall)#458
Open
atilafassina wants to merge 2 commits into
Open
fix(appkit): ensure @ast-grep/napi binding at install (napi-postinstall)#458atilafassina wants to merge 2 commits into
atilafassina wants to merge 2 commits into
Conversation
Signed-off-by: Atila Fassina <atila@fassina.eu>
atilafassina
requested review from
Copilot and
ditadi
and removed request for
Copilot
Contributor
|
Signed-off-by: Atila Fassina <atila@fassina.eu>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a best-effort install-time prefetch to ensure the @ast-grep/napi native binding is present (via napi-postinstall), preventing appkit generate-types from failing on environments where npm skips optional native deps.
Changes:
- Add
napi-postinstall@0.3.4as a runtime dependency ofpackages/shared(and update lockfile). - Expand the published
postinstallscript to prefetch@ast-grep/napi’s binding under npm with a hard timeout, then print the existing setup hint. - Add unit tests for the new
ensureAstGrepBindingbehavior and TS declarations to allow importing the standalone JS script in tests.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks the new napi-postinstall dependency. |
| packages/shared/tsconfig.json | Includes scripts/**/*.d.ts so tests can import the standalone JS postinstall script without allowJs. |
| packages/shared/src/scripts/postinstall.test.ts | Adds tests covering npm-only behavior, timeout/failure non-fatal behavior, and pnpm no-op. |
| packages/shared/scripts/postinstall.js | Implements the npm-only, time-bounded prefetch plus a main guard and the setup hint printer. |
| packages/shared/scripts/postinstall.d.ts | Provides dev-only typings for the standalone postinstall script to support TS tests. |
| packages/shared/package.json | Adds napi-postinstall to runtime dependencies. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+22
| // Runs in the child: re-resolve and materialize the @ast-grep/napi host binding. | ||
| // `.catch(exit 1)` keeps the child quiet and turns a rejection into a non-zero exit | ||
| // that the parent treats as a failed (non-fatal) pre-fetch. | ||
| const PREFETCH_SCRIPT = | ||
| "Promise.resolve(require('napi-postinstall').checkAndPreparePackage('@ast-grep/napi')).catch(() => process.exit(1))"; |
Comment on lines
+54
to
+58
| execFileSync(process.execPath, ["-e", PREFETCH_SCRIPT], { | ||
| cwd: pkgDir, | ||
| stdio: "inherit", | ||
| timeout: PREFETCH_TIMEOUT_MS, | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds an install-time guarantee that the
@ast-grep/napinative host binary is present, vianapi-postinstall.packages/sharedgainsnapi-postinstall@0.3.4(flows into published@databricks/appkitthrough the existingtools/dist-appkit.tsCLI-deps merge).postinstall(packages/shared/scripts/postinstall.js) now ensures the binding before printing its setup hint.Why
On Databricks Apps deploy, npm runs a bare
npm installand silently skips the listed optional platform binary@ast-grep/napi-linux-x64-gnu(npm/cli#4828 cross-platform lockfile, or a supply-chain cutoff). The parent@ast-grep/napithen can't load its binding, crashingappkit generate-typesduring the app's own postinstall.napi-postinstalldetects the host platform and re-materializes the correct binding package, working around the npm skip. This is the root-cause fix, companion to the survivability/lazy-load change in #457: with the binding present, serving-type generation works fully rather than degrading.Design
npm install.napi-postinstallis CJS → loaded via dynamicimport().postinstall.jsships standalone (dist-appkit.tscopies just that file), so it imports only external runtime deps — never appkitsrc/dist.Verification
build/-r typecheck/test/checkgreen.ensureAstGrepBinding: invoked under an npm user-agent, non-fatal when the pre-fetch rejects, no-op under a non-npm (pnpm) user-agent.node scripts/postinstall.jsinvocation (so the side effects actually run on install); an npm-UA run loads the real CommonJSnapi-postinstallvia dynamicimport()and exits 0; a pnpm-UA run no-ops to just the hint.Refs: npm/cli#4828.
This pull request and its description were written by Isaac.