Skip to content

Commit

Permalink
fix(arborist): save workspace version (#4578)
Browse files Browse the repository at this point in the history 
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: #3403
  • Loading branch information
@ruyadorno
ruyadorno committed Mar 17, 2022
1 parent 84d1921 commit e9a2981
Show file tree
Hide file tree
Showing 3 changed files with 258 additions and 12 deletions.
25 changes: 17 additions & 8 deletions workspaces/arborist/lib/arborist/reify.js
View file
Expand Up @@ -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
}
Expand Down
231 changes: 230 additions & 1 deletion workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down
14 changes: 11 additions & 3 deletions workspaces/arborist/test/arborist/reify.js
View file
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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 => {
Expand Down

0 comments on commit e9a2981

Please sign in to comment.