🐛 Build npm packages from their local release closure - #847
Merged
Merged
Conversation
A tagged publish waited on npm indexing a sibling the previous job had just published, because the builder rewrote every `workspace:*` dependency to `^<version>` before calling dnt. `publish-one.yml` bounded that wait with four attempts and 15-second sleeps; the observed lag exceeded it on 0.12.0, 0.12.1 and 0.13.1, and 0.13.1 reached npm only after four rounds of rerunning failed jobs. `scripts/build-npm.ts` now builds in two phases. Phase 1 builds the requested package's internal dependencies depth-first from the same checkout, each at most once, and hands dnt absolute `file:` ranges naming those artifacts, so the install and the type check never reach the registry for a package from this release. Phase 2 finalizes every manifest in the closure together, once the last dnt call has returned, replacing each internal `file:` range with the sibling's `^<version>`. Together, not per package: in a chain A → B → C, rewriting B when its own build finishes puts C's registry version back in front of A's install. Before reporting success the builder refuses any dependency range still starting with `workspace:` or `file:`, and any string naming the checkout's path — a release gate, so an unexpected local dependency fails the build instead of being normalized into something npm would accept. An internal dependency no workspace member declares is refused by name, with no registry fallback. `DNT_LOCAL_SIBLINGS` is gone: the default path is what it used to provide, and the artifact a developer builds is now the artifact a release publishes. `DNT_SKIP_INSTALL` keeps its leaf-only contract. `publish-one.yml` builds once and keeps its already-published guard; `publish-packages.yml` keeps its dependency-ordered `needs:`, which is now a publication guarantee rather than a build one — a failed upstream still withholds its dependents, so whatever npm holds is dependency-closed. The builder is importable, so the regression drives the real path over a three-member closure whose versions npm has never seen.
N4 was returned as unprovable, and that was wrong about the mechanism rather than about the contract. npm symlinks a directory `file:` dependency by default, so a dependent reaches whatever the sibling's own build left in its `node_modules` and an early-finalized sibling costs nothing. npm's supported `install-links=true` packs and installs it as an ordinary dependency instead, which leaves the sibling's own manifest as the only statement of where its dependencies come from — and that is the discriminator. N4 now runs the real builder twice under an invocation-private npm configuration: a positive control, so packed local artifacts are known to work before a failure means anything, then the same closure with `b` rewritten to the registry range the moment its build completes. `a`'s install fails, no manifest is finalized, and `a` never completes. The registry it would have to reach is unreachable and retries are off, so the failure is hermetic and immediate rather than whatever npmjs.org happens to answer. The environment is restored however the case ends; every other build in the file is the ordinary one. Setting `install-links=false` makes the negative control pass, which is what makes the scoped configuration load-bearing rather than decoration. The three places that explained phase 2 by claiming an early rewrite puts a registry version in front of the dependent's install now say what is actually true: the cost of finalizing early depends on how npm installs a local directory and on the sibling's own build residue, and finalizing the closure at the end is what makes the build independent of both.
`expect(caught).toBeInstanceOf(Error)` accepted any throw, including one from the observer that applies the mutation — so a negative control that never reached `a` would have passed while proving nothing. The build-start events must now be exactly `c`, `b`, `a`, which is what says the mutation was applied and `a` entered dnt; an observer that threw leaves that list one short. The caught error must be dnt's `npm install failed with exit code 1`, which is what says it failed where a registry range has to be resolved rather than anywhere else in the run. The proof that only `c` and `b` complete and that finalization never starts is unchanged. Making the observer throw before its rewrite now fails the case.
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
1 participant
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.
Closes #843.
Why
A tagged publish waited on npm indexing a sibling the previous job had just
published.
scripts/build-npm.tsrewrote everyworkspace:*dependency to^<version>before calling dnt, so each downstream package's build needed theregistry to already serve what its upstream job published moments earlier.
publish-one.ymlbounded that with four attempts and 15-second sleeps.It lost three releases running:
core404 …/runtime-0.12.0.tgz, 38s after the sibling publishedcliETARGET … test-agent@^0.12.1; the sibling indexed ~1 min after cli gave upcore, then two tiers after itETARGET … durable-streams@^0.13.1, four attempts in 53s0.13.1 reached npm only after four rounds of rerunning failed jobs, and
@executablemd/clihad by then missed two releases outright.What changes
Before: the builder resolved internal siblings from the public registry, and the
release workflow retried the build to wait that out.
After: the builder constructs the requested package's internal closure from the
same checkout and finalizes every manifest at the end, so no build asks npm for
a package from its own release.
publish-one.ymlbuilds once.How it works
Phase 1 builds internal dependencies first, each at most once so a diamond
shares one artifact, and hands dnt absolute
file:ranges naming the artifactsthis same invocation produced. Install and typecheck therefore resolve every
sibling from the working tree.
Phase 2 runs once the last dnt call has returned, replacing each internal
file:range with the sibling's^<version>from the workspace manifests. Allof them together, not each package as its own build finishes — see What must
stay true.
The result is publishable or the build fails. Every generated manifest is
inspected before any is written: a dependency range still starting with
workspace:orfile:, or any string naming the checkout's path orfile:URL, fails the command. A known internal range is rewritten; anything else local
is an unexpected dependency, and normalizing it away is the one edit that would
make an unpublishable artifact look fine.
DNT_LOCAL_SIBLINGSis gone — the default path is what it used to provide, sothe artifact a developer builds is now the artifact a release publishes.
DNT_SKIP_INSTALLkeeps its leaf-only contract unchanged.Review guide
Start with:
specs/release-process-spec.md, the "Building a package" sectionThen review:
scripts/build-npm.ts—buildNpmPackage, thenfinalizeClosureandunpublishablescripts/tests/build-npm.test.ts— N1–N5, especiallyusePackedLocalDependencies.github/workflows/publish-one.ymland its W1 assertionLook carefully at:
unpublishablemust refuse an unexpected local range rather than rewrite it.A blanket
file:replacement would launder an unpublishable dependency.What must stay true
when its own build finishes leaves it describing a dependency only the
registry could supply; whether the dependent survives then depends on how npm
installs a local directory and on that sibling's build residue. Enforced by
finalizeClosurerunning once frombuildNpmPackage, checked by N2 and N4.publish-packages.yml'sneeds:edges are untouched. They no longer serve the build, but a failed upstream
publish must still withhold its dependents so whatever npm holds is
dependency-closed. Checked by W2.
already-published guard. Checked by W1.
How to verify it
closure, and fails if only direct siblings are local.
bstill namesclocally ata's build start and that nobuild event follows finalization. Finalizing per-package instead makes it fail.
^<version>and no localreference. It passes under per-package finalization, which is why N2 and N4
exist.
install-links=truenpmconfiguration — npm's packed layout, where a sibling's own manifest is the
only statement of where its dependencies come from. A positive control runs
first, then the same closure with
brewritten early:areaches dnt, itsinstall fails with
npm install failed with exit code 1, and nothingfinalizes. Setting
install-links=false, or making the observer throw beforeits rewrite, both make it fail.
registry fallback, and that an unrelated external
file:dependency isconsumable during dnt but refused at the gate.
that the CLI's whole closure is publishable after the built bin consumed it.
already-published guard, and that
needs:ordering remains.deno task test \ scripts/tests/build-npm.test.ts \ scripts/tests/cli-npm-bin.test.ts \ scripts/tests/adapter-npm-package.test.ts \ scripts/tests/publish-workflow-membership.test.ts \ scripts/tests/publish-workflow-generator.test.ts→
ok | 9 passed (30 steps) | 0 failed, plusdeno task check,deno task lintand
git diff --checkall exit 0.Scope
Included
entrypoint so the regression drives the real path.
publish-one.ymlreduced to one build attempt.Intentionally unchanged
publish-packages.ymland its generator. Theneeds:graph is deliberate.change.
same step.
New abstractions
buildNpmPackage+BuildEventexist because the regression has to drive thereal release path; the CLI is an adapter over the same operation, guarded by
import.meta.main.resolution and no finalization policy, so a test cannot accidentally become a
second implementation.
Risks and limitations
built more than once across the matrix. That is deliberate: each output is
derived from the tag's checkout, with no artifact transfer or shared mutable
workspace. Jobs get slower; the release stops depending on the registry clock.
the point — the retry existed for propagation, which no longer happens — and
spec §7 rerun recovery is unchanged. 🧪 dnt's npm install aborts with exit 134 during esbuild postinstall in cli-npm-bin #830's flake would still need a rerun.
so reverting restores the former build without touching registry identities or
versions.
Generated or mechanical changes
None.
deno task gen:publish-workflowleaves no diff.Scope confirmation