fix(build): prepare npm git installs without pnpm - #792
Conversation
npm v11's git dep preparation runs `prepare` before node_modules exist in the temp clone directory, causing TypeScript compilation to fail. Changes: - build.js: skip build gracefully when node_modules absent - package.json: use `node build.js` directly in prepare/prepack for npm compatibility (avoids pnpm dependency during git dep install) Note: postinstall.js already handles all errors internally via main().catch(() => process.exit(0)), so no `|| true` wrapper needed. Install from GitHub with: npm pack github:user/repo#branch npm install -g ./fission-ai-openspec-x.y.z.tgz Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe package ChangesNPM source installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR routes package preparation through the existing Node build script for npm Git installs without adding a new runtime or deployment path. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR fixes npm v11 git-dep installation by making the build lifecycle hooks pnpm-agnostic and adding a graceful skip in Key changes:
Minor concerns:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant npm as npm v11
participant prep as prepare hook
participant build as build.js
participant deps as dep installer
participant prepack as prepack hook
Note over npm: git dep install flow
npm->>prep: run prepare
prep->>build: node build.js
build->>build: existsSync('node_modules')?
alt node_modules absent
build-->>prep: exit 0 (skip gracefully)
end
npm->>deps: install dependencies
deps-->>npm: node_modules ready
npm->>prepack: run prepack
prepack->>build: node build.js
build->>build: existsSync('node_modules') → true
build->>build: clean dist/, compile TypeScript
build-->>prepack: exit 0 (build success)
npm->>npm: pack tarball & install
Note over npm: npm publish flow
npm->>prep: run prepare
prep->>build: node build.js → build #1
npm->>prepack: run prepack
prepack->>build: node build.js → build #2 (redundant)
Last reviewed commit: 7473589 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "prepare": "node build.js", | ||
| "prepack": "node build.js", |
There was a problem hiding this comment.
Redundant double build during npm publish
Both prepare and prepack are set to node build.js, and npm publish triggers both hooks (plus prepublishOnly which calls pnpm run build). This means every publish results in 3 full build passes. Since build.js cleans dist first, the result is correct but the extra compilation time is wasted.
If the intent is for prepare to cover local dev installs and prepack to cover the git-dep pack phase, consider guarding prepack to only run when dist is absent (i.e. when prepare was skipped):
"prepare": "node build.js",
"prepack": "node -e \"const {existsSync}=require('fs'); if(!existsSync('dist')) process.exit(0);\" || node build.js",Or more cleanly, extract the "build if not already built" logic into build.js itself so both hooks share a single idempotent entry point without redundant work.
| if (!existsSync('node_modules')) { | ||
| console.log('⏭️ Skipping build (node_modules not yet available)'); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
node_modules check is CWD-sensitive
existsSync('node_modules') resolves relative to the process's current working directory, not the script's location. npm lifecycle scripts set CWD to the package root, so this is safe in the intended scenarios. However, if build.js is ever invoked via a path like node packages/openspec/build.js from the monorepo root (e.g. in a CI script), node_modules would refer to the root-level directory rather than the package's own deps, making the check unreliable.
Consider anchoring the check to the script's own directory for robustness:
| if (!existsSync('node_modules')) { | |
| console.log('⏭️ Skipping build (node_modules not yet available)'); | |
| process.exit(0); | |
| } | |
| if (!existsSync(new URL('node_modules', import.meta.url).pathname)) { |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
clay-good
changed the title
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at d7bfad5. The production change is limited to invoking build.js directly from prepare. Local verification passed all eight npm source-install and package lifecycle tests, build, TypeScript checking, and diff checking. Hosted fork CI remains pending, but the exact head is clean.
3 participants
Status
LGTM; ready for final review. All GitHub checks passed, including Linux, macOS, Windows, Nix, lint/type checking, security, and CodeRabbit. GitHub reports CLEAN/MERGEABLE. This PR has not been merged.
What was wrong
Installing OpenSpec from Git required pnpm because npm invokes the package's
preparescript, which ranpnpm run build.The original patch also skipped builds when
node_moduleswas absent and addedprepack. That could silently pack missing or stale output. npm installs Git dependencies' development dependencies before preparation; the extra hook is unnecessary.How it was fixed
node build.jsdirectly fromprepare.prepackhook.mainand resolve the obsolete postinstall-script conflict without restoring removed install hooks.The production diff against
mainis one line inpackage.json.Replication / proof
npm install --omit=dev git+file://...with pnpm blocked. The installed CLI returned version1.11.0, displayed help, and loaded the packagedspec-drivenschema. JavaScript and declarations were present; TypeScript was not installed in the consumer.prepareback topnpm run buildmade the same Git-install smoke fail at the pnpm sentinel (exit 93).npm packreturned success with missing or stale output. Both pass after removing the guard.pnpm exec vitest run test/package-install-scripts.test.ts: 8 passed, including 5 new integration cases; independently rerun by a second reviewer.pnpm test: 4,234 passed across 145 files.pnpm run build,pnpm exec tsc --noEmit,pnpm lint, andgit diff --check: passed.Notes / nits
ZSH/ZSH_CUSTOM, and allowed its localhost mock server. The initial environment-related failures were reproduced on unchangedmain; no unrelated test fixes are included.