diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 4083d79f4fa25..c7bf9f4e4755a 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -815,7 +815,16 @@ module.exports = cls => class Reifier extends cls { // if the node is optional, then the failure of the promise is nonfatal // just add it and its optional set to the trash list. [_handleOptionalFailure] (node, p) { - return (node.optional ? p.catch(() => { + return (node.optional ? p.catch((e) => { + // Optional dependencies that fail to install due to an incompatible platform + // should not be added to the trash list to prevent rebuild issues when the lockfile is not present. + // This does cause optional dependencies to be present on all platforms in the node_modules directory, + // even though these aren't really used + const trashOptionalDependency = e?.code !== 'EBADPLATFORM' + if (!trashOptionalDependency) { + log.verbose('reify', `skip trashing optional dependency due to platform mismatch`, node.path) + return + } const set = optionalSet(node) for (node of set) { log.verbose('reify', 'failed optional dependency', node.path) diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs index cec3560033241..35cbe71f02d3a 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs @@ -2842,11 +2842,29 @@ ArboristNode { exports[`test/arborist/reify.js TAP do not install optional deps with mismatched platform specifications > expect resolving Promise 1`] = ` ArboristNode { + "children": Map { + "platform-specifying-test-package" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "platform-specifying-test-package", + "spec": "1.0.0", + "type": "optional", + }, + }, + "location": "node_modules/platform-specifying-test-package", + "name": "platform-specifying-test-package", + "optional": true, + "path": "{CWD}/test/arborist/tap-testdir-reify-do-not-install-optional-deps-with-mismatched-platform-specifications/node_modules/platform-specifying-test-package", + "resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz", + "version": "1.0.0", + }, + }, "edgesOut": Map { "platform-specifying-test-package" => EdgeOut { "name": "platform-specifying-test-package", "spec": "1.0.0", - "to": null, + "to": "node_modules/platform-specifying-test-package", "type": "optional", }, }, @@ -3320,11 +3338,29 @@ ArboristNode { exports[`test/arborist/reify.js TAP fail to install optional deps with matched os and matched cpu and mismatched libc with os and cpu and libc options > expect resolving Promise 1`] = ` ArboristNode { + "children": Map { + "platform-specifying-test-package" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "platform-specifying-test-package", + "spec": "1.0.0", + "type": "optional", + }, + }, + "location": "node_modules/platform-specifying-test-package", + "name": "platform-specifying-test-package", + "optional": true, + "path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-matched-cpu-and-mismatched-libc-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package", + "resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz", + "version": "1.0.0", + }, + }, "edgesOut": Map { "platform-specifying-test-package" => EdgeOut { "name": "platform-specifying-test-package", "spec": "1.0.0", - "to": null, + "to": "node_modules/platform-specifying-test-package", "type": "optional", }, }, @@ -3339,11 +3375,29 @@ ArboristNode { exports[`test/arborist/reify.js TAP fail to install optional deps with matched os and mismatched cpu with os and cpu and libc options > expect resolving Promise 1`] = ` ArboristNode { + "children": Map { + "platform-specifying-test-package" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "platform-specifying-test-package", + "spec": "1.0.0", + "type": "optional", + }, + }, + "location": "node_modules/platform-specifying-test-package", + "name": "platform-specifying-test-package", + "optional": true, + "path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-mismatched-cpu-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package", + "resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz", + "version": "1.0.0", + }, + }, "edgesOut": Map { "platform-specifying-test-package" => EdgeOut { "name": "platform-specifying-test-package", "spec": "1.0.0", - "to": null, + "to": "node_modules/platform-specifying-test-package", "type": "optional", }, }, @@ -3358,11 +3412,29 @@ ArboristNode { exports[`test/arborist/reify.js TAP fail to install optional deps with mismatched os and matched cpu with os and cpu and libc options > expect resolving Promise 1`] = ` ArboristNode { + "children": Map { + "platform-specifying-test-package" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "platform-specifying-test-package", + "spec": "1.0.0", + "type": "optional", + }, + }, + "location": "node_modules/platform-specifying-test-package", + "name": "platform-specifying-test-package", + "optional": true, + "path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-mismatched-os-and-matched-cpu-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package", + "resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz", + "version": "1.0.0", + }, + }, "edgesOut": Map { "platform-specifying-test-package" => EdgeOut { "name": "platform-specifying-test-package", "spec": "1.0.0", - "to": null, + "to": "node_modules/platform-specifying-test-package", "type": "optional", }, }, @@ -33177,11 +33249,29 @@ exports[`test/arborist/reify.js TAP scoped registries > should preserve original exports[`test/arborist/reify.js TAP still do not install optional deps with mismatched platform specifications even when forced > expect resolving Promise 1`] = ` ArboristNode { + "children": Map { + "platform-specifying-test-package" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "platform-specifying-test-package", + "spec": "1.0.0", + "type": "optional", + }, + }, + "location": "node_modules/platform-specifying-test-package", + "name": "platform-specifying-test-package", + "optional": true, + "path": "{CWD}/test/arborist/tap-testdir-reify-still-do-not-install-optional-deps-with-mismatched-platform-specifications-even-when-forced/node_modules/platform-specifying-test-package", + "resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz", + "version": "1.0.0", + }, + }, "edgesOut": Map { "platform-specifying-test-package" => EdgeOut { "name": "platform-specifying-test-package", "spec": "1.0.0", - "to": null, + "to": "node_modules/platform-specifying-test-package", "type": "optional", }, }, diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index dcc293b82dc77..8699b5951d589 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -552,9 +552,10 @@ tap.test('failing optional deps are not installed', async t => { const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) await arborist.reify({ installStrategy: 'linked' }) - t.notOk(setupRequire(dir)('which'), 'Failing optional deps should not be installed') + t.ok(setupRequire(dir)('which'), 'Failing optional deps should not be installed, but the empty directory is there') - t.notOk(fs.existsSync(path.join(dir, 'node_modules', '.bin', 'which'))) + t.ok(fs.existsSync(path.join(dir, 'node_modules', 'which'))) + t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'which'))) }) tap.test('Optional deps are installed when possible', async t => { @@ -1319,7 +1320,12 @@ tap.test('failing optional peer deps are not installed', async t => { const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) await arborist.reify({ installStrategy: 'linked' }) - t.notOk(setupRequire(dir)('bar', 'which'), 'Failing optional peer deps should not be installed') + t.ok(setupRequire(dir)('bar', 'which'), 'Failing optional peer deps should not be installed, but an empty directory is there') + + t.ok(fs.existsSync(path.join(dir, 'node_modules', 'which'))) + t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'which'))) + t.ok(fs.existsSync(path.join(dir, 'node_modules', 'bar'))) + t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'bar'))) }) // Virtual packages are 2 packages that have the same version but are @@ -1452,6 +1458,23 @@ function setupRequire (cwd) { } } +/** + * Determine whether the given `path` points to an empty directory. + * @param {String} path + * @returns {boolean} + */ +async function isEmptyDir (path) { + try { + const directory = await fs.opendir(path) + const entry = await directory.read() + await directory.close() + + return entry === null + } catch (error) { + return false + } +} + function pathExists (path) { try { fs.statSync(path)