Skip to content

Commit 2a03860

Browse files
committed
fix!: run root preinstall before reify
BREAKING CHANGE: root \`preinstall\` now runs before dependencies are installed.
1 parent 979518d commit 2a03860

5 files changed

Lines changed: 267 additions & 28 deletions

File tree

docs/lib/content/using-npm/scripts.md

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,33 +111,38 @@ It is run AFTER the changes have been applied and the `package.json` and `packag
111111

112112
#### [`npm ci`](/commands/npm-ci)
113113

114-
* `preinstall`
114+
* `preinstall` (before dependencies are installed)
115115
* `install`
116116
* `postinstall`
117117
* `prepublish`
118118
* `preprepare`
119119
* `prepare`
120120
* `postprepare`
121121

122-
These all run after the actual installation of modules into
123-
`node_modules`, in order, with no internal actions happening in between
122+
`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 tarballs are fetched. For `npm ci`, `preinstall` fires *after* the lockfile has been validated against `package.json`, so it cannot influence dependency resolution — that remains locked to `package-lock.json`. The remaining scripts run after the installation of modules into `node_modules`, in order, with no internal actions happening in between.
123+
124+
Because `preinstall` runs before reify, scripts cannot rely on packages from `node_modules`. `npm ci` wipes `node_modules` before `preinstall` fires, so `require()` of a dependency will always fail. Use `install` or `postinstall` for setup that depends on installed packages.
124125

125126
#### [`npm diff`](/commands/npm-diff)
126127

127128
* `prepare`
128129

129130
#### [`npm install`](/commands/npm-install)
130131

131-
These also run when you run `npm install -g <pkg-name>`
132+
These run on a bare `npm install` in a local project (no package arguments).
132133

133-
* `preinstall`
134+
* `preinstall` (before dependencies are installed)
134135
* `install`
135136
* `postinstall`
136137
* `prepublish`
137138
* `preprepare`
138139
* `prepare`
139140
* `postprepare`
140141

142+
`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.
143+
144+
Because `preinstall` runs before reify, scripts cannot rely on packages from `node_modules`. On a fresh checkout, `require()` of a dependency will fail. On a repeat `npm install` against an existing `node_modules/`, it may incidentally succeed because the previously-installed tree is still on disk, but the version available is whatever was previously installed and may be removed or replaced by the upcoming install. Use `install` or `postinstall` for setup that depends on installed packages.
145+
141146
If there is a `binding.gyp` file in the root of your package and you haven't defined your own `install` or `preinstall` scripts, npm will default the `install` command to compile using node-gyp via `node-gyp rebuild`
142147

143148
These are run from the scripts of `<pkg-name>`

lib/commands/ci.js

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -99,28 +99,33 @@ class CI extends ArboristWorkspaceCmd {
9999
})
100100
}
101101

102+
// Root lifecycle scripts for `npm ci` mirror those run by `npm install`. `preinstall` runs *before* reify so that scripts can bootstrap the environment (e.g. private-registry auth) before any dependency is fetched or unpacked. The remaining scripts run after reify as they did before.
103+
const scriptShell = this.npm.config.get('script-shell') || undefined
104+
const runRootScript = (event) => runScript({
105+
path: where,
106+
args: [],
107+
scriptShell,
108+
stdio: 'inherit',
109+
event,
110+
})
111+
112+
if (!ignoreScripts) {
113+
await runRootScript('preinstall')
114+
}
115+
102116
await arb.reify(opts)
103117

104-
// run the same set of scripts that `npm install` runs.
105118
if (!ignoreScripts) {
106-
const scripts = [
107-
'preinstall',
119+
const postReifyScripts = [
108120
'install',
109121
'postinstall',
110122
'prepublish', // XXX should we remove this finally??
111123
'preprepare',
112124
'prepare',
113125
'postprepare',
114126
]
115-
const scriptShell = this.npm.config.get('script-shell') || undefined
116-
for (const event of scripts) {
117-
await runScript({
118-
path: where,
119-
args: [],
120-
scriptShell,
121-
stdio: 'inherit',
122-
event,
123-
})
127+
for (const event of postReifyScripts) {
128+
await runRootScript(event)
124129
}
125130
}
126131
await reifyFinish(this.npm, arb)

lib/commands/install.js

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,27 +145,35 @@ class Install extends ArboristWorkspaceCmd {
145145
add: args,
146146
workspaces: this.workspaceNames,
147147
}
148+
149+
// Root lifecycle scripts only run for a bare `npm install` in a local project. `preinstall` runs *before* Arborist touches the filesystem so that scripts can bootstrap the environment (e.g. set up private-registry auth, generate files consumed during resolution) before dependencies are fetched or unpacked. The remaining scripts run after reify as they did before.
150+
const runRootLifecycle = !args.length && !isGlobalInstall && !ignoreScripts
151+
const runRootScript = (event) => runScript({
152+
path: where,
153+
args: [],
154+
scriptShell,
155+
stdio: 'inherit',
156+
event,
157+
})
158+
159+
if (runRootLifecycle) {
160+
await runRootScript('preinstall')
161+
}
162+
148163
const arb = new Arborist(opts)
149164
await arb.reify(opts)
150165

151-
if (!args.length && !isGlobalInstall && !ignoreScripts) {
152-
const scripts = [
153-
'preinstall',
166+
if (runRootLifecycle) {
167+
const postReifyScripts = [
154168
'install',
155169
'postinstall',
156170
'prepublish', // XXX(npm9) should we remove this finally??
157171
'preprepare',
158172
'prepare',
159173
'postprepare',
160174
]
161-
for (const event of scripts) {
162-
await runScript({
163-
path: where,
164-
args: [],
165-
scriptShell,
166-
stdio: 'inherit',
167-
event,
168-
})
175+
for (const event of postReifyScripts) {
176+
await runRootScript(event)
169177
}
170178
}
171179
await reifyFinish(this.npm, arb)

test/lib/commands/ci.js

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,115 @@ t.test('lifecycle scripts', async t => {
182182
], 'runs appropriate scripts, in order')
183183
})
184184

185+
// Regression test: `npm ci` must run root `preinstall` before reify populates node_modules, matching `npm install` behavior.
186+
t.test('preinstall runs before reify for npm ci', async t => {
187+
const events = []
188+
const { npm, registry } = await loadMockNpm(t, {
189+
prefixDir: {
190+
abbrev: abbrev,
191+
'package.json': JSON.stringify({
192+
...packageJson,
193+
scripts: {
194+
preinstall: 'echo preinstall',
195+
postinstall: 'echo postinstall',
196+
},
197+
}),
198+
'package-lock.json': JSON.stringify(packageLock),
199+
},
200+
mocks: {
201+
'@npmcli/run-script': async (opts) => {
202+
if (opts.path === npm.prefix) {
203+
const abbrevPkg = path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')
204+
events.push({ event: opts.event, depInstalled: fs.existsSync(abbrevPkg) })
205+
}
206+
},
207+
},
208+
})
209+
const manifest = registry.manifest({ name: 'abbrev' })
210+
await registry.tarball({
211+
manifest: manifest.versions['1.0.0'],
212+
tarball: path.join(npm.prefix, 'abbrev'),
213+
})
214+
registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {})
215+
await npm.exec('ci', [])
216+
217+
const pre = events.find(e => e.event === 'preinstall')
218+
const post = events.find(e => e.event === 'postinstall')
219+
t.ok(pre, 'preinstall ran')
220+
t.ok(post, 'postinstall ran')
221+
t.equal(pre.depInstalled, false, 'preinstall runs before dependencies are installed')
222+
t.equal(post.depInstalled, true, 'postinstall runs after dependencies are installed')
223+
})
224+
225+
// Regression test: --ignore-scripts must suppress the new pre-reify `preinstall` path in `npm ci`, matching the symmetric guarantee in `npm install`.
226+
t.test('--ignore-scripts skips preinstall entirely for npm ci', async t => {
227+
const events = []
228+
const { npm, registry } = await loadMockNpm(t, {
229+
config: { 'ignore-scripts': true, audit: false },
230+
prefixDir: {
231+
abbrev: abbrev,
232+
'package.json': JSON.stringify({
233+
...packageJson,
234+
scripts: {
235+
preinstall: 'echo preinstall',
236+
postinstall: 'echo postinstall',
237+
},
238+
}),
239+
'package-lock.json': JSON.stringify(packageLock),
240+
},
241+
mocks: {
242+
'@npmcli/run-script': async (opts) => {
243+
if (opts.path === npm.prefix) {
244+
events.push(opts.event)
245+
}
246+
},
247+
},
248+
})
249+
const manifest = registry.manifest({ name: 'abbrev' })
250+
await registry.tarball({
251+
manifest: manifest.versions['1.0.0'],
252+
tarball: path.join(npm.prefix, 'abbrev'),
253+
})
254+
await npm.exec('ci', [])
255+
t.strictSame(events, [], 'no root lifecycle scripts run when --ignore-scripts is set')
256+
})
257+
258+
// Regression test: symmetric to the install-side guarantee — a failing root `preinstall` must short-circuit before reify runs in `npm ci`, so dependencies never reach disk on failure.
259+
t.test('a failing preinstall prevents reify for npm ci', async t => {
260+
const events = []
261+
const { npm } = await loadMockNpm(t, {
262+
prefixDir: {
263+
abbrev: abbrev,
264+
'package.json': JSON.stringify({
265+
...packageJson,
266+
scripts: {
267+
preinstall: 'exit 1',
268+
postinstall: 'echo postinstall',
269+
},
270+
}),
271+
'package-lock.json': JSON.stringify(packageLock),
272+
},
273+
mocks: {
274+
'@npmcli/run-script': async (opts) => {
275+
if (opts.path === npm.prefix) {
276+
events.push(opts.event)
277+
if (opts.event === 'preinstall') {
278+
throw Object.assign(new Error('preinstall failed'), { code: 'ELIFECYCLE' })
279+
}
280+
}
281+
},
282+
},
283+
})
284+
285+
await t.rejects(npm.exec('ci', []), /preinstall failed/, 'ci rejects when preinstall fails')
286+
t.strictSame(events, ['preinstall'], 'only preinstall ran; no post-reify scripts')
287+
t.equal(
288+
fs.existsSync(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')),
289+
false,
290+
'no dependency reached disk after preinstall failure'
291+
)
292+
})
293+
185294
t.test('should throw if package-lock.json is missing', async t => {
186295
const { npm } = await loadMockNpm(t, {
187296
prefixDir: {

test/lib/commands/install.js

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,118 @@ t.test('exec commands', async t => {
101101
t.strictSame(lifecycleScripts, runOrder, 'all script ran in the correct order')
102102
})
103103

104+
// Regression test: root `preinstall` must run before any dependency is fetched/unpacked, while `install` and `postinstall` run after reify has populated node_modules.
105+
await t.test('preinstall runs before reify, post-reify scripts run after', async t => {
106+
const events = []
107+
const { npm, registry } = await loadMockNpm(t, {
108+
config: { audit: false },
109+
prefixDir: {
110+
'package.json': JSON.stringify({
111+
...packageJson,
112+
scripts: {
113+
preinstall: 'echo preinstall',
114+
install: 'echo install',
115+
postinstall: 'echo postinstall',
116+
},
117+
}),
118+
abbrev,
119+
},
120+
mocks: {
121+
'@npmcli/run-script': async (opts) => {
122+
// Only record scripts targeted at the project root, not any that arborist may run for dependencies during reify.
123+
if (opts.path === npm.prefix) {
124+
const abbrevPkg = path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')
125+
events.push({ event: opts.event, depInstalled: fs.existsSync(abbrevPkg) })
126+
}
127+
},
128+
},
129+
})
130+
const manifest = registry.manifest({ name: 'abbrev' })
131+
await registry.package({ manifest })
132+
await registry.tarball({
133+
manifest: manifest.versions['1.0.0'],
134+
tarball: path.join(npm.prefix, 'abbrev'),
135+
})
136+
137+
await npm.exec('install')
138+
139+
const pre = events.find(e => e.event === 'preinstall')
140+
const post = events.find(e => e.event === 'postinstall')
141+
t.ok(pre, 'preinstall ran')
142+
t.ok(post, 'postinstall ran')
143+
t.equal(pre.depInstalled, false, 'preinstall runs before dependencies are installed')
144+
t.equal(post.depInstalled, true, 'postinstall runs after dependencies are installed')
145+
})
146+
147+
await t.test('without args, --ignore-scripts skips preinstall entirely', async t => {
148+
const events = []
149+
const { npm, registry } = await loadMockNpm(t, {
150+
config: { audit: false, 'ignore-scripts': true },
151+
prefixDir: {
152+
'package.json': JSON.stringify({
153+
...packageJson,
154+
scripts: {
155+
preinstall: 'echo preinstall',
156+
postinstall: 'echo postinstall',
157+
},
158+
}),
159+
abbrev,
160+
},
161+
mocks: {
162+
'@npmcli/run-script': async (opts) => {
163+
if (opts.path === npm.prefix) {
164+
events.push(opts.event)
165+
}
166+
},
167+
},
168+
})
169+
const manifest = registry.manifest({ name: 'abbrev' })
170+
await registry.package({ manifest })
171+
await registry.tarball({
172+
manifest: manifest.versions['1.0.0'],
173+
tarball: path.join(npm.prefix, 'abbrev'),
174+
})
175+
176+
await npm.exec('install')
177+
t.strictSame(events, [], 'no root lifecycle scripts run when --ignore-scripts is set')
178+
})
179+
180+
// Regression test: a failing root `preinstall` must short-circuit before reify runs, so dependencies never reach disk on failure. This is the cleaner failure mode the PR was motivated by; future refactors that swallow the rejection and still call reify must fail here.
181+
await t.test('a failing preinstall prevents reify', async t => {
182+
const events = []
183+
const { npm } = await loadMockNpm(t, {
184+
config: { audit: false },
185+
prefixDir: {
186+
'package.json': JSON.stringify({
187+
...packageJson,
188+
scripts: {
189+
preinstall: 'exit 1',
190+
postinstall: 'echo postinstall',
191+
},
192+
}),
193+
abbrev,
194+
},
195+
mocks: {
196+
'@npmcli/run-script': async (opts) => {
197+
if (opts.path === npm.prefix) {
198+
events.push(opts.event)
199+
if (opts.event === 'preinstall') {
200+
throw Object.assign(new Error('preinstall failed'), { code: 'ELIFECYCLE' })
201+
}
202+
}
203+
},
204+
},
205+
})
206+
207+
await t.rejects(npm.exec('install'), /preinstall failed/, 'install rejects when preinstall fails')
208+
t.strictSame(events, ['preinstall'], 'only preinstall ran; no post-reify scripts')
209+
t.equal(
210+
fs.existsSync(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')),
211+
false,
212+
'no dependency reached disk after preinstall failure'
213+
)
214+
})
215+
104216
await t.test('should ignore scripts with --ignore-scripts', async t => {
105217
const { npm, registry } = await loadMockNpm(t, {
106218
config: {

0 commit comments

Comments
 (0)