Skip to content

chore: add prepare script so git installs build dist/#982

Closed
JohnMcLear wants to merge 1 commit into
mainfrom
chore/prepare-script
Closed

chore: add prepare script so git installs build dist/#982
JohnMcLear wants to merge 1 commit into
mainfrom
chore/prepare-script

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

Test plan

  • Diff is one line; no test surface
  • Once merged, etherpad#7831 can pin `github:ether/ueberDB#main` to get green CI without waiting for the publish workflow to be repaired

🤖 Generated with Claude Code

Consumers that install ueberdb2 via a git ref (e.g. `pnpm add
github:ether/ueberDB#main`) need the TypeScript sources compiled into
dist/ — the published tarball ships the built artefacts but the git
tree does not (dist/ is gitignored). `npm`/`pnpm`'s `prepare` hook runs
on git installs and is a no-op for tarball installs from the npm
registry, so this fix has zero impact on the published-tarball path.

Picked up while unblocking ether/etherpad#7831, whose CI cannot resolve
`ueberdb2 ^6.1.0` until the npm publish workflow on this repo (failing
E404 since #981 merged) is restored. A git-ref pin is the natural
stopgap and requires this script to work end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add prepare script for git-based installations

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add prepare script to auto-build dist/ on git installs
• Enables consumers to install via git refs without manual build
• No impact on npm tarball installation path
• Unblocks etherpad CI pinning to main branch
Diagram
flowchart LR
  A["Git install via ref"] -->|"prepare hook runs"| B["pnpm run build"]
  B --> C["dist/ generated"]
  C --> D["Consumer gets built artifacts"]
  E["npm tarball install"] -->|"prepare skipped"| F["No change to existing flow"]

Loading

File Changes

1. package.json ⚙️ Configuration changes +1/-0

Add prepare script for automatic builds

• Added prepare script entry that runs pnpm run build
• Script executes automatically on git-based installations
• Ensures TypeScript sources are compiled to dist/ for git consumers
• Zero impact on npm registry tarball installations

package.json


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 22, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Action required

1. Prepare requires pnpm 🐞 Bug ☼ Reliability
Description
The added prepare lifecycle script runs pnpm run build, so any install path that executes
prepare without pnpm on PATH (e.g., installing from a git ref via npm/yarn) will fail before the
package can be used. This is especially risky because the repo’s README documents installation via
npm and the package.json does not declare/enforce a package manager.
Code

package.json[141]

Evidence
prepare was added and it invokes pnpm, which requires pnpm to be present at install time. The
README’s install instructions use npm, and package.json contains no packageManager field (file
ends without one), so consumers are not guided/enforced to have pnpm available; additionally, the
publish workflow notes special handling for packages that have a prepare script.

package.json[135-147]
README.md[32-36]
.github/workflows/npmpublish.yml[87-91]
package.json[1-6]
package.json[158-161]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`prepare` is a package-manager lifecycle hook that can be executed by installers (not just pnpm). Defining it as `pnpm run build` introduces a hard dependency on a global `pnpm` binary, which can break installs when pnpm is unavailable.

## Issue Context
- `prepare` now shells out to pnpm.
- The README shows `npm install ueberdb2`, so npm usage is expected.
- The publish workflow explicitly calls out special handling when a package has a `prepare` script.

## Fix options (pick one)
1) **Remove pnpm dependency from lifecycle scripts (recommended):**
  - Update `build`, `build:js`, and `build:types` scripts to call local binaries directly (from `node_modules/.bin`), e.g. `rolldown -c ...` and `tsc ...`.
  - Set `prepare` to `npm run build` (or simply `run build`-equivalent) once `build` no longer calls pnpm.

2) **Ensure pnpm is always available when `prepare` runs:**
  - Add a `packageManager` field (pin to a chosen pnpm version) and change `prepare` to use Corepack (e.g., `corepack pnpm run build`) so pnpm can be provisioned even if not globally installed.

## Fix Focus Areas
- package.json[135-147]
- package.json[1-6]
- package.json[158-161]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread package.json
"format": "oxfmt --write .",
"format:check": "oxfmt --check .",
"build": "pnpm run build:js && pnpm run build:types",
"prepare": "pnpm run build",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Prepare requires pnpm 🐞 Bug ☼ Reliability

The added prepare lifecycle script runs pnpm run build, so any install path that executes
prepare without pnpm on PATH (e.g., installing from a git ref via npm/yarn) will fail before the
package can be used. This is especially risky because the repo’s README documents installation via
npm and the package.json does not declare/enforce a package manager.
Agent Prompt
## Issue description
`prepare` is a package-manager lifecycle hook that can be executed by installers (not just pnpm). Defining it as `pnpm run build` introduces a hard dependency on a global `pnpm` binary, which can break installs when pnpm is unavailable.

## Issue Context
- `prepare` now shells out to pnpm.
- The README shows `npm install ueberdb2`, so npm usage is expected.
- The publish workflow explicitly calls out special handling when a package has a `prepare` script.

## Fix options (pick one)
1) **Remove pnpm dependency from lifecycle scripts (recommended):**
   - Update `build`, `build:js`, and `build:types` scripts to call local binaries directly (from `node_modules/.bin`), e.g. `rolldown -c ...` and `tsc ...`.
   - Set `prepare` to `npm run build` (or simply `run build`-equivalent) once `build` no longer calls pnpm.

2) **Ensure pnpm is always available when `prepare` runs:**
   - Add a `packageManager` field (pin to a chosen pnpm version) and change `prepare` to use Corepack (e.g., `corepack pnpm run build`) so pnpm can be provisioned even if not globally installed.

## Fix Focus Areas
- package.json[135-147]
- package.json[1-6]
- package.json[158-161]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@JohnMcLear
Copy link
Copy Markdown
Member Author

Closing in favour of #983, which fixes the publish workflow root cause (stored NPM_TOKEN → OIDC trusted publishing). Once #983 lands and the one-time npmjs.com trusted-publisher setup is done, ueberdb2 publishes will be reliable end-to-end and the git-ref install path is no longer needed to unblock ether/etherpad#7831.

@JohnMcLear JohnMcLear closed this May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant