fix(overrides): move invalid peers to prod deps (#9000)

close #8978

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/slow-elephants-greet.md

@@ -0,0 +1,9 @@
+---
+"@pnpm/hooks.read-package-hook": patch
+"@pnpm/semver.peer-range": major
+"@pnpm/resolve-dependencies": patch
+"@pnpm/core": patch
+"pnpm": patch
+---
+
+Prevent `overrides` from adding invalid version ranges to `peerDependencies` by keeping the `peerDependencies` and overriding them with prod `dependencies` [#8978](https://github.com/pnpm/pnpm/issues/8978).

cspell.json

@@ -260,6 +260,7 @@
     "todomvc",
     "tsparticles",
     "typecheck",
+    "unallowed",
     "underperformance",
     "undollar",
     "uninstallation",

hooks/read-package-hook/package.json

@@ -33,6 +33,7 @@
     "@pnpm/parse-overrides": "workspace:*",
     "@pnpm/parse-wanted-dependency": "workspace:*",
     "@pnpm/types": "workspace:*",
+    "@pnpm/semver.peer-range": "workspace:*",
     "@yarnpkg/extensions": "catalog:",
     "normalize-path": "catalog:",
     "ramda": "catalog:",

hooks/read-package-hook/src/createVersionsOverrider.ts

@@ -3,6 +3,7 @@ import semver from 'semver'
 import partition from 'ramda/src/partition'
 import { type Dependencies, type PackageManifest, type ReadPackageHook } from '@pnpm/types'
 import { type PackageSelector, type VersionOverride as VersionOverrideBase } from '@pnpm/parse-overrides'
+import { isValidPeerRange } from '@pnpm/semver.peer-range'
 import normalizePath from 'normalize-path'
 import { isIntersectingRange } from './isIntersectingRange'
 
@@ -68,20 +69,28 @@ function overrideDepsOfPkg (
   genericVersionOverrides: VersionOverride[]
 ): void {
   const { dependencies, optionalDependencies, devDependencies, peerDependencies } = manifest
-  for (const deps of [dependencies, optionalDependencies, devDependencies, peerDependencies]) {
+  const _overrideDeps = overrideDeps.bind(null, { versionOverrides, genericVersionOverrides, dir })
+  for (const deps of [dependencies, optionalDependencies, devDependencies]) {
     if (deps) {
-      overrideDeps(versionOverrides, genericVersionOverrides, deps, dir)
+      _overrideDeps(deps, undefined)
     }
   }
+  if (peerDependencies) {
+    if (!manifest.dependencies) manifest.dependencies = {}
+    _overrideDeps(manifest.dependencies, peerDependencies)
+  }
 }
 
 function overrideDeps (
-  versionOverrides: VersionOverrideWithParent[],
-  genericVersionOverrides: VersionOverride[],
+  { versionOverrides, genericVersionOverrides, dir }: {
+    versionOverrides: VersionOverrideWithParent[]
+    genericVersionOverrides: VersionOverride[]
+    dir: string | undefined
+  },
   deps: Dependencies,
-  dir: string | undefined
+  peerDeps: Dependencies | undefined
 ): void {
-  for (const [name, pref] of Object.entries(deps)) {
+  for (const [name, pref] of Object.entries(peerDeps ?? deps)) {
     const versionOverride =
     pickMostSpecificVersionOverride(
       versionOverrides.filter(
@@ -98,15 +107,22 @@ function overrideDeps (
     if (!versionOverride) continue
 
     if (versionOverride.newPref === '-') {
-      delete deps[versionOverride.targetPkg.name]
+      if (peerDeps) {
+        delete peerDeps[versionOverride.targetPkg.name]
+      } else {
+        delete deps[versionOverride.targetPkg.name]
+      }
       continue
     }
 
-    if (versionOverride.localTarget) {
-      deps[versionOverride.targetPkg.name] = `${versionOverride.localTarget.protocol}${resolveLocalOverride(versionOverride.localTarget, dir)}`
-      continue
+    const newPref = versionOverride.localTarget
+      ? `${versionOverride.localTarget.protocol}${resolveLocalOverride(versionOverride.localTarget, dir)}`
+      : versionOverride.newPref
+    if (peerDeps == null || !isValidPeerRange(newPref)) {
+      deps[versionOverride.targetPkg.name] = newPref
+    } else if (isValidPeerRange(newPref)) {
+      peerDeps[versionOverride.targetPkg.name] = newPref
     }
-    deps[versionOverride.targetPkg.name] = versionOverride.newPref
   }
 }
 

hooks/read-package-hook/test/createVersionOverrider.test.ts

@@ -610,13 +610,14 @@ test('createVersionsOverrider() overrides peerDependencies of another dependency
   ).toStrictEqual({
     name: 'react-dom',
     version: '18.2.0',
+    dependencies: {},
     peerDependencies: {
       react: '18.1.0',
     },
   })
 })
 
-test('createVersionOverrider() removes dependencies', () => {
+test('createVersionsOverrider() removes dependencies', () => {
   const overrider = createVersionsOverrider([
     {
       targetPkg: {
@@ -689,3 +690,76 @@ test('createVersionOverrider() removes dependencies', () => {
     },
   })
 })
+
+test('createVersionsOverrider() moves invalid versions from peerDependencies to dependencies', () => {
+  const overrider = createVersionsOverrider([
+    {
+      targetPkg: {
+        name: 'foo',
+      },
+      newPref: 'link:foo',
+    },
+    {
+      targetPkg: {
+        name: 'bar',
+      },
+      newPref: 'file:bar',
+    },
+    {
+      targetPkg: {
+        name: 'baz',
+      },
+      newPref: '7.7.7',
+    },
+  ], process.cwd())
+  expect(
+    overrider({
+      peerDependencies: {
+        foo: '^1.0.0 || ^2.0.0',
+        bar: '^1.0.0 || ^2.0.0',
+        baz: '^1.0.0 || ^2.0.0',
+        qux: '^1.0.0 || ^2.0.0',
+      },
+    })
+  ).toStrictEqual({
+    dependencies: {
+      foo: expect.stringMatching(/^link:.*foo[/\\]?$/),
+      bar: expect.stringMatching(/^file:.*bar[/\\]?$/),
+    },
+    peerDependencies: {
+      bar: '^1.0.0 || ^2.0.0',
+      baz: '7.7.7',
+      foo: '^1.0.0 || ^2.0.0',
+      qux: '^1.0.0 || ^2.0.0',
+    },
+  })
+  expect(
+    overrider({
+      dependencies: {
+        foo: '^1.0.0',
+        bar: '^2.0.0',
+        baz: '^1.2.3',
+        qux: '^2.1.0',
+      },
+      peerDependencies: {
+        foo: '^1.0.0 || ^2.0.0',
+        bar: '^1.0.0 || ^2.0.0',
+        baz: '^1.0.0 || ^2.0.0',
+        qux: '^1.0.0 || ^2.0.0',
+      },
+    })
+  ).toStrictEqual({
+    dependencies: {
+      foo: expect.stringMatching(/^link:.*foo[/\\]?$/),
+      bar: expect.stringMatching(/^file:.*bar[/\\]?$/),
+      baz: '7.7.7',
+      qux: '^2.1.0',
+    },
+    peerDependencies: {
+      bar: '^1.0.0 || ^2.0.0',
+      baz: '7.7.7',
+      foo: '^1.0.0 || ^2.0.0',
+      qux: '^1.0.0 || ^2.0.0',
+    },
+  })
+})

hooks/read-package-hook/tsconfig.json

@@ -23,6 +23,9 @@
     },
     {
       "path": "../../packages/types"
+    },
+    {
+      "path": "../../semver/peer-range"
     }
   ]
 }

pkg-manager/core/test/install/validatePeerDependencies.ts

@@ -0,0 +1,100 @@
+import fs from 'fs'
+import path from 'path'
+import { install } from '@pnpm/core'
+import { readWantedLockfile } from '@pnpm/lockfile.fs'
+import { preparePackages } from '@pnpm/prepare'
+import { testDefaults } from '../utils'
+
+test('throws an error when the peerDependencies have unallowed specs', async () => {
+  preparePackages([
+    {
+      name: 'foo',
+      version: '1.0.0',
+      private: true,
+    },
+  ])
+
+  const { rejects } = expect(
+    install({
+      name: 'root',
+      version: '0.0.0',
+      private: true,
+      peerDependencies: {
+        foo: 'link:foo',
+      },
+    }, testDefaults())
+  )
+
+  await rejects.toHaveProperty(['code'], 'ERR_PNPM_INVALID_PEER_DEPENDENCY_SPECIFICATION')
+  await rejects.toHaveProperty(['message'], "The peerDependencies field named 'foo' of package 'root' has an invalid value: 'link:foo'")
+})
+
+test('overrides are not prevented from replacing peerDependencies with local links', async () => {
+  preparePackages([
+    {
+      name: 'fake-is-positive',
+      version: '1.0.0',
+      private: true,
+    },
+  ])
+
+  const overrides = {
+    'is-positive': 'link:fake-is-positive',
+  }
+
+  await install({
+    name: 'root',
+    version: '0.0.0',
+    private: true,
+    dependencies: {
+      'is-positive': '1.0.0',
+    },
+    peerDependencies: {
+      'is-positive': '^1.0.0',
+    },
+    pnpm: { overrides },
+  }, testDefaults({ overrides }))
+
+  expect(await readWantedLockfile('.', { ignoreIncompatible: false })).toMatchObject({
+    overrides,
+    importers: {
+      '.': {
+        dependencies: {
+          'is-positive': overrides['is-positive'],
+        },
+        specifiers: {
+          'is-positive': overrides['is-positive'],
+        },
+      },
+    },
+  })
+
+  expect(fs.realpathSync('node_modules/is-positive')).toBe(path.resolve('fake-is-positive'))
+})
+
+test("empty overrides don't disable peer dependencies validation", async () => {
+  preparePackages([
+    {
+      name: 'foo',
+      version: '1.0.0',
+      private: true,
+    },
+  ])
+
+  const overrides = {}
+
+  const { rejects } = expect(
+    install({
+      name: 'root',
+      version: '0.0.0',
+      private: true,
+      peerDependencies: {
+        foo: 'link:foo',
+      },
+      pnpm: { overrides },
+    }, testDefaults({ overrides }))
+  )
+
+  await rejects.toHaveProperty(['code'], 'ERR_PNPM_INVALID_PEER_DEPENDENCY_SPECIFICATION')
+  await rejects.toHaveProperty(['message'], "The peerDependencies field named 'foo' of package 'root' has an invalid value: 'link:foo'")
+})

pkg-manager/resolve-dependencies/package.json

@@ -47,6 +47,7 @@
     "@pnpm/pick-registry-for-package": "workspace:*",
     "@pnpm/read-package-json": "workspace:*",
     "@pnpm/resolver-base": "workspace:*",
+    "@pnpm/semver.peer-range": "workspace:*",
     "@pnpm/store-controller-types": "workspace:*",
     "@pnpm/types": "workspace:*",
     "@pnpm/which-version-is-pinned": "workspace:*",

pkg-manager/resolve-dependencies/src/validatePeerDependencies.ts

@@ -1,6 +1,6 @@
 import { PnpmError } from '@pnpm/error'
 import { type ProjectManifest } from '@pnpm/types'
-import { validRange } from 'semver'
+import { isValidPeerRange } from '@pnpm/semver.peer-range'
 
 export interface ProjectToValidate {
   rootDir: string
@@ -12,7 +12,7 @@ export function validatePeerDependencies (project: ProjectToValidate): void {
   const projectId = name ?? project.rootDir
   for (const depName in peerDependencies) {
     const version = peerDependencies[depName]
-    if (!isValidPeerVersion(version)) {
+    if (!isValidPeerRange(version)) {
       throw new PnpmError(
         'INVALID_PEER_DEPENDENCY_SPECIFICATION',
         `The peerDependencies field named '${depName}' of package '${projectId}' has an invalid value: '${version}'`,
@@ -23,8 +23,3 @@ export function validatePeerDependencies (project: ProjectToValidate): void {
     }
   }
 }
-
-function isValidPeerVersion (version: string): boolean {
-  // we use `includes` instead of `startsWith` because `workspace:*` and `catalog:*` could be a part of a wider version range expression
-  return typeof validRange(version) === 'string' || version.includes('workspace:') || version.includes('catalog:')
-}

pkg-manager/resolve-dependencies/tsconfig.json

@@ -72,6 +72,9 @@
     {
       "path": "../../resolving/resolver-base"
     },
+    {
+      "path": "../../semver/peer-range"
+    },
     {
       "path": "../../store/store-controller-types"
     },

pnpm-lock.yaml

@@ -3011,6 +3011,9 @@ importers:
       '@pnpm/parse-wanted-dependency':
         specifier: workspace:*
         version: link:../../packages/parse-wanted-dependency
+      '@pnpm/semver.peer-range':
+        specifier: workspace:*
+        version: link:../../semver/peer-range
       '@pnpm/types':
         specifier: workspace:*
         version: link:../../packages/types
@@ -5526,6 +5529,9 @@ importers:
       '@pnpm/resolver-base':
         specifier: workspace:*
         version: link:../../resolving/resolver-base
+      '@pnpm/semver.peer-range':
+        specifier: workspace:*
+        version: link:../../semver/peer-range
       '@pnpm/store-controller-types':
         specifier: workspace:*
         version: link:../../store/store-controller-types
@@ -7124,6 +7130,19 @@ importers:
         specifier: 'catalog:'
         version: '@types/table@6.0.0'
 
+  semver/peer-range:
+    dependencies:
+      semver:
+        specifier: 'catalog:'
+        version: 7.6.2
+    devDependencies:
+      '@pnpm/semver.peer-range':
+        specifier: workspace:*
+        version: 'link:'
+      '@types/semver':
+        specifier: 'catalog:'
+        version: 7.5.3
+
   store/cafs:
     dependencies:
       '@pnpm/fetcher-base':
@@ -15693,7 +15712,7 @@ snapshots:
 
   '@npmcli/fs@4.0.0':
     dependencies:
-      semver: 7.6.2
+      semver: 7.6.3
 
   '@pkgjs/parseargs@0.11.0':
     optional: true

pnpm-workspace.yaml

@@ -32,6 +32,7 @@ packages:
   - releasing/*
   - resolving/*
   - reviewing/*
+  - semver/*
   - store/*
   - text/*
   - workspace/*

semver/peer-range/README.md

@@ -0,0 +1,15 @@
+# @pnpm/semver.peer-range
+
+> Validates peer ranges
+
+[![npm version](https://img.shields.io/npm/v/@pnpm/semver.peer-range.svg)](https://www.npmjs.com/package/@pnpm/semver.peer-range)
+
+## Installation
+
+```sh
+pnpm add @pnpm/semver.peer-range
+```
+
+## License
+
+MIT

semver/peer-range/package.json

@@ -0,0 +1,46 @@
+{
+  "name": "@pnpm/semver.peer-range",
+  "version": "1000.0.0-0",
+  "description": "Validates peer ranges",
+  "main": "lib/index.js",
+  "types": "lib/index.d.ts",
+  "files": [
+    "lib",
+    "!*.map"
+  ],
+  "engines": {
+    "node": ">=18.12"
+  },
+  "scripts": {
+    "lint": "eslint \"src/**/*.ts\"",
+    "test": "pnpm run compile",
+    "prepublishOnly": "pnpm run compile",
+    "compile": "tsc --build && pnpm run lint --fix"
+  },
+  "repository": "https://github.com/pnpm/pnpm/blob/main/semver/peer-range",
+  "keywords": [
+    "pnpm10",
+    "pnpm",
+    "semver",
+    "peer"
+  ],
+  "license": "MIT",
+  "bugs": {
+    "url": "https://github.com/pnpm/pnpm/issues"
+  },
+  "homepage": "https://github.com/pnpm/pnpm/blob/main/semver/peer-range#readme",
+  "dependencies": {
+    "semver": "catalog:"
+  },
+  "devDependencies": {
+    "@pnpm/semver.peer-range": "workspace:*",
+    "@types/semver": "catalog:"
+  },
+  "funding": "https://opencollective.com/pnpm",
+  "exports": {
+    ".": "./lib/index.js"
+  },
+  "jest": {
+    "preset": "@pnpm/jest-config"
+  }
+}

semver/peer-range/src/index.ts

@@ -0,0 +1,6 @@
+import { validRange } from 'semver'
+
+export function isValidPeerRange (version: string): boolean {
+  // we use `includes` instead of `startsWith` because `workspace:*` and `catalog:*` could be a part of a wider version range expression
+  return typeof validRange(version) === 'string' || version.includes('workspace:') || version.includes('catalog:')
+}

semver/peer-range/tsconfig.json

@@ -0,0 +1,12 @@
+{
+  "extends": "@pnpm/tsconfig",
+  "compilerOptions": {
+    "outDir": "lib",
+    "rootDir": "src"
+  },
+  "include": [
+    "src/**/*.ts",
+    "../../__typings__/**/*.d.ts"
+  ],
+  "references": []
+}

semver/peer-range/tsconfig.lint.json

@@ -0,0 +1,8 @@
+{
+  "extends": "./tsconfig.json",
+  "include": [
+    "src/**/*.ts",
+    "test/**/*.ts",
+    "../../__typings__/**/*.d.ts"
+  ]
+}