fix: do not download optional dependencies that are not needed (#3672)

close #2038
2026-04-11 02:29:48 -04:00 · 2021-08-17 02:33:36 +03:00
parent 60b122339c
commit 07e7b1c0c3
27 changed files with 189 additions and 99 deletions
--- a/.changeset/gentle-pandas-live.md
+++ b/.changeset/gentle-pandas-live.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/resolve-dependencies": major
+---
+
+Optional dependencies are always marked as `requiresBuild` as they are not always fetched and as a result there is no way to check whethere they need to be built or not.
--- a/.changeset/selfish-clocks-bow.md
+++ b/.changeset/selfish-clocks-bow.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/package-requester": minor
+---
+
+Do not fetch optional packages that are not installable on the target system.
--- a/.changeset/sweet-pianos-sniff.md
+++ b/.changeset/sweet-pianos-sniff.md
@@ -0,0 +1,5 @@
+---
+"pnpm": patch
+---
+
+Those optional dependencies that don't support the target system should not be downloaded from the registry.
--- a/.meta-updater/src/index.ts
+++ b/.meta-updater/src/index.ts
@@ -103,6 +103,7 @@ async function updateManifest (workspaceDir: string, manifest: ProjectManifest,
    break
  case '@pnpm/headless':
  case '@pnpm/outdated':
+  case '@pnpm/package-requester':
  case '@pnpm/plugin-commands-import':
  case '@pnpm/plugin-commands-installation':
  case '@pnpm/plugin-commands-listing':
--- a/packages/package-requester/package.json
+++ b/packages/package-requester/package.json
@@ -14,10 +14,13 @@
  "scripts": {
    "start": "pnpm run tsc -- --watch",
    "lint": "eslint -c ../../eslint.json src/**/*.ts test/**/*.ts",
-    "_test": "jest",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7772 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
-    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
+    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix",
+    "registry-mock": "registry-mock",
+    "test:jest": "jest",
+    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest"
  },
  "repository": "https://github.com/pnpm/pnpm/blob/master/packages/package-requester",
  "keywords": [
@@ -39,6 +42,7 @@
    "@pnpm/error": "workspace:2.0.0",
    "@pnpm/fetcher-base": "workspace:11.0.3",
    "@pnpm/graceful-fs": "workspace:1.0.0",
+    "@pnpm/package-is-installable": "workspace:5.0.4",
    "@pnpm/read-package-json": "workspace:5.0.4",
    "@pnpm/resolver-base": "workspace:8.0.4",
    "@pnpm/store-controller-types": "workspace:11.0.5",
--- a/packages/package-requester/src/packageRequester.ts
+++ b/packages/package-requester/src/packageRequester.ts
@@ -20,6 +20,7 @@ import {
 } from '@pnpm/fetcher-base'
 import gfs from '@pnpm/graceful-fs'
 import logger from '@pnpm/logger'
+import packageIsInstallable from '@pnpm/package-is-installable'
 import readPackage from '@pnpm/read-package-json'
 import {
  DirectoryResolution,
@@ -68,6 +69,10 @@ const pickBundledManifest = pick([

 export default function (
  opts: {
+    engineStrict?: boolean
+    force?: boolean
+    nodeVersion?: string
+    pnpmVersion?: string
    resolve: ResolveFunction
    fetchers: {[type: string]: FetchFunction}
    cafs: Cafs
@@ -103,6 +108,10 @@ export default function (
    verifyStoreIntegrity: opts.verifyStoreIntegrity,
  })
  const requestPackage = resolveAndFetch.bind(null, {
+    engineStrict: opts.engineStrict,
+    nodeVersion: opts.nodeVersion,
+    pnpmVersion: opts.pnpmVersion,
+    force: opts.force,
    fetchPackageToStore,
    requestsQueue,
    resolve: opts.resolve,
@@ -115,13 +124,17 @@ export default function (

 async function resolveAndFetch (
  ctx: {
+    engineStrict?: boolean
+    force?: boolean
+    nodeVersion?: string
+    pnpmVersion?: string
    requestsQueue: {add: <T>(fn: () => Promise<T>, opts: {priority: number}) => Promise<T>}
    resolve: ResolveFunction
    fetchPackageToStore: FetchPackageToStoreFunction
    storeDir: string
    verifyStoreIntegrity: boolean
  },
-  wantedDependency: WantedDependency,
+  wantedDependency: WantedDependency & { optional?: boolean },
  options: RequestPackageOptions
 ): Promise<PackageResponse> {
  let latest: string | undefined
@@ -139,7 +152,7 @@ async function resolveAndFetch (
  // When we don't fetch, the only way to get the package's manifest is via resolving it.
  //
  // The resolution step is never skipped for local dependencies.
-  if (!skipResolution || options.skipFetch === true || Boolean(pkgId?.startsWith('file:'))) {
+  if (!skipResolution || options.skipFetch === true || Boolean(pkgId?.startsWith('file:')) || wantedDependency.optional === true) {
    const resolveResult = await ctx.requestsQueue.add<ResolveResult>(async () => ctx.resolve(wantedDependency, {
      alwaysTryWorkspacePackages: options.alwaysTryWorkspacePackages,
      defaultTag: options.defaultTag,
@@ -188,13 +201,28 @@ async function resolveAndFetch (
    }
  }

+  const isInstallable = (
+    ctx.force === true ||
+      (
+        manifest == null
+          ? undefined
+          : packageIsInstallable(id, manifest, {
+            engineStrict: ctx.engineStrict,
+            lockfileDir: options.lockfileDir,
+            nodeVersion: ctx.nodeVersion,
+            optional: wantedDependency.optional === true,
+            pnpmVersion: ctx.pnpmVersion,
+          })
+      )
+  )
  // We can skip fetching the package only if the manifest
  // is present after resolution
-  if (options.skipFetch && (manifest != null)) {
+  if ((options.skipFetch === true || isInstallable === false) && (manifest != null)) {
    return {
      body: {
        id,
        isLocal: false as const,
+        isInstallable: isInstallable ?? undefined,
        latest,
        manifest,
        normalizedPref,
--- a/packages/package-requester/test/index.ts
+++ b/packages/package-requester/test/index.ts
@@ -7,6 +7,7 @@ import createClient from '@pnpm/client'
 import { streamParser } from '@pnpm/logger'
 import createPackageRequester, { PackageFilesResponse, PackageResponse } from '@pnpm/package-requester'
 import { createCafsStore } from '@pnpm/package-store'
+import { REGISTRY_MOCK_PORT } from '@pnpm/registry-mock'
 import { DependencyManifest } from '@pnpm/types'
 import delay from 'delay'
 import { depPathToFilename } from 'dependency-path'
@@ -16,7 +17,7 @@ import nock from 'nock'
 import normalize from 'normalize-path'
 import tempy from 'tempy'

-const registry = 'https://registry.npmjs.org/'
+const registry = `http://localhost:${REGISTRY_MOCK_PORT}`
 const IS_POSTIVE_TARBALL = path.join(__dirname, 'is-positive-1.0.0.tgz')
 const ncp = promisify(ncpCB as any) // eslint-disable-line @typescript-eslint/no-explicit-any

@@ -52,7 +53,7 @@ test('request package', async () => {
  expect(pkgResponse).toBeTruthy()
  expect(pkgResponse.body).toBeTruthy()

-  expect(pkgResponse.body.id).toBe('registry.npmjs.org/is-positive/1.0.0')
+  expect(pkgResponse.body.id).toBe(`localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`)
  expect(pkgResponse.body.resolvedVia).toBe('npm-registry')
  expect(pkgResponse.body.isLocal).toBe(false)
  expect(typeof pkgResponse.body.latest).toBe('string')
@@ -60,8 +61,8 @@ test('request package', async () => {
  expect(!pkgResponse.body.normalizedPref).toBeTruthy()
  expect(pkgResponse.body.resolution).toStrictEqual({
    integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-    registry: 'https://registry.npmjs.org/',
-    tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+    registry: `http://localhost:${REGISTRY_MOCK_PORT}`,
+    tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
  })

  const files = await pkgResponse.files!()
@@ -97,15 +98,15 @@ test('request package but skip fetching', async () => {
  expect(pkgResponse).toBeTruthy()
  expect(pkgResponse.body).toBeTruthy()

-  expect(pkgResponse.body.id).toBe('registry.npmjs.org/is-positive/1.0.0')
+  expect(pkgResponse.body.id).toBe(`localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`)
  expect(pkgResponse.body.isLocal).toBe(false)
  expect(typeof pkgResponse.body.latest).toBe('string')
  expect(pkgResponse.body.manifest?.name).toBe('is-positive')
  expect(!pkgResponse.body.normalizedPref).toBeTruthy()
  expect(pkgResponse.body.resolution).toStrictEqual({
    integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-    registry: 'https://registry.npmjs.org/',
-    tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+    registry: `http://localhost:${REGISTRY_MOCK_PORT}`,
+    tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
  })

  expect(pkgResponse.files).toBeFalsy()
@@ -130,11 +131,11 @@ test('request package but skip fetching, when resolution is already available',
    currentPkg: {
      name: 'is-positive',
      version: '1.0.0',
-      id: 'registry.npmjs.org/is-positive/1.0.0',
+      id: `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`,
      resolution: {
        integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-        registry: 'https://registry.npmjs.org/',
-        tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+        registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+        tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
      },
    },
    downloadPriority: 0,
@@ -156,15 +157,15 @@ test('request package but skip fetching, when resolution is already available',
  expect(pkgResponse).toBeTruthy()
  expect(pkgResponse.body).toBeTruthy()

-  expect(pkgResponse.body.id).toBe('registry.npmjs.org/is-positive/1.0.0')
+  expect(pkgResponse.body.id).toBe(`localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`)
  expect(pkgResponse.body.isLocal).toBe(false)
  expect(typeof pkgResponse.body.latest).toBe('string')
  expect(pkgResponse.body.manifest.name).toBe('is-positive')
  expect(!pkgResponse.body.normalizedPref).toBeTruthy()
  expect(pkgResponse.body.resolution).toStrictEqual({
    integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-    registry: 'https://registry.npmjs.org/',
-    tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+    registry: `http://localhost:${REGISTRY_MOCK_PORT}`,
+    tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
  })

  expect(pkgResponse.files).toBeFalsy()
@@ -383,7 +384,7 @@ test('fetchPackageToStore()', async () => {
    verifyStoreIntegrity: true,
  })

-  const pkgId = 'registry.npmjs.org/is-positive/1.0.0'
+  const pkgId = `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`
  const fetchResult = packageRequester.fetchPackageToStore({
    force: false,
    lockfileDir: tempy.directory(),
@@ -393,8 +394,8 @@ test('fetchPackageToStore()', async () => {
      id: pkgId,
      resolution: {
        integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-        registry: 'https://registry.npmjs.org/',
-        tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+        registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+        tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
      },
    },
  })
@@ -421,8 +422,8 @@ test('fetchPackageToStore()', async () => {
      id: pkgId,
      resolution: {
        integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-        registry: 'https://registry.npmjs.org/',
-        tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+        registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+        tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
      },
    },
  })
@@ -454,7 +455,7 @@ test('fetchPackageToStore() concurrency check', async () => {
    verifyStoreIntegrity: true,
  })

-  const pkgId = 'registry.npmjs.org/is-positive/1.0.0'
+  const pkgId = `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`
  const projectDir1 = tempy.directory()
  const projectDir2 = tempy.directory()
  const fetchResults = await Promise.all([
@@ -467,8 +468,8 @@ test('fetchPackageToStore() concurrency check', async () => {
        id: pkgId,
        resolution: {
          integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-          registry: 'https://registry.npmjs.org/',
-          tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+          registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+          tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
        },
      },
    }),
@@ -481,8 +482,8 @@ test('fetchPackageToStore() concurrency check', async () => {
        id: pkgId,
        resolution: {
          integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-          registry: 'https://registry.npmjs.org/',
-          tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+          registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+          tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
        },
      },
    }),
@@ -544,7 +545,7 @@ test('fetchPackageToStore() does not cache errors', async () => {
    verifyStoreIntegrity: true,
  })

-  const pkgId = 'registry.npmjs.org/is-positive/1.0.0'
+  const pkgId = `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`

  const badRequest = packageRequester.fetchPackageToStore({
    force: false,
@@ -555,8 +556,8 @@ test('fetchPackageToStore() does not cache errors', async () => {
      id: pkgId,
      resolution: {
        integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-        registry: 'https://registry.npmjs.org/',
-        tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+        registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+        tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
      },
    },
  })
@@ -571,8 +572,8 @@ test('fetchPackageToStore() does not cache errors', async () => {
      id: pkgId,
      resolution: {
        integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-        registry: 'https://registry.npmjs.org/',
-        tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+        registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+        tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
      },
    },
  })
@@ -618,11 +619,11 @@ test('always return a package manifest in the response', async () => {
      currentPkg: {
        name: 'is-positive',
        version: '1.0.0',
-        id: 'registry.npmjs.org/is-positive/1.0.0',
+        id: `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`,
        resolution: {
          integrity: 'sha1-iACYVrZKLx632LsBeUGEJK4EUss=',
-          registry: 'https://registry.npmjs.org/',
-          tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+          registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+          tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
        },
      },
      downloadPriority: 0,
@@ -663,10 +664,10 @@ test('fetchPackageToStore() fetch raw manifest of cached package', async () => {
    verifyStoreIntegrity: true,
  })

-  const pkgId = 'registry.npmjs.org/is-positive/1.0.0'
+  const pkgId = `localhost+${REGISTRY_MOCK_PORT}/is-positive/1.0.0`
  const resolution = {
-    registry: 'https://registry.npmjs.org/',
-    tarball: 'https://registry.npmjs.org/is-positive/-/is-positive-1.0.0.tgz',
+    registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+    tarball: `http://localhost:${REGISTRY_MOCK_PORT}/is-positive/-/is-positive-1.0.0.tgz`,
  }
  const fetchResults = await Promise.all([
    packageRequester.fetchPackageToStore({
@@ -702,10 +703,10 @@ test('refetch package to store if it has been modified', async () => {
  const cafsDir = path.join(storeDir, 'files')
  const lockfileDir = tempy.directory()

-  const pkgId = 'registry.npmjs.org/magic-hook/2.0.0'
+  const pkgId = `localhost+${REGISTRY_MOCK_PORT}/magic-hook/2.0.0`
  const resolution = {
-    registry: 'https://registry.npmjs.org/',
-    tarball: 'https://registry.npmjs.org/magic-hook/-/magic-hook-2.0.0.tgz',
+    registry: `http://localhost:${REGISTRY_MOCK_PORT}/`,
+    tarball: `http://localhost:${REGISTRY_MOCK_PORT}/magic-hook/-/magic-hook-2.0.0.tgz`,
  }

  let indexJsFile!: string
@@ -780,3 +781,35 @@ test('refetch package to store if it has been modified', async () => {
    prefix: lockfileDir,
  }))
 })
+
+test('do not fetch an optional package that is not installable', async () => {
+  const storeDir = '.store'
+  const cafs = createCafsStore(storeDir)
+  const requestPackage = createPackageRequester({
+    resolve,
+    fetchers,
+    cafs,
+    networkConcurrency: 1,
+    storeDir,
+    verifyStoreIntegrity: true,
+  })
+  expect(typeof requestPackage).toBe('function')
+
+  const projectDir = tempy.directory()
+  const pkgResponse = await requestPackage({ alias: 'not-compatible-with-any-os', optional: true, pref: '*' }, {
+    downloadPriority: 0,
+    lockfileDir: projectDir,
+    preferredVersions: {},
+    projectDir,
+    registry,
+  })
+
+  expect(pkgResponse).toBeTruthy()
+  expect(pkgResponse.body).toBeTruthy()
+
+  expect(pkgResponse.body.isInstallable).toBe(false)
+  expect(pkgResponse.body.id).toBe(`localhost+${REGISTRY_MOCK_PORT}/not-compatible-with-any-os/1.0.0`)
+
+  expect(pkgResponse.files).toBeFalsy()
+  expect(pkgResponse.finishing).toBeFalsy()
+})
--- a/packages/package-requester/tsconfig.json
+++ b/packages/package-requester/tsconfig.json
@@ -30,6 +30,9 @@
    {
      "path": "../graceful-fs"
    },
+    {
+      "path": "../package-is-installable"
+    },
    {
      "path": "../read-package-json"
    },
--- a/packages/package-store/src/storeController/index.ts
+++ b/packages/package-store/src/storeController/index.ts
@@ -73,6 +73,10 @@ export default async function (
  resolve: ResolveFunction,
  fetchers: {[type: string]: FetchFunction},
  initOpts: {
+    engineStrict?: boolean
+    force?: boolean
+    nodeVersion?: string
+    pnpmVersion?: string
    ignoreFile?: (filename: string) => boolean
    storeDir: string
    networkConcurrency?: number
@@ -83,6 +87,10 @@ export default async function (
  const storeDir = initOpts.storeDir
  const cafs = createCafsStore(storeDir, initOpts)
  const packageRequester = createPackageRequester({
+    force: initOpts.force,
+    engineStrict: initOpts.engineStrict,
+    nodeVersion: initOpts.nodeVersion,
+    pnpmVersion: initOpts.pnpmVersion,
    resolve,
    fetchers,
    cafs,
--- a/packages/plugin-commands-import/package.json
+++ b/packages/plugin-commands-import/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7772 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7773 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-installation/package.json
+++ b/packages/plugin-commands-installation/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7773 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7774 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-listing/package.json
+++ b/packages/plugin-commands-listing/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7774 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7775 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-outdated/package.json
+++ b/packages/plugin-commands-outdated/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7775 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7776 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-publishing/package.json
+++ b/packages/plugin-commands-publishing/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7776 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7777 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-rebuild/package.json
+++ b/packages/plugin-commands-rebuild/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7777 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7778 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-script-runners/package.json
+++ b/packages/plugin-commands-script-runners/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7778 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7779 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/plugin-commands-store/package.json
+++ b/packages/plugin-commands-store/package.json
@@ -16,7 +16,7 @@
    "registry-mock": "registry-mock",
    "test:jest": "jest",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7779 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7780 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm run compile",
    "compile": "rimraf lib tsconfig.tsbuildinfo && tsc --build && pnpm run lint -- --fix"
--- a/packages/pnpm/package.json
+++ b/packages/pnpm/package.json
@@ -156,7 +156,7 @@
    "test:jest": "jest",
    "pretest:e2e": "rimraf node_modules/.bin/pnpm",
    "test:e2e": "registry-mock prepare && run-p -r registry-mock test:jest",
-    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7780 pnpm run test:e2e",
+    "_test": "cross-env PNPM_REGISTRY_MOCK_PORT=7781 pnpm run test:e2e",
    "test": "pnpm run compile && pnpm run _test",
    "prepublishOnly": "pnpm compile && npm cache clear --force && publish-packed --prune --npm-client yarn --dest dist",
    "postpublish": "publish-packed",
--- a/packages/resolve-dependencies/package.json
+++ b/packages/resolve-dependencies/package.json
@@ -34,7 +34,6 @@
    "@pnpm/lockfile-utils": "workspace:3.0.8",
    "@pnpm/manifest-utils": "workspace:2.0.4",
    "@pnpm/npm-resolver": "workspace:12.0.2",
-    "@pnpm/package-is-installable": "workspace:5.0.4",
    "@pnpm/pick-registry-for-package": "workspace:2.0.4",
    "@pnpm/prune-lockfile": "workspace:3.0.8",
    "@pnpm/read-package-json": "workspace:5.0.4",
--- a/packages/resolve-dependencies/src/index.ts
+++ b/packages/resolve-dependencies/src/index.ts
@@ -176,7 +176,7 @@ export default async function (
  const { newLockfile, pendingRequiresBuilds } = updateLockfile(dependenciesGraph, opts.wantedLockfile, opts.virtualStoreDir, opts.registries) // eslint-disable-line:prefer-const

  // waiting till package requests are finished
-  const waitTillAllFetchingsFinish = async () => Promise.all(Object.values(resolvedPackagesByDepPath).map(async ({ finishing }) => finishing()))
+  const waitTillAllFetchingsFinish = async () => Promise.all(Object.values(resolvedPackagesByDepPath).map(async ({ finishing }) => finishing?.()))

  return {
    dependenciesByProjectId,
@@ -197,22 +197,27 @@ async function finishLockfileUpdates (
 ) {
  return Promise.all(pendingRequiresBuilds.map(async (depPath) => {
    const depNode = dependenciesGraph[depPath]
-    if (depNode.fetchingBundledManifest == null) {
+    if (depNode.optional) {
+      // We assume that all optional dependencies have to be built.
+      // Optional dependencies are not always downloaded, so there is no way to know whether they need to be built or not.
+      depNode.requiresBuild = true
+    } else if (depNode.fetchingBundledManifest != null) {
+      const filesResponse = await depNode.fetchingFiles()
+      // The npm team suggests to always read the package.json for deciding whether the package has lifecycle scripts
+      const pkgJson = await depNode.fetchingBundledManifest()
+      depNode.requiresBuild = Boolean(
+        pkgJson.scripts != null && (
+          Boolean(pkgJson.scripts.preinstall) ||
+          Boolean(pkgJson.scripts.install) ||
+          Boolean(pkgJson.scripts.postinstall)
+        ) ||
+        filesResponse.filesIndex['binding.gyp'] ||
+          Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this
+      )
+    } else {
      // This should never ever happen
      throw new Error(`Cannot create ${WANTED_LOCKFILE} because raw manifest (aka package.json) wasn't fetched for "${depPath}"`)
    }
-    const filesResponse = await depNode.fetchingFiles()
-    // The npm team suggests to always read the package.json for deciding whether the package has lifecycle scripts
-    const pkgJson = await depNode.fetchingBundledManifest()
-    depNode.requiresBuild = Boolean(
-      pkgJson.scripts != null && (
-        Boolean(pkgJson.scripts.preinstall) ||
-        Boolean(pkgJson.scripts.install) ||
-        Boolean(pkgJson.scripts.postinstall)
-      ) ||
-      filesResponse.filesIndex['binding.gyp'] ||
-        Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this
-    )

    // TODO: try to cover with unit test the case when entry is no longer available in lockfile
    // It is an edge that probably happens if the entry is removed during lockfile prune
--- a/packages/resolve-dependencies/src/resolveDependencies.ts
+++ b/packages/resolve-dependencies/src/resolveDependencies.ts
@@ -16,7 +16,6 @@ import {
  pkgSnapshotToResolution,
 } from '@pnpm/lockfile-utils'
 import logger from '@pnpm/logger'
-import packageIsInstallable from '@pnpm/package-is-installable'
 import pickRegistryForPackage from '@pnpm/pick-registry-for-package'
 import {
  DirectoryResolution,
@@ -752,22 +751,12 @@ async function resolveDependency (
    ? pkgResponse.body.id
    : createNodeId(options.parentPkg.nodeId, depPath)

-  const currentIsInstallable = (
-    ctx.force ||
-      packageIsInstallable(pkgResponse.body.id, pkg, {
-        engineStrict: ctx.engineStrict,
-        lockfileDir: ctx.lockfileDir,
-        nodeVersion: ctx.nodeVersion,
-        optional: wantedDependency.optional,
-        pnpmVersion: ctx.pnpmVersion,
-      })
-  )
  const parentIsInstallable = options.parentPkg.installable === undefined || options.parentPkg.installable
-  const installable = parentIsInstallable && currentIsInstallable !== false
+  const installable = parentIsInstallable && pkgResponse.body.isInstallable !== false
  const isNew = !ctx.resolvedPackagesByDepPath[depPath]

  if (isNew) {
-    if (currentIsInstallable !== true || !parentIsInstallable) {
+    if (pkgResponse.body.isInstallable === false || !parentIsInstallable) {
      ctx.skipped.add(pkgResponse.body.id)
    }
    progressLogger.debug({
--- a/packages/resolve-dependencies/tsconfig.json
+++ b/packages/resolve-dependencies/tsconfig.json
@@ -33,9 +33,6 @@
    {
      "path": "../npm-resolver"
    },
-    {
-      "path": "../package-is-installable"
-    },
    {
      "path": "../pick-registry-for-package"
    },
--- a/packages/store-connection-manager/src/createNewStoreController.ts
+++ b/packages/store-connection-manager/src/createNewStoreController.ts
@@ -2,6 +2,7 @@ import { promises as fs } from 'fs'
 import createClient from '@pnpm/client'
 import { Config } from '@pnpm/config'
 import createStore from '@pnpm/package-store'
+import pnpm from '@pnpm/cli-meta'

 type CreateResolverOptions = Pick<Config,
 | 'fetchRetries'
@@ -17,6 +18,9 @@ type CreateResolverOptions = Pick<Config,
 export type CreateNewStoreControllerOptions = CreateResolverOptions & Pick<Config,
 | 'ca'
 | 'cert'
+| 'engineStrict'
+| 'force'
+| 'nodeVersion'
 | 'fetchTimeout'
 | 'httpProxy'
 | 'httpsProxy'
@@ -67,6 +71,10 @@ export default async (
  await fs.mkdir(opts.storeDir, { recursive: true })
  return {
    ctrl: await createStore(resolve, fetchers, {
+      engineStrict: opts.engineStrict,
+      force: opts.force,
+      nodeVersion: opts.nodeVersion,
+      pnpmVersion: pnpm.version,
      ignoreFile: opts.ignoreFile,
      networkConcurrency: opts.networkConcurrency,
      packageImportMethod: opts.packageImportMethod,
--- a/packages/store-controller-types/src/index.ts
+++ b/packages/store-controller-types/src/index.ts
@@ -66,7 +66,7 @@ export interface FetchPackageToStoreOptions {
 }

 export type RequestPackageFunction = (
-  wantedDependency: WantedDependency,
+  wantedDependency: WantedDependency & { optional?: boolean },
  options: RequestPackageOptions
 ) => Promise<PackageResponse>

@@ -98,6 +98,7 @@ export interface PackageResponse {
  finishing?: () => Promise<void> // a package request is finished once its integrity is generated and saved
  body: {
    isLocal: boolean
+    isInstallable?: boolean
    resolution: Resolution
    manifest?: PackageManifest
    id: string
--- a/packages/supi/test/install/installationChecks.ts
+++ b/packages/supi/test/install/installationChecks.ts
@@ -6,15 +6,13 @@ import { testDefaults } from '../utils'
 test('fail if installed package does not support the current engine and engine-strict = true', async () => {
  const project = prepareEmpty()

-  try {
-    await addDependenciesToPackage({}, ['not-compatible-with-any-os'], await testDefaults({
+  await expect(
+    addDependenciesToPackage({}, ['not-compatible-with-any-os'], await testDefaults({}, {}, {}, {
      engineStrict: true,
    }))
-    throw new Error('tests failed')
-  } catch (err) {
-    await project.hasNot('not-compatible-with-any-os')
-    await project.storeHasNot('not-compatible-with-any-os', '1.0.0')
-  }
+  ).rejects.toThrow()
+  await project.hasNot('not-compatible-with-any-os')
+  await project.storeHasNot('not-compatible-with-any-os', '1.0.0')
 })

 test('do not fail if installed package does not support the current engine and engine-strict = false', async () => {
--- a/packages/supi/test/install/optionalDependencies.ts
+++ b/packages/supi/test/install/optionalDependencies.ts
@@ -74,6 +74,11 @@ test('skip optional dependency that does not support the current OS', async () =

  const lockfile = await project.readLockfile()
  expect(lockfile.packages['/not-compatible-with-any-os/1.0.0']).toBeTruthy()
+
+  // optional dependencies always get requiresBuild: true
+  // this is to resolve https://github.com/pnpm/pnpm/issues/2038
+  expect(lockfile.packages['/not-compatible-with-any-os/1.0.0'].requiresBuild).toBeTruthy()
+
  expect(lockfile.packages['/dep-of-optional-pkg/1.0.0']).toBeTruthy()

  const currentLockfile = await project.readCurrentLockfile()
@@ -165,11 +170,9 @@ test('skip optional dependency that does not support the current pnpm version',
      'for-legacy-pnpm': '*',
    },
  }, await testDefaults({
-    packageManager: {
-      name: 'pnpm',
-      version: '4.0.0',
-    },
    reporter,
+  }, {}, {}, {
+    pnpmVersion: '4.0.0',
  }))

  await project.hasNot('for-legacy-pnpm')
@@ -194,9 +197,7 @@ test('don\'t skip optional dependency that does not support the current OS when
    optionalDependencies: {
      'not-compatible-with-any-os': '*',
    },
-  }, await testDefaults({
-    force: true,
-  }))
+  }, await testDefaults({}, {}, {}, { force: true }))

  await project.has('not-compatible-with-any-os')
  await project.storeHas('not-compatible-with-any-os', '1.0.0')
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -1566,6 +1566,7 @@ importers:
      '@pnpm/fetcher-base': workspace:11.0.3
      '@pnpm/graceful-fs': workspace:1.0.0
      '@pnpm/logger': ^4.0.0
+      '@pnpm/package-is-installable': workspace:5.0.4
      '@pnpm/package-requester': 'link:'
      '@pnpm/package-store': workspace:12.0.12
      '@pnpm/read-package-json': workspace:5.0.4
@@ -1597,6 +1598,7 @@ importers:
      '@pnpm/error': link:../error
      '@pnpm/fetcher-base': link:../fetcher-base
      '@pnpm/graceful-fs': link:../graceful-fs
+      '@pnpm/package-is-installable': link:../package-is-installable
      '@pnpm/read-package-json': link:../read-package-json
      '@pnpm/resolver-base': link:../resolver-base
      '@pnpm/store-controller-types': link:../store-controller-types
@@ -2812,7 +2814,6 @@ importers:
      '@pnpm/logger': ^4.0.0
      '@pnpm/manifest-utils': workspace:2.0.4
      '@pnpm/npm-resolver': workspace:12.0.2
-      '@pnpm/package-is-installable': workspace:5.0.4
      '@pnpm/pick-registry-for-package': workspace:2.0.4
      '@pnpm/prune-lockfile': workspace:3.0.8
      '@pnpm/read-package-json': workspace:5.0.4
@@ -2838,7 +2839,6 @@ importers:
      '@pnpm/lockfile-utils': link:../lockfile-utils
      '@pnpm/manifest-utils': link:../manifest-utils
      '@pnpm/npm-resolver': link:../npm-resolver
-      '@pnpm/package-is-installable': link:../package-is-installable
      '@pnpm/pick-registry-for-package': link:../pick-registry-for-package
      '@pnpm/prune-lockfile': link:../prune-lockfile
      '@pnpm/read-package-json': link:../read-package-json