fix: verify-deps-before-run after install --prod|--no-optional (#9055)

close #9019

.changeset/red-pens-cheer.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/deps.status": minor
+"@pnpm/plugin-commands-script-runners": patch
+"pnpm": patch
+---
+
+Fix a false negative of `verify-deps-before-run` after `pnpm install --production|--no-optional` [#9019](https://github.com/pnpm/pnpm/issues/9019).

deps/status/src/checkDepsStatus.ts

@@ -38,11 +38,10 @@ import {
 } from '@pnpm/types'
 import { findWorkspacePackages } from '@pnpm/workspace.find-packages'
 import { readWorkspaceManifest } from '@pnpm/workspace.read-manifest'
-import { loadWorkspaceState, updateWorkspaceState } from '@pnpm/workspace.state'
+import { type WorkspaceState, type WorkspaceStateSettings, loadWorkspaceState, updateWorkspaceState } from '@pnpm/workspace.state'
 import { assertLockfilesEqual } from './assertLockfilesEqual'
 import { safeStat, safeStatSync } from './safeStat'
 import { statManifestFile } from './statManifestFile'
-import { type WorkspaceStateSettings } from '@pnpm/workspace.state/src/types'
 
 export type CheckDepsStatusOptions = Pick<Config,
 | 'allProjects'
@@ -62,16 +61,32 @@ export type CheckDepsStatusOptions = Pick<Config,
 | 'pnpmfile'
 > & {
   ignoreFilteredInstallCache?: boolean
+  ignoredWorkspaceStateSettings?: Array<keyof WorkspaceStateSettings>
 } & WorkspaceStateSettings
 
-export async function checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDate: boolean | undefined, issue?: string }> {
+export interface CheckDepsStatusResult {
+  upToDate: boolean | undefined
+  issue?: string
+  workspaceState: WorkspaceState | undefined
+}
+
+export async function checkDepsStatus (opts: CheckDepsStatusOptions): Promise<CheckDepsStatusResult> {
+  const workspaceState = loadWorkspaceState(opts.workspaceDir ?? opts.rootProjectManifestDir)
+  if (!workspaceState) {
+    return {
+      upToDate: false,
+      issue: 'Cannot check whether dependencies are outdated',
+      workspaceState,
+    }
+  }
   try {
-    return await _checkDepsStatus(opts)
+    return await _checkDepsStatus(opts, workspaceState)
   } catch (error) {
     if (util.types.isNativeError(error) && 'code' in error && String(error.code).startsWith('ERR_PNPM_RUN_CHECK_DEPS_')) {
       return {
         upToDate: false,
         issue: error.message,
+        workspaceState,
       }
     }
     // This function never throws an error.
@@ -80,11 +95,12 @@ export async function checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{
     return {
       upToDate: undefined,
       issue: util.types.isNativeError(error) ? error.message : undefined,
+      workspaceState,
     }
   }
 }
 
-async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDate: boolean | undefined, issue?: string }> {
+async function _checkDepsStatus (opts: CheckDepsStatusOptions, workspaceState: WorkspaceState): Promise<CheckDepsStatusResult> {
   const {
     allProjects,
     autoInstallPeers,
@@ -102,25 +118,20 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
     ? getOptionsFromRootManifest(rootProjectManifestDir, rootProjectManifest)
     : undefined
 
-  const workspaceState = loadWorkspaceState(workspaceDir ?? rootProjectManifestDir)
-  if (!workspaceState) {
-    return {
-      upToDate: false,
-      issue: 'Cannot check whether dependencies are outdated',
-    }
-  }
   if (opts.ignoreFilteredInstallCache && workspaceState.filteredInstall) {
-    return { upToDate: undefined }
+    return { upToDate: undefined, workspaceState }
   }
 
   if (workspaceState.settings) {
+    const ignoredSettings = new Set<keyof WorkspaceStateSettings>(opts.ignoredWorkspaceStateSettings)
+    ignoredSettings.add('catalogs') // 'catalogs' is always ignored
     for (const [settingName, settingValue] of Object.entries(workspaceState.settings)) {
-      if (settingName === 'catalogs') continue
-      // @ts-expect-error
-      if (!equals(settingValue, opts[settingName])) {
+      if (ignoredSettings.has(settingName as keyof WorkspaceStateSettings)) continue
+      if (!equals(settingValue, opts[settingName as keyof WorkspaceStateSettings])) {
         return {
           upToDate: false,
           issue: `The value of the ${settingName} setting has changed`,
+          workspaceState,
         }
       }
     }
@@ -129,6 +140,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
     return {
       upToDate: false,
       issue: 'Configuration dependencies are not up to date',
+      workspaceState,
     }
   }
 
@@ -140,6 +152,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       return {
         upToDate: false,
         issue: 'Catalogs cache outdated',
+        workspaceState,
       }
     }
 
@@ -153,6 +166,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       return {
         upToDate: false,
         issue: 'The workspace structure has changed since last install',
+        workspaceState,
       }
     }
 
@@ -181,6 +195,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
         return {
           upToDate: false,
           issue: `Workspace package ${id} has dependencies but does not have a modules directory`,
+          workspaceState,
         }
       }
     }
@@ -192,7 +207,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
 
     if (modifiedProjects.length === 0) {
       logger.debug({ msg: 'No manifest files were modified since the last validation. Exiting check.' })
-      return { upToDate: true }
+      return { upToDate: true, workspaceState }
     }
 
     const issue = await patchesAreModified({
@@ -203,7 +218,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       hadPnpmfile: workspaceState.pnpmfileExists,
     })
     if (issue) {
-      return { upToDate: false, issue }
+      return { upToDate: false, issue, workspaceState }
     }
 
     logger.debug({ msg: 'Some manifest files were modified since the last validation. Continuing check.' })
@@ -287,7 +302,11 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
         })
       }))
     } catch (err) {
-      return { upToDate: false, issue: (util.types.isNativeError(err) && 'message' in err) ? err.message : undefined }
+      return {
+        upToDate: false,
+        issue: (util.types.isNativeError(err) && 'message' in err) ? err.message : undefined,
+        workspaceState,
+      }
     }
 
     // update lastValidatedTimestamp to prevent pointless repeat
@@ -299,7 +318,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       filteredInstall: workspaceState.filteredInstall,
     })
 
-    return { upToDate: true }
+    return { upToDate: true, workspaceState }
   }
 
   if (!allProjects) {
@@ -344,7 +363,7 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       hadPnpmfile: workspaceState.pnpmfileExists,
     })
     if (issue) {
-      return { upToDate: false, issue }
+      return { upToDate: false, issue, workspaceState }
     }
 
     if (currentLockfileStats && wantedLockfileStats.mtime.valueOf() > currentLockfileStats.mtime.valueOf()) {
@@ -379,7 +398,11 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
           wantedLockfileDir: rootProjectManifestDir,
         })
       } catch (err) {
-        return { upToDate: false, issue: (util.types.isNativeError(err) && 'message' in err) ? err.message : undefined }
+        return {
+          upToDate: false,
+          issue: (util.types.isNativeError(err) && 'message' in err) ? err.message : undefined,
+          workspaceState,
+        }
       }
     } else if (currentLockfileStats) {
       logger.debug({ msg: 'The manifest file is not newer than the lockfile. Exiting check.' })
@@ -392,14 +415,14 @@ async function _checkDepsStatus (opts: CheckDepsStatusOptions): Promise<{ upToDa
       }
     }
 
-    return { upToDate: true }
+    return { upToDate: true, workspaceState }
   }
 
   // `opts.allProject` being `undefined` means that the run command was not run with `--recursive`.
   // `rootProjectManifest` being `undefined` means that there's no root manifest.
   // Both means that `pnpm run` would fail, so checking lockfiles here is pointless.
   globalWarn('Skipping check.')
-  return { upToDate: undefined }
+  return { upToDate: undefined, workspaceState }
 }
 
 interface AssertWantedLockfileUpToDateContext {

deps/status/src/index.ts

@@ -1 +1,2 @@
-export { type CheckDepsStatusOptions, checkDepsStatus } from './checkDepsStatus'
+export { type WorkspaceStateSettings } from '@pnpm/workspace.state'
+export { type CheckDepsStatusOptions, type CheckDepsStatusResult, checkDepsStatus } from './checkDepsStatus'

exec/plugin-commands-script-runners/src/installCommand.ts

@@ -0,0 +1,40 @@
+import path from 'path'
+import { sync as execSync } from 'execa'
+
+export type InstallOptions = Array<'--production' | '--dev' | '--no-optional'>
+export type InstallCommand = ['install', ...InstallOptions]
+
+export interface Flags {
+  dev?: boolean
+  optional?: boolean
+  production?: boolean
+}
+
+export function createFromFlags (flags: Flags | undefined): InstallCommand {
+  const command: InstallCommand = ['install']
+  if (!flags) return command
+  const { dev, optional, production } = flags
+  if (production && !dev) {
+    command.push('--production')
+  } else if (dev && !production) {
+    command.push('--dev')
+  }
+  if (!optional) {
+    command.push('--no-optional')
+  }
+  return command
+}
+
+export function run (cwd: string, command: InstallCommand): void {
+  const execOpts = {
+    cwd,
+    stdio: 'inherit' as const,
+  }
+  if (path.basename(process.execPath) === 'pnpm') {
+    execSync(process.execPath, command, execOpts)
+  } else if (path.basename(process.argv[1]) === 'pnpm.cjs') {
+    execSync(process.execPath, [process.argv[1], ...command], execOpts)
+  } else {
+    execSync('pnpm', command, execOpts)
+  }
+}

exec/plugin-commands-script-runners/src/runDepsStatusCheck.ts

@@ -1,16 +1,24 @@
-import path from 'path'
-import { sync as execSync } from 'execa'
 import { type VerifyDepsBeforeRun } from '@pnpm/config'
 import { PnpmError } from '@pnpm/error'
 import { globalWarn } from '@pnpm/logger'
-import { checkDepsStatus, type CheckDepsStatusOptions } from '@pnpm/deps.status'
+import { checkDepsStatus, type CheckDepsStatusOptions, type WorkspaceStateSettings } from '@pnpm/deps.status'
 import { prompt } from 'enquirer'
+import * as installCommand from './installCommand'
 
 export type RunDepsStatusCheckOptions = CheckDepsStatusOptions & { dir: string, verifyDepsBeforeRun?: VerifyDepsBeforeRun }
 
 export async function runDepsStatusCheck (opts: RunDepsStatusCheckOptions): Promise<void> {
-  const { upToDate, issue } = await checkDepsStatus(opts)
+  // the following flags are always the default values during `pnpm run` and `pnpm exec`,
+  // so they may not match the workspace state after `pnpm install --production|--no-optional`
+  const ignoredWorkspaceStateSettings = ['dev', 'optional', 'production'] satisfies Array<keyof WorkspaceStateSettings>
+  opts.ignoredWorkspaceStateSettings = ignoredWorkspaceStateSettings
+
+  const { upToDate, issue, workspaceState } = await checkDepsStatus(opts)
   if (upToDate) return
+
+  const command = installCommand.createFromFlags(workspaceState?.settings)
+  const install = installCommand.run.bind(null, opts.dir, command)
+
   switch (opts.verifyDepsBeforeRun) {
   case 'install':
     install()
@@ -21,7 +29,7 @@ export async function runDepsStatusCheck (opts: RunDepsStatusCheckOptions): Prom
       name: 'runInstall',
       message: `Your "node_modules" directory is out of sync with the "pnpm-lock.yaml" file. This can lead to issues during scripts execution.
 
-Would you like to run "pnpm install" to update your "node_modules"?`,
+Would you like to run "pnpm ${command.join(' ')}" to update your "node_modules"?`,
       initial: true,
     })
     if (confirmed.runInstall) {
@@ -37,18 +45,4 @@ Would you like to run "pnpm install" to update your "node_modules"?`,
     globalWarn(`Your node_modules are out of sync with your lockfile. ${issue}`)
     break
   }
-
-  function install () {
-    const execOpts = {
-      cwd: opts.dir,
-      stdio: 'inherit' as const,
-    }
-    if (path.basename(process.execPath) === 'pnpm') {
-      execSync(process.execPath, ['install'], execOpts)
-    } else if (path.basename(process.argv[1]) === 'pnpm.cjs') {
-      execSync(process.execPath, [process.argv[1], 'install'], execOpts)
-    } else {
-      execSync('pnpm', ['install'], execOpts)
-    }
-  }
 }

exec/plugin-commands-script-runners/test/installCommand.test.ts

@@ -0,0 +1,14 @@
+import { type Flags, type InstallCommand, createFromFlags } from '../src/installCommand'
+
+describe(createFromFlags, () => {
+  test.each([
+    [{ production: true, optional: true }, ['install', '--production']],
+    [{ production: true, optional: false }, ['install', '--production', '--no-optional']],
+    [{ dev: true, optional: true }, ['install', '--dev']],
+    [{ dev: true, optional: false }, ['install', '--dev', '--no-optional']],
+    [{ production: true, dev: true, optional: true }, ['install']],
+    [{ production: true, dev: true, optional: false }, ['install', '--no-optional']],
+  ] as Array<[Flags, InstallCommand]>)('%o -> %o', (flags, expected) => {
+    expect(createFromFlags(flags)).toStrictEqual(expected)
+  })
+})

pnpm/test/verifyDepsBeforeRun/install.ts

@@ -0,0 +1,139 @@
+import fs from 'fs'
+import { prepare } from '@pnpm/prepare'
+import { type ProjectManifest } from '@pnpm/types'
+import { loadWorkspaceState } from '@pnpm/workspace.state'
+import { execPnpm, execPnpmSync } from '../utils'
+
+const CONFIG = [
+  '--config.verify-deps-before-run=install',
+  '--reporter=append-only',
+] as const
+
+test('verify-deps-before-run=install reuses the same flags as specified by the workspace state (#9109)', async () => {
+  const manifest: ProjectManifest = {
+    name: 'root',
+    private: true,
+    dependencies: {
+      '@pnpm.e2e/foo': '100.0.0',
+    },
+    devDependencies: {
+      '@pnpm.e2e/bar': '100.0.0',
+    },
+    scripts: {
+      start: 'echo hello from script',
+      postinstall: 'echo install was executed',
+    },
+  }
+
+  const project = prepare(manifest)
+
+  await execPnpm([...CONFIG, 'install'])
+
+  // --production
+  {
+    fs.rmSync('node_modules', { recursive: true })
+    await execPnpm([...CONFIG, 'install', '--production', '--frozen-lockfile'])
+    project.has('@pnpm.e2e/foo')
+    project.hasNot('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: false,
+        optional: true,
+        production: true,
+      },
+    })
+
+    project.writePackageJson({
+      ...manifest,
+      dependencies: {
+        ...manifest.dependencies,
+        '@pnpm.e2e/foo': '100.1.0', // different from the initial manifest
+      },
+    })
+
+    const { stdout } = execPnpmSync([...CONFIG, 'start'], { expectSuccess: true })
+    expect(stdout.toString()).toContain('install was executed')
+    expect(stdout.toString()).toContain('hello from script')
+    project.has('@pnpm.e2e/foo')
+    project.hasNot('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: false,
+        optional: true,
+        production: true,
+      },
+    })
+  }
+
+  // --dev
+  {
+    fs.rmSync('node_modules', { recursive: true })
+    await execPnpm([...CONFIG, 'install', '--dev', '--frozen-lockfile'])
+    project.hasNot('@pnpm.e2e/foo')
+    project.has('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: true,
+        optional: false,
+        production: false,
+      },
+    })
+
+    project.writePackageJson({
+      ...manifest,
+      dependencies: {
+        ...manifest.dependencies,
+        '@pnpm.e2e/foo': '100.0.0', // different from the manifest created by --production
+      },
+    })
+
+    const { stdout } = execPnpmSync([...CONFIG, 'start'], { expectSuccess: true })
+    expect(stdout.toString()).toContain('install was executed')
+    expect(stdout.toString()).toContain('hello from script')
+    project.hasNot('@pnpm.e2e/foo')
+    project.has('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: true,
+        optional: false,
+        production: false,
+      },
+    })
+  }
+
+  // neither --dev nor --production
+  {
+    fs.rmSync('node_modules', { recursive: true })
+    await execPnpm([...CONFIG, 'install', '--frozen-lockfile'])
+    project.has('@pnpm.e2e/foo')
+    project.has('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: true,
+        optional: true,
+        production: true,
+      },
+    })
+
+    project.writePackageJson({
+      ...manifest,
+      dependencies: {
+        ...manifest.dependencies,
+        '@pnpm.e2e/foo': '100.1.0', // different from the manifest created by --dev
+      },
+    })
+
+    const { stdout } = execPnpmSync([...CONFIG, 'start'], { expectSuccess: true })
+    expect(stdout.toString()).toContain('install was executed')
+    expect(stdout.toString()).toContain('hello from script')
+    project.has('@pnpm.e2e/foo')
+    project.has('@pnpm.e2e/bar')
+    expect(loadWorkspaceState(process.cwd())).toMatchObject({
+      settings: {
+        dev: true,
+        optional: true,
+        production: true,
+      },
+    })
+  }
+})