Skip to content

Build dist on install so a branch or SHA can be depended on - #1228

Draft
jeremy wants to merge 1 commit into
mainfrom
build-dist-on-install
Draft

Build dist on install so a branch or SHA can be depended on#1228
jeremy wants to merge 1 commit into
mainfrom
build-dist-on-install

Conversation

@jeremy

@jeremy jeremy commented Aug 11, 2026

Copy link
Copy Markdown
Member

One line in package.json: a prepare script, so yarn install builds dist/.

Without it, "@37signals/lexxy": "basecamp/lexxy#<sha>" installs a package with
no built output and the import fails. With it, a consumer can point at a branch
or a commit to try a change before it is released — which is how the rest of this
series gets QA'd against bc3.

No effect on the published package: dist/ is built by the release process
either way. prepare runs on yarn install from a git ref and on local installs;
it does not run for consumers installing from the registry.


Part of a series re-filing #1227 at reviewable scope, after #1227 was reverted
from main in 8c64aa4. Merge order: this → #dompurify → #images → #instance →
#trusted-types → #1226. Nothing here is released.

Draft: needs human review and a soak period before merging.

package.json ships `files: ["dist"]` and dist is gitignored, so
`"@37signals/lexxy": "basecamp/lexxy#some-sha"` installed a package whose
`module` entry pointed at a file that wasn't there. Verified before adding this:
the install succeeds, and node_modules/@37signals/lexxy has no dist at all.

`prepare` is the lifecycle npm and yarn both run for a git dependency, with
devDependencies available, so pointing it at the npm rollup config makes a
branch install produce the same dist a publish would. It also runs before
publish, which is harmless — release already builds explicitly.

That matters right now because three security branches are open against this
repo and the consumer needs to test against them before any of it is released.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an npm lifecycle script to automatically run the Rollup build for the npm bundle.

Changes:

  • Adds a prepare script that runs rollup -c rollup.config.npm.mjs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread package.json
Comment thread package.json
@jeremy

jeremy commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

⚠️ QA result: this does not do what the title says, for the consumers that matter

The bc3 QA step in this campaign was run against a git ref, which is exactly what this PR exists to enable. It doesn't work.

yarn 1.22 does not run a git dependency's prepare script. bc3 and fizzy both use yarn 1.22.22.

Installing basecamp/lexxy#<sha> into bc3 produces a package directory with src/, rollup.config.npm.mjs and no node_modules — and no dist/. Since package.json points module and exports at dist/lexxy.esm.js, the import fails.

Reproduced in isolation, same SHA, same machine:

# yarn 1.22.22
$ yarn install
$ ls node_modules/@37signals/lexxy/dist
ls: node_modules/@37signals/lexxy/dist: No such file or directory

# npm 11
$ npm install
$ ls node_modules/@37signals/lexxy/dist
lexxy.esm.js   lexxy_helpers.esm.js   stylesheets

So the prepare script is correct and works — under npm. Under yarn 1 it is inert, and yarn 1 is what the two consumers use.

What this means for the campaign

The QA in step 7 needed an installable ref, so I built one separately: integration/qa-build (4b0c9b9d) is integration/sanitizer-campaign plus one commit that force-adds the built dist/. That branch is for QA only and must not be merged. It exists purely so bc3 could install the code; the bc3 results reported on the other PRs are from it.

Options for a human

  1. Keep it anyway. It costs one line, it is correct, and it helps anyone using npm or yarn ≥2. Retitle so it doesn't promise something yarn-1 consumers won't get.
  2. Publish a prerelease instead. yarn release:alpha already exists, and an alpha on npm is installable by every package manager. This is probably the honest answer for "let bc3 try it before it ships" and it makes the git-ref path unnecessary.
  3. Commit dist/ on a QA branch each time, as I did here. Works, but it is a manual step someone has to remember and un-remember.

I'd take (2) — it removes the problem rather than working around it — but this is a judgement call, and (1) is cheap enough to keep regardless.

Either way the current title should not survive as a release note, since it states a capability that doesn't hold for bc3 or fizzy.


CI: green (12/12).

@jeremy

jeremy commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

Handoff note for the whole campaign, including QA results and what still needs a human decision: #1234

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.

2 participants