From e9a2981f55f84ff521ef597883a4e732d08ce1c1 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Thu, 17 Mar 2022 14:14:21 -0400 Subject: [PATCH] fix(arborist): save workspace version (#4578) When declaring dependencies to workspaces the common practice is to refer to their version numbers, currently the cli adds a link reference instead of the proper semver range when trying to install/declare as a direct direct dependency one of its own workspaces. This change fixes it by adding a new condition for handling workspace edges when saving the current ideal tree. Relates to: https://github.com/npm/cli/issues/3403 --- workspaces/arborist/lib/arborist/reify.js | 25 +- .../test/arborist/reify.js.test.cjs | 231 +++++++++++++++++- workspaces/arborist/test/arborist/reify.js | 14 +- 3 files changed, 258 insertions(+), 12 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 4bc1c7ee4e72e..acb889cebca3d 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -1225,14 +1225,23 @@ module.exports = cls => class Reifier extends cls { newSpec = h.shortcut(opt) } } else if (isLocalDep) { - // save the relative path in package.json - // Normally saveSpec is updated with the proper relative - // path already, but it's possible to specify a full absolute - // path initially, in which case we can end up with the wrong - // thing, so just get the ultimate fetchSpec and relativize it. - const p = req.fetchSpec.replace(/^file:/, '') - const rel = relpath(addTree.realpath, p) - newSpec = `file:${rel}` + // when finding workspace nodes, make sure that + // we save them using their version instead of + // using their relative path + if (edge.type === 'workspace') { + const { version } = edge.to.target + const prefixRange = version ? this[_savePrefix] + version : '*' + newSpec = prefixRange + } else { + // save the relative path in package.json + // Normally saveSpec is updated with the proper relative + // path already, but it's possible to specify a full absolute + // path initially, in which case we can end up with the wrong + // thing, so just get the ultimate fetchSpec and relativize it. + const p = req.fetchSpec.replace(/^file:/, '') + const rel = relpath(addTree.realpath, p) + newSpec = `file:${rel}` + } } else { newSpec = req.saveSpec } 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 ffd568c686103..49b02273ec59f 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs @@ -240,6 +240,235 @@ exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be m ` +exports[`test/arborist/reify.js TAP add deps to workspaces add a to root > lockfile added workspace as dep 1`] = ` +Object { + "lockfileVersion": 3, + "name": "tap-testdir-reify-add-deps-to-workspaces-add-a-to-root", + "packages": Object { + "": Object { + "dependencies": Object { + "a": "^1.2.3", + "mkdirp": "^1.0.4", + }, + "workspaces": Array [ + "packages/*", + ], + }, + "node_modules/a": Object { + "link": true, + "resolved": "packages/a", + }, + "node_modules/b": Object { + "link": true, + "resolved": "packages/b", + }, + "node_modules/minimist": Object { + "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", + "version": "1.2.5", + }, + "node_modules/mkdirp": Object { + "bin": Object { + "mkdirp": "bin/cmd.js", + }, + "engines": Object { + "node": ">=10", + }, + "integrity": "sha512-vVqVZQyf3WLx2Shd0qJ9xuvqgAyKPLAiqITEtqW0oIUjzo3PePDd6fW9iFz30ef7Ysp/oiWqbhszeGWW2T6Gzw==", + "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-1.0.4.tgz", + "version": "1.0.4", + }, + "packages/a": Object { + "dependencies": Object { + "mkdirp": "^0.5.0", + }, + "version": "1.2.3", + }, + "packages/a/node_modules/mkdirp": Object { + "bin": Object { + "mkdirp": "bin/cmd.js", + }, + "dependencies": Object { + "minimist": "^1.2.5", + }, + "integrity": "sha512-NKmAlESf6jMGym1++R0Ra7wvhV+wFW63FaSOFPwRahvea0gMUcGUhVeAg/0BC0wiv9ih5NYPB1Wn1UEI1/L+xQ==", + "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.5.tgz", + "version": "0.5.5", + }, + "packages/b": Object { + "version": "1.2.3", + }, + }, + "requires": true, +} +` + +exports[`test/arborist/reify.js TAP add deps to workspaces add a to root > package.json added workspace as dep 1`] = ` +Object { + "dependencies": Object { + "a": "^1.2.3", + "mkdirp": "^1.0.4", + }, + "workspaces": Array [ + "packages/*", + ], +} +` + +exports[`test/arborist/reify.js TAP add deps to workspaces add a to root > returned tree 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/a", + "type": "workspace", + }, + }, + "isWorkspace": true, + "location": "node_modules/a", + "name": "a", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/node_modules/a", + "realpath": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/a", + "resolved": "file:../packages/a", + "target": ArboristNode { + "location": "packages/a", + }, + "version": "1.2.3", + }, + "b" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "b", + "spec": "file:{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/b", + "type": "workspace", + }, + }, + "isWorkspace": true, + "location": "node_modules/b", + "name": "b", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/node_modules/b", + "realpath": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/b", + "resolved": "file:../packages/b", + "target": ArboristNode { + "location": "packages/b", + }, + "version": "1.2.3", + }, + "minimist" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/a/node_modules/mkdirp", + "name": "minimist", + "spec": "^1.2.5", + "type": "prod", + }, + }, + "location": "node_modules/minimist", + "name": "minimist", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/node_modules/minimist", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", + "version": "1.2.5", + }, + "mkdirp" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "mkdirp", + "spec": "^1.0.4", + "type": "prod", + }, + }, + "location": "node_modules/mkdirp", + "name": "mkdirp", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/node_modules/mkdirp", + "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-1.0.4.tgz", + "version": "1.0.4", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/a", + "to": "node_modules/a", + "type": "workspace", + }, + "b" => EdgeOut { + "name": "b", + "spec": "file:{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/b", + "to": "node_modules/b", + "type": "workspace", + }, + "mkdirp" => EdgeOut { + "name": "mkdirp", + "spec": "^1.0.4", + "to": "node_modules/mkdirp", + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "children": Map { + "mkdirp" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/a", + "name": "mkdirp", + "spec": "^0.5.0", + "type": "prod", + }, + }, + "edgesOut": Map { + "minimist" => EdgeOut { + "name": "minimist", + "spec": "^1.2.5", + "to": "node_modules/minimist", + "type": "prod", + }, + }, + "location": "packages/a/node_modules/mkdirp", + "name": "mkdirp", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/a/node_modules/mkdirp", + "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.5.tgz", + "version": "0.5.5", + }, + }, + "edgesOut": Map { + "mkdirp" => EdgeOut { + "name": "mkdirp", + "spec": "^0.5.0", + "to": "packages/a/node_modules/mkdirp", + "type": "prod", + }, + }, + "isWorkspace": true, + "location": "packages/a", + "name": "a", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "isWorkspace": true, + "location": "packages/b", + "name": "b", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root/packages/b", + "version": "1.2.3", + }, + }, + "isProjectRoot": true, + "location": "", + "name": "tap-testdir-reify-add-deps-to-workspaces-add-a-to-root", + "path": "{CWD}/test/arborist/tap-testdir-reify-add-deps-to-workspaces-add-a-to-root", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, +} +` + exports[`test/arborist/reify.js TAP add deps to workspaces add mkdirp 0.5.0 to b > lockfile 1`] = ` Object { "dependencies": Object { @@ -33222,7 +33451,7 @@ Object { "a": "github:foo/bar#baz", "b": "^1.2.3", "d": "npm:c@1.x <1.9.9", - "e": "file:e", + "e": "*", "f": "git+https://user:pass@github.com/baz/quux.git#asdf", "g": "*", "h": "~1.2.3", diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index f69547db406f7..7c22f7c69c183 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -894,8 +894,7 @@ t.test('saving the ideal tree', t => { a: 'git+ssh://git@github.com:foo/bar#baz', b: '', d: 'npm:c@1.x <1.9.9', - // XXX: should we remove dependencies that are also workspaces? - e: 'file:e', + e: '*', f: 'git+https://user:pass@github.com/baz/quux#asdf', g: '', h: '~1.2.3', @@ -1028,7 +1027,7 @@ t.test('saving the ideal tree', t => { a: 'github:foo/bar#baz', b: '^1.2.3', d: 'npm:c@1.x <1.9.9', - e: 'file:e', + e: '*', f: 'git+https://user:pass@github.com/baz/quux.git#asdf', g: '*', h: '~1.2.3', @@ -2045,6 +2044,15 @@ t.test('add deps to workspaces', async t => { t.matchSnapshot(require(path + '/packages/a/package.json'), 'package.json a') t.matchSnapshot(require(path + '/package-lock.json'), 'lockfile') }) + + t.test('add a to root', async t => { + const path = t.testdir(fixture) + await reify(path) + const tree = await reify(path, { add: ['a'], lockfileVersion: 3 }) + t.matchSnapshot(printTree(tree), 'returned tree') + t.matchSnapshot(require(path + '/package.json'), 'package.json added workspace as dep') + t.matchSnapshot(require(path + '/package-lock.json'), 'lockfile added workspace as dep') + }) }) t.test('reify audit only workspace deps when reifying workspace', async t => {