fix!: run root preinstall before reify#9267
Conversation
owlstronaut
force-pushed
the
fix/preinstall-ordering-2660
branch
from
4ce2da1 to
586dd87
Compare
| * `postprepare` | ||
|
|
||
| `preinstall` runs before any dependencies are fetched or unpacked into `node_modules`, so scripts can prepare the environment (for example, setting up authentication for a private registry) before resolution begins. The remaining scripts run after installation has completed. | ||
|
|
There was a problem hiding this comment.
note: this means any scripts that require() a dependency will probably fail
There was a problem hiding this comment.
This was the biggest reason for the change in the first place. This tripped up more folks than anticipated. It seems obvious in hindsight but it isn't.
There was a problem hiding this comment.
+1 — worth calling out explicitly in the doc since this is the historical footgun. Maybe a sentence at the end of the new paragraph, e.g.:
Because dependencies are not yet on disk when
preinstallruns, scripts cannotrequire()any package fromnode_modulesat this point. Useinstall/postinstallfor setup that depends on installed packages.
This comment was marked as off-topic.
This comment was marked as off-topic.
| t.ok(post, 'postinstall ran') | ||
| t.equal(pre.depInstalled, false, 'preinstall runs before dependencies are installed') | ||
| t.equal(post.depInstalled, true, 'postinstall runs after dependencies are installed') | ||
| }) |
There was a problem hiding this comment.
nit — the new test covers the success path; should we also lock in the failure contract? Something like:
await t.test('a failing preinstall prevents reify', async t => {
// mock '@npmcli/run-script' to throw on preinstall
// assert npm.exec('install') rejects AND node_modules/abbrev does not exist
})The "no dep on disk before preinstall" guarantee is the real reason for the change — worth a regression test so a future refactor cannot quietly swallow a rejection and still reify.
| 'package-lock.json': JSON.stringify(packageLock), | ||
| }, | ||
| mocks: { | ||
| '@npmcli/run-script': (opts) => { |
There was a problem hiding this comment.
nit — the install.js equivalent mocks this as async (opts) => {...}; here it is sync. Both work, but matching the shape would make it obvious the helper returns a promise:
'@npmcli/run-script': async (opts) => { ... },
BREAKING CHANGE: root \`preinstall\` now runs before dependencies are installed.
owlstronaut
force-pushed
the
fix/preinstall-ordering-2660
branch
from
586dd87 to
9ac918f
Compare
4 participants
BREAKING CHANGE: root `preinstall` now runs before dependencies are installed.
Summary
Moves the root
preinstallscript back to its documented position: before dependencies are installed. Restores behavior that shipped through npm 6 and was unintentionally broken in npm 7 when Arborist took over reify.Fixes #2660.
The bug
The scripts docs have always said
preinstallruns "before the package is installed." Since npm 7, the rootpackage.json'spreinstallhas actually run afterarb.reify()i.e. after every dependency has been resolved, fetched, and unpacked. Arborist explicitly excludes the project root from its rebuild queue (reify.js:1234,!diff.ideal.isProjectRoot), andlib/commands/install.jsappendedpreinstallto the post-reify lifecycle loop along withinstall,postinstall,prepublish,preprepare,prepare,postprepare.npm cihas the same shape.Net effect: there is currently no way to run a script at the root before dependencies hit disk. Projects that want to bootstrap auth, generate files consumed during resolution, or gate installs behind a precondition have had no supported hook for five years.
The fix
Split the root lifecycle loop in two:
preinstallruns via a small helper beforearb.reify().install,postinstall,prepublish,preprepare,prepare,postpreparerun after reify, as they did before.Same split applied to
npm ci.Why now, and why not wait for a lifecycle redesign
This has history:
@ljharb.@wraithgarwith essentially this same code change, then self-closed as "too breaking without broader design work."preinstallordering in the next major.preunpack) and RFC #437 (preinstall revert) opened. Neither progressed. RFC 403'spreunpackhook was rejected in triage as not solving the actual bug.preunpacklifecycle script that runs before installation #8972 opened implementing the rejected RFC 403preunpackapproach. Close but not the right fix.A comprehensive lifecycle redesign is still the right long-term project and has clearly not happened in five years. Gating the revert on it was reasonable in 2021; but has proven that continuing to gate on it in 2026 just means users keep hitting a bug the docs promise doesn't exist. npm 12 is the right window to ship the scoped revert and decouple it from a future lifecycle rewrite.
The code change here is functionally the same as #2713 The differences are: it also patches
npm ci, updates the docs to match reality, adds regression tests, and ships in a major (npm 12) rather than being blocked on a design gate that no longer has owners.Closes / supersedes
When this lands:
preunpacklifecycle script that runs before installation #8972 (supersedes thepreunpackapproach)preunpacklife cycle script rfcs#403 (supersedes —preunpacknot needed)