diff --git a/lib/commands/ls.js b/lib/commands/ls.js index 5dacd3919882e..9c57a4482e82b 100644 --- a/lib/commands/ls.js +++ b/lib/commands/ls.js @@ -412,7 +412,9 @@ const filterLinkedStrategyEdges = ({ node, currentDepth }) => { ))) return (edge) => { - // Skip workspace edges for undeclared workspaces at root level + // Skip workspace edges for undeclared workspaces at root level. + // loadActual now creates in-memory Links for workspace targets so workspace edges should always resolve. + /* istanbul ignore next - defensive, workspace edges resolve via in-memory Links */ if (currentDepth === 0 && edge.type === 'workspace' && edge.missing) { if (!declaredDeps.has(edge.name)) { return false diff --git a/tap-snapshots/workspaces/arborist/test/calc-dep-flags.js.test.cjs b/tap-snapshots/workspaces/arborist/test/calc-dep-flags.js.test.cjs index acdc2a937a41c..315579e6feead 100644 --- a/tap-snapshots/workspaces/arborist/test/calc-dep-flags.js.test.cjs +++ b/tap-snapshots/workspaces/arborist/test/calc-dep-flags.js.test.cjs @@ -175,6 +175,7 @@ ArboristNode { "location": "node_modules/metapeerdep", "name": "metapeerdep", "path": "/x/node_modules/metapeerdep", + "peer": true, "version": "1.2.3", }, "optional" => ArboristNode { @@ -185,6 +186,12 @@ ArboristNode { "spec": "*", "type": "optional", }, + EdgeIn { + "from": "node_modules/peeroptional", + "name": "optional", + "spec": "*", + "type": "prod", + }, }, "edgesOut": Map { "devoptional" => EdgeOut { @@ -242,6 +249,32 @@ ArboristNode { "location": "node_modules/peerdep", "name": "peerdep", "path": "/x/node_modules/peerdep", + "peer": true, + "version": "1.2.3", + }, + "peeroptional" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "peeroptional", + "spec": "*", + "type": "peerOptional", + }, + }, + "edgesOut": Map { + "optional" => EdgeOut { + "name": "optional", + "spec": "*", + "to": "node_modules/optional", + "type": "prod", + }, + }, + "extraneous": true, + "location": "node_modules/peeroptional", + "name": "peeroptional", + "optional": true, + "path": "/x/node_modules/peeroptional", + "peer": true, "version": "1.2.3", }, "prod" => ArboristNode { @@ -326,6 +359,12 @@ ArboristNode { "to": "node_modules/peer", "type": "peer", }, + "peeroptional" => EdgeOut { + "name": "peeroptional", + "spec": "*", + "to": "node_modules/peeroptional", + "type": "peerOptional", + }, "prod" => EdgeOut { "name": "prod", "spec": "*", @@ -401,408 +440,22 @@ ArboristNode { "location": "node_modules/foo", "name": "foo", "path": "/some/path/node_modules/foo", - "version": "1.2.3", - }, - }, - "dev": true, - "edgesOut": Map { - "foo" => EdgeOut { - "name": "foo", - "spec": "*", - "to": "node_modules/foo", - "type": "prod", - }, - }, - "isProjectRoot": true, - "location": "", - "name": "path", - "path": "/some/path", -} -` - -exports[`workspaces/arborist/test/calc-dep-flags.js TAP peer dependency with optional dependency > after calcDepFlags 1`] = ` -ArboristNode { - "children": Map { - "B" => ArboristNode { - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "B", - "spec": "1.0.0", - "type": "prod", - }, - }, - "edgesOut": Map { - "C" => EdgeOut { - "name": "C", - "spec": "1.0.0", - "to": "node_modules/C", - "type": "peer", - }, - }, - "location": "node_modules/B", - "name": "B", - "path": "/project/node_modules/B", - "version": "1.0.0", - }, - "C" => ArboristNode { - "edgesIn": Set { - EdgeIn { - "from": "node_modules/B", - "name": "C", - "spec": "1.0.0", - "type": "peer", - }, - }, - "edgesOut": Map { - "D" => EdgeOut { - "name": "D", - "spec": "1.0.0", - "to": "node_modules/D", - "type": "optional", - }, - }, - "location": "node_modules/C", - "name": "C", - "path": "/project/node_modules/C", - "peer": true, - "version": "1.0.0", - }, - "D" => ArboristNode { - "edgesIn": Set { - EdgeIn { - "from": "node_modules/C", - "name": "D", - "spec": "1.0.0", - "type": "optional", - }, - }, - "location": "node_modules/D", - "name": "D", - "optional": true, - "path": "/project/node_modules/D", - "version": "1.0.0", - }, - }, - "edgesOut": Map { - "B" => EdgeOut { - "name": "B", - "spec": "1.0.0", - "to": "node_modules/B", - "type": "prod", - }, - }, - "isProjectRoot": true, - "location": "", - "name": "project", - "packageName": "A", - "path": "/project", - "version": "1.0.0", -} -` - -exports[`workspaces/arborist/test/calc-dep-flags.js TAP peer dependency with optional dependency > before calcDepFlags 1`] = ` -ArboristNode { - "children": Map { - "B" => ArboristNode { - "dev": true, - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "B", - "spec": "1.0.0", - "type": "prod", - }, - }, - "edgesOut": Map { - "C" => EdgeOut { - "name": "C", - "spec": "1.0.0", - "to": "node_modules/C", - "type": "peer", - }, - }, - "extraneous": true, - "location": "node_modules/B", - "name": "B", - "optional": true, - "path": "/project/node_modules/B", - "peer": true, - "version": "1.0.0", - }, - "C" => ArboristNode { - "dev": true, - "edgesIn": Set { - EdgeIn { - "from": "node_modules/B", - "name": "C", - "spec": "1.0.0", - "type": "peer", - }, - }, - "edgesOut": Map { - "D" => EdgeOut { - "name": "D", - "spec": "1.0.0", - "to": "node_modules/D", - "type": "optional", - }, - }, - "extraneous": true, - "location": "node_modules/C", - "name": "C", - "optional": true, - "path": "/project/node_modules/C", - "peer": true, - "version": "1.0.0", - }, - "D" => ArboristNode { - "dev": true, - "edgesIn": Set { - EdgeIn { - "from": "node_modules/C", - "name": "D", - "spec": "1.0.0", - "type": "optional", - }, - }, - "extraneous": true, - "location": "node_modules/D", - "name": "D", - "optional": true, - "path": "/project/node_modules/D", - "peer": true, - "version": "1.0.0", - }, - }, - "dev": true, - "edgesOut": Map { - "B" => EdgeOut { - "name": "B", - "spec": "1.0.0", - "to": "node_modules/B", - "type": "prod", - }, - }, - "extraneous": true, - "isProjectRoot": true, - "location": "", - "name": "project", - "optional": true, - "packageName": "A", - "path": "/project", - "peer": true, - "version": "1.0.0", -} -` - -exports[`workspaces/arborist/test/calc-dep-flags.js TAP set parents to not extraneous when visiting > after 1`] = ` -ArboristNode { - "children": Map { - "asdf" => ArboristNode { - "children": Map { - "baz" => ArboristNode { - "location": "node_modules/asdf/node_modules/baz", - "name": "baz", - "path": "/some/path/node_modules/asdf/node_modules/baz", - "version": "1.2.3", - }, - }, - "location": "node_modules/asdf", - "name": "asdf", - "path": "/some/path/node_modules/asdf", - "version": "1.2.3", - }, - "baz" => ArboristLink { - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "baz", - "spec": "file:node_modules/asdf/node_modules/baz", - "type": "prod", - }, - }, - "location": "node_modules/baz", - "name": "baz", - "path": "/some/path/node_modules/baz", - "realpath": "/some/path/node_modules/asdf/node_modules/baz", - "resolved": "file:asdf/node_modules/baz", - "target": ArboristNode { - "location": "node_modules/asdf/node_modules/baz", - }, - "version": "1.2.3", - }, - "foo" => ArboristLink { - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "foo", - "spec": "file:bar/foo", - "type": "prod", - }, - }, - "location": "node_modules/foo", - "name": "foo", - "path": "/some/path/node_modules/foo", - "realpath": "/some/path/bar/foo", - "resolved": "file:../bar/foo", - "target": ArboristNode { - "location": "bar/foo", - }, - "version": "1.2.3", - }, - }, - "edgesOut": Map { - "baz" => EdgeOut { - "name": "baz", - "spec": "file:node_modules/asdf/node_modules/baz", - "to": "node_modules/baz", - "type": "prod", - }, - "foo" => EdgeOut { - "name": "foo", - "spec": "file:bar/foo", - "to": "node_modules/foo", - "type": "prod", - }, - }, - "fsChildren": Set { - ArboristNode { - "fsChildren": Set { - ArboristNode { - "location": "bar/foo", - "name": "foo", - "path": "/some/path/bar/foo", - "version": "1.2.3", - }, - }, - "location": "bar", - "name": "bar", - "path": "/some/path/bar", - }, - }, - "isProjectRoot": true, - "location": "", - "name": "path", - "path": "/some/path", -} -` - -exports[`workspaces/arborist/test/calc-dep-flags.js TAP set parents to not extraneous when visiting > before 1`] = ` -ArboristNode { - "children": Map { - "asdf" => ArboristNode { - "children": Map { - "baz" => ArboristNode { - "dev": true, - "extraneous": true, - "location": "node_modules/asdf/node_modules/baz", - "name": "baz", - "optional": true, - "path": "/some/path/node_modules/asdf/node_modules/baz", - "peer": true, - "version": "1.2.3", - }, - }, - "dev": true, - "extraneous": true, - "location": "node_modules/asdf", - "name": "asdf", - "optional": true, - "path": "/some/path/node_modules/asdf", - "peer": true, - "version": "1.2.3", - }, - "baz" => ArboristLink { - "dev": true, - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "baz", - "spec": "file:node_modules/asdf/node_modules/baz", - "type": "prod", - }, - }, - "extraneous": true, - "location": "node_modules/baz", - "name": "baz", - "optional": true, - "path": "/some/path/node_modules/baz", - "peer": true, - "realpath": "/some/path/node_modules/asdf/node_modules/baz", - "resolved": "file:asdf/node_modules/baz", - "target": ArboristNode { - "location": "node_modules/asdf/node_modules/baz", - }, - "version": "1.2.3", - }, - "foo" => ArboristLink { - "dev": true, - "edgesIn": Set { - EdgeIn { - "from": "", - "name": "foo", - "spec": "file:bar/foo", - "type": "prod", - }, - }, - "extraneous": true, - "location": "node_modules/foo", - "name": "foo", - "optional": true, - "path": "/some/path/node_modules/foo", "peer": true, - "realpath": "/some/path/bar/foo", - "resolved": "file:../bar/foo", - "target": ArboristNode { - "location": "bar/foo", - }, "version": "1.2.3", }, }, "dev": true, "edgesOut": Map { - "baz" => EdgeOut { - "name": "baz", - "spec": "file:node_modules/asdf/node_modules/baz", - "to": "node_modules/baz", - "type": "prod", - }, "foo" => EdgeOut { "name": "foo", - "spec": "file:bar/foo", + "spec": "*", "to": "node_modules/foo", "type": "prod", }, }, - "extraneous": true, - "fsChildren": Set { - ArboristNode { - "dev": true, - "extraneous": true, - "fsChildren": Set { - ArboristNode { - "dev": true, - "extraneous": true, - "location": "bar/foo", - "name": "foo", - "optional": true, - "path": "/some/path/bar/foo", - "peer": true, - "version": "1.2.3", - }, - }, - "location": "bar", - "name": "bar", - "optional": true, - "path": "/some/path/bar", - "peer": true, - }, - }, "isProjectRoot": true, "location": "", "name": "path", - "optional": true, "path": "/some/path", "peer": true, } diff --git a/test/lib/commands/ls.js b/test/lib/commands/ls.js index ab98773bc68e5..59893381a5ce0 100644 --- a/test/lib/commands/ls.js +++ b/test/lib/commands/ls.js @@ -5376,8 +5376,8 @@ t.test('ls --install-strategy=linked', async t => { t.match(output, /nopt/, 'should list the dependency') }) - t.test('should still report declared workspace as UNMET DEPENDENCY when missing', async t => { - const { ls } = await mockLs(t, { + t.test('should resolve declared workspace even without symlink', async t => { + const { result, ls } = await mockLs(t, { config: { 'install-strategy': 'linked', }, @@ -5397,11 +5397,12 @@ t.test('ls --install-strategy=linked', async t => { }, }, node_modules: { - // workspace-a is declared but its symlink is missing + // workspace-a symlink is missing but loadActual creates an in-memory Link }, }, }) - await t.rejects(ls.exec([]), { code: 'ELSPROBLEMS' }, - 'should report declared workspace as UNMET DEPENDENCY') + await ls.exec([]) + const output = cleanCwd(result()) + t.match(output, /workspace-a/, 'should resolve workspace without symlink') }) }) diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index 2356227a5baa3..30bd04a3fb4c5 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/workspaces/arborist/lib/arborist/load-actual.js @@ -171,6 +171,30 @@ module.exports = cls => class ActualLoader extends cls { } } await Promise.all(promises) + + // In linked strategy, workspaces not explicitly declared in root's dependencies don't get symlinks in root's node_modules. + // Create Links so that root's workspace edges can resolve, allowing calcDepFlags to reach workspace packages. + if (this.options.installStrategy === 'linked') { + for (const [name, wsPath] of this.#actualTree.workspaces.entries()) { + if (!this.#actualTree.children.has(name)) { + const target = this.#cache.get(wsPath) + // target is always present since workspace paths are loaded above via #loadFSNode + /* istanbul ignore next - defensive check for unexpected cache miss */ + if (!target) { + continue + } + const linkPath = resolve(this.#actualTree.path, 'node_modules', name) + const link = new Link({ + name, + path: linkPath, + realpath: wsPath, + target, + parent: this.#actualTree, + }) + this.#cache.set(linkPath, link) + } + } + } } if (!ignoreMissing) { diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 5f2484858094d..c1843550478de 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -29,18 +29,35 @@ const calcDepFlags = (tree, resetRoot = true) => { } } - // for links, map their hierarchy appropriately + // For links, map their hierarchy appropriately. + // Only unset flags (true→false), never set them back. + // A target may be reached through multiple Links (e.g. root-level + workspace-level in the linked install strategy). + // Each Link may have different flags based on the path that reached it. + // We must keep the "strongest" (false) classification from any Link, so we only propagate when unsetting. if (node.isLink) { // node.target can be null, we check to ensure it's not null before proceeding if (node.target == null) { continue } - node.target.dev = node.dev - node.target.optional = node.optional - node.target.devOptional = node.devOptional - node.target.peer = node.peer - node.target.extraneous = node.extraneous - queue.push(node.target) + if (node.target.extraneous && !node.extraneous) { + node.target.extraneous = false + } + if (node.target.dev && !node.dev) { + node.target.dev = false + } + if (node.target.optional && !node.optional) { + node.target.optional = false + } + if (node.target.devOptional && !node.devOptional) { + node.target.devOptional = false + } + if (node.target.peer && !node.peer) { + node.target.peer = false + } + // Always queue the target if not yet seen, so its edges get processed. + if (!seen.has(node.target)) { + queue.push(node.target) + } continue } diff --git a/workspaces/arborist/test/arborist/load-actual.js b/workspaces/arborist/test/arborist/load-actual.js index 94ad4e7269286..bb134226e4b14 100644 --- a/workspaces/arborist/test/arborist/load-actual.js +++ b/workspaces/arborist/test/arborist/load-actual.js @@ -365,6 +365,47 @@ t.test('load workspaces when loading from hidden lockfile', async t => { t.matchSnapshot(tree, 'actual tree') }) +t.test('linked strategy creates in-memory links for workspaces without root symlinks', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'my-project', + version: '1.0.0', + workspaces: ['packages/*'], + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.0.0', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.0.0', + dependencies: { a: '^1.0.0' }, + }), + }, + }, + node_modules: { + // Workspace 'a' already has a symlink in root's node_modules (covers the children.has branch). + a: t.fixture('symlink', '../packages/a'), + '.store': {}, + }, + }) + + const tree = await loadActual(path, { installStrategy: 'linked' }) + + // Both workspaces should be resolvable as children of root + t.ok(tree.children.has('a'), 'workspace a is a child of root') + t.ok(tree.children.has('b'), 'workspace b is a child of root') + + const childA = tree.children.get('a') + const childB = tree.children.get('b') + t.ok(childA.isLink, 'workspace a child is a Link') + t.ok(childB.isLink, 'workspace b child is a Link') +}) + t.test('recalc dep flags for virtual load actual', async t => { const path = t.testdir({ node_modules: { diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index b371ae55f6e4b..de47e55383bf0 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -343,6 +343,105 @@ t.test('set parents to not extraneous when visiting', t => { t.end() }) +// Simulates the linked install strategy where the same target has multiple +// Links: one through a prod path (root -> workspace link) and one through a +// dev path (another workspace's devDependencies -> workspace link). The target +// should be dev=false because it IS reachable through a prod path. +t.test('multiple links to same target keep strongest flag', t => { + // root depends on both workspaces (prod) + const root = new Node({ + path: '/project', + realpath: '/project', + pkg: { + dependencies: { app: '', tools: '' }, + }, + }) + + // workspace target for "tools" (lives outside node_modules) + const toolsTarget = new Node({ + path: '/project/packages/tools', + realpath: '/project/packages/tools', + root, + pkg: { + name: 'tools', + version: '1.0.0', + dependencies: { dep: '' }, + }, + }) + + // "dep" is a prod dependency of tools + const dep = new Node({ + pkg: { name: 'dep', version: '1.0.0' }, + parent: root, + }) + + // Link1: root-level link (prod path from root) + const toolsLink1 = new Link({ + name: 'tools', + target: toolsTarget, + parent: root, + realpath: toolsTarget.path, + }) + + // workspace target for "app" + const appTarget = new Node({ + path: '/project/packages/app', + realpath: '/project/packages/app', + root, + pkg: { + name: 'app', + version: '1.0.0', + devDependencies: { tools: '' }, + }, + }) + + // Link for app at root level (prod path from root). + // Construction wires appTarget into root's children via parent. + new Link({ + name: 'app', + target: appTarget, + parent: root, + realpath: appTarget.path, + }) + + // Link2: workspace-level link (dev path from app -> tools) + const toolsLink2 = new Link({ + name: 'tools', + target: toolsTarget, + parent: appTarget, + realpath: toolsTarget.path, + }) + + calcDepFlags(root) + + t.match(toolsTarget, { + dev: false, + extraneous: false, + }, 'tools target should be dev=false (reachable via prod from root)') + + t.match(toolsLink1, { + dev: false, + extraneous: false, + }, 'tools root-level link should be dev=false') + + t.match(dep, { + dev: false, + extraneous: false, + }, 'dep (prod dep of tools) should be dev=false') + + t.match(appTarget, { + dev: false, + extraneous: false, + }, 'app target should be dev=false') + + t.match(toolsLink2, { + dev: true, + extraneous: false, + }, 'tools workspace-level link should be dev=true (reached via dev edge)') + + t.end() +}) + t.test('check null target in link', async t => { const root = new Link({ path: '/some/path',