fix(install): add prepare script so git installs build dist/#165
Open
johnkattenhorn wants to merge 1 commit intoLarsCowe:mainfrom
Open
fix(install): add prepare script so git installs build dist/#165johnkattenhorn wants to merge 1 commit intoLarsCowe:mainfrom
johnkattenhorn wants to merge 1 commit intoLarsCowe:mainfrom
Conversation
Consumers installing via `npm install -g github:LarsCowe/bmalph` (or any fork thereof) run npm's `prepare` lifecycle, not `prepack`. Without a prepare script, `dist/` is never built at install time and the CLI fails at launch (missing compiled output). The `[ -f tsconfig.json ]` guard skips compilation in the published tarball case where tsconfig.json is excluded (via .npmignore or the `files` whitelist), so the script is a no-op for regular `npm install bmalph` consumers. Test plan: 1. From a clean directory: `npm install -g github:LarsCowe/bmalph#pr-prepare-script` 2. `bmalph --version` should print the installed version (previously: `/usr/bin/bmalph: cannot execute` because `dist/` missing). 3. Regular npm registry installs (`npm install bmalph`) unchanged — tsconfig.json not in the tarball, guard trips, no-op.
3 tasks
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.
Problem
Installing bmalph from git (e.g.
npm install -g github:LarsCowe/bmalphor any fork thereof) leaves the CLI broken:Root cause: npm's git-install path runs the
preparelifecycle script, notprepack. The currentpackage.jsononly definesprepack(which runs whennpm packbuilds a tarball for registry publishing). So git installs skip the TypeScript build entirely and the installed package has nodist/directory —bin/bmalph.jsimports../dist/...and crashes at launch.Regular registry installs (
npm install bmalph) work because the tarball published to npm already hasdist/baked in (viaprepack).Fix
Add a
preparescript that builds whentsconfig.jsonis present (git-install case) and no-ops otherwise (tarball-install case):The guard ensures the script is safe for both install paths:
tsconfig.jsonis in the clone → build runs →dist/populated → CLI works.tsconfig.jsonis not shipped in the tarball (not in thefileswhitelist) → guard is false → no-op.Why this matters for forks
Anyone forking bmalph for private or team use (pinning a commit for stability, or prototyping a change before upstreaming) hits this immediately. The fix is one line and is npm-lifecycle-standard.
Test plan
npm installin a clone still works —preparebuilds dist/ automatically.npm packstill produces a working tarball (prepackhandles that path).npm install -g github:johnkattenhorn/bmalph#pr-prepare-script→bmalph --versionprints version. Before this change: install succeeds but the binary is broken.npm test) — unchanged (4 pre-existing unrelated failures ininstaller.test.tsabout.ralphrctemplate variants; not affected by this change).Not in scope
.ralphrctemplate-variant test failures I spotted during this work. Will follow up with a separate PR for that.