From 050284d2abb6aa91a0f9ffad5b0c4f074e5dbf6d Mon Sep 17 00:00:00 2001 From: nlf Date: Mon, 1 Aug 2022 11:20:50 -0700 Subject: [PATCH] fix(arborist): pass the edge to fromPath in order to determine correct path (#5233) by passing in the edge we can determine if the edge is overridden, and if it is the path we want to return is the project root since that's what user's will have define their overrides relative to --- .../arborist/lib/arborist/build-ideal-tree.js | 3 +- workspaces/arborist/lib/dep-valid.js | 2 +- workspaces/arborist/lib/from-path.js | 19 +- .../tap-snapshots/test/edge.js.test.cjs | 189 ++++++++++++++++++ workspaces/arborist/test/dep-valid.js | 53 ++--- workspaces/arborist/test/edge.js | 3 + workspaces/arborist/test/from-path.js | 29 ++- 7 files changed, 266 insertions(+), 32 deletions(-) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 0e98ed6fc533c..945bae56b63de 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -1076,9 +1076,8 @@ This is a one-time fix-up, please be patient... // if it fails at this point, though, dont' worry because it // may well be an optional dep that has gone missing. it'll // fail later anyway. - const from = fromPath(placed) promises.push(...this[_problemEdges](placed).map(e => - this[_fetchManifest](npa.resolve(e.name, e.spec, from)) + this[_fetchManifest](npa.resolve(e.name, e.spec, fromPath(placed, e))) .catch(er => null))) }, }) diff --git a/workspaces/arborist/lib/dep-valid.js b/workspaces/arborist/lib/dep-valid.js index c69ab557ae491..d02f397fcdf17 100644 --- a/workspaces/arborist/lib/dep-valid.js +++ b/workspaces/arborist/lib/dep-valid.js @@ -20,7 +20,7 @@ const depValid = (child, requested, requestor) => { // file: deps that depend on other files/dirs, we must resolve the // location based on the *requestor* file/dir, not where it ends up. // '' is equivalent to '*' - requested = npa.resolve(child.name, requested || '*', fromPath(requestor)) + requested = npa.resolve(child.name, requested || '*', fromPath(requestor, requestor.edgesOut.get(child.name))) } catch (er) { // Not invalid because the child doesn't match, but because // the spec itself is not supported. Nothing would match, diff --git a/workspaces/arborist/lib/from-path.js b/workspaces/arborist/lib/from-path.js index 2a3617844c001..1006f73af3d07 100644 --- a/workspaces/arborist/lib/from-path.js +++ b/workspaces/arborist/lib/from-path.js @@ -6,8 +6,19 @@ const { dirname } = require('path') const npa = require('npm-package-arg') -const fromPath = (node, spec) => - spec && spec.type === 'file' ? dirname(spec.fetchSpec) - : node.realpath +const fromPath = (node, spec, edge) => { + if (edge && edge.overrides && edge.overrides.name === edge.name && edge.overrides.value) { + // fromPath could be called with a node that has a virtual root, if that happens + // we want to make sure we get the real root node when overrides are in use. this + // is to allow things like overriding a dependency with a tarball file that's a + // relative path from the project root + return node.sourceReference + ? node.sourceReference.root.realpath + : node.root.realpath + } -module.exports = node => fromPath(node, node.resolved && npa(node.resolved)) + return spec && spec.type === 'file' ? dirname(spec.fetchSpec) + : node.realpath +} + +module.exports = (node, edge) => fromPath(node, node.resolved && npa(node.resolved), edge) diff --git a/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs b/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs index 6feae28b570d6..17dc0b0c9fb0b 100644 --- a/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs @@ -88,6 +88,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "invalid": false, @@ -188,6 +209,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "invalid": true, @@ -345,6 +387,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "resolve": Function resolve(n), @@ -414,6 +477,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "resolve": Function resolve(n), @@ -614,6 +698,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "resolve": Function resolve(n), @@ -697,6 +802,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "a" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "type": "peer", @@ -748,6 +874,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "invalid": false, @@ -814,6 +961,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "resolve": Function resolve(n), @@ -868,6 +1036,27 @@ Edge { "version": "1.2.3", }, "resolve": Function resolve(n), + "root": Object { + "addEdgeIn": Function addEdgeIn(edge), + "addEdgeOut": Function addEdgeOut(edge), + "edgesIn": Set {}, + "edgesOut": Map { + "missing" => Edge { + "peerConflicted": false, + }, + }, + "explain": Function explain(), + "isTop": true, + "name": "top", + "package": Object { + "name": "top", + "version": "1.2.3", + }, + "packageName": "top", + "parent": null, + "resolve": Function resolve(n), + "version": "1.2.3", + }, "version": "1.2.3", }, "invalid": false, diff --git a/workspaces/arborist/test/dep-valid.js b/workspaces/arborist/test/dep-valid.js index 901325b2390b9..8c0e761658873 100644 --- a/workspaces/arborist/test/dep-valid.js +++ b/workspaces/arborist/test/dep-valid.js @@ -4,7 +4,12 @@ const npa = require('npm-package-arg') const { normalizePaths } = require('./fixtures/utils.js') const { resolve } = require('path') -t.ok(depValid({}, '', null, {}), '* is always ok') +// dep-valid reads from requestor.edgesOut so we use this instead of {} in these tests +const emptyRequestor = { + edgesOut: new Map(), +} + +t.ok(depValid({}, '', null, emptyRequestor), '* is always ok') t.ok(depValid({ package: { @@ -13,7 +18,7 @@ t.ok(depValid({ get version () { return this.package.version }, -}, '1.x', null, {}), 'range that is satisfied') +}, '1.x', null, emptyRequestor), 'range that is satisfied') t.ok(depValid({ package: { @@ -22,21 +27,21 @@ t.ok(depValid({ get version () { return this.package.version }, -}, '1.x', '2.x', {}), 'range that is acceptable') +}, '1.x', '2.x', emptyRequestor), 'range that is acceptable') t.ok(depValid({ isLink: true, realpath: '/some/path', -}, normalizePaths(npa('file:/some/path')), null, {}), 'links must point at intended target') +}, normalizePaths(npa('file:/some/path')), null, emptyRequestor), 'links must point at intended target') t.notOk(depValid({ isLink: true, realpath: '/some/other/path', -}, 'file:/some/path', null, {}), 'links must point at intended target') +}, 'file:/some/path', null, emptyRequestor), 'links must point at intended target') t.notOk(depValid({ realpath: '/some/path', -}, 'file:/some/path', null, {}), 'file:// must be a link') +}, 'file:/some/path', null, emptyRequestor), 'file:// must be a link') t.ok(depValid({ name: 'foo', @@ -47,7 +52,7 @@ t.ok(depValid({ get version () { return this.package.version }, -}, 'git://host/repo#semver:1.x', null, {}), 'git url with semver range') +}, 'git://host/repo#semver:1.x', null, emptyRequestor), 'git url with semver range') t.ok(depValid({ name: 'foo', @@ -58,7 +63,7 @@ t.ok(depValid({ get version () { return this.package.version }, -}, 'npm:bar@1.2.3', null, {}), 'alias is ok') +}, 'npm:bar@1.2.3', null, emptyRequestor), 'alias is ok') t.ok(depValid({ resolved: 'https://registry/abbrev-1.1.1.tgz', @@ -66,7 +71,7 @@ t.ok(depValid({ get version () { return this.package.version }, -}, 'https://registry/abbrev-1.1.1.tgz', null, {}), 'remote url match') +}, 'https://registry/abbrev-1.1.1.tgz', null, emptyRequestor), 'remote url match') t.ok(depValid({ resolved: 'git+ssh://git@github.com/foo/bar', @@ -74,7 +79,7 @@ t.ok(depValid({ get version () { return this.package.version }, -}, 'git+ssh://git@github.com/foo/bar.git', null, {}), 'matching _from saveSpec') +}, 'git+ssh://git@github.com/foo/bar.git', null, emptyRequestor), 'matching _from saveSpec') t.notOk(depValid({ resolved: 'git+ssh://git@github.com/foo/bar', @@ -82,26 +87,26 @@ t.notOk(depValid({ get version () { return this.package.version }, -}, 'git+ssh://git@github.com/bar/foo.git', null, {}), 'different repo') +}, 'git+ssh://git@github.com/bar/foo.git', null, emptyRequestor), 'different repo') t.notOk(depValid({ package: {}, get version () { return this.package.version }, -}, 'git+ssh://git@github.com/bar/foo.git', null, {}), 'missing repo') +}, 'git+ssh://git@github.com/bar/foo.git', null, emptyRequestor), 'missing repo') t.ok(depValid({ resolved: `file:${resolve('/path/to/tarball.tgz')}`, -}, resolve('/path/to/tarball.tgz'), null, {}), 'same tarball') +}, resolve('/path/to/tarball.tgz'), null, emptyRequestor), 'same tarball') t.notOk(depValid({ resolved: 'file:/path/to/other/tarball.tgz', -}, '/path/to/tarball.tgz', null, {}), 'different tarball') +}, '/path/to/tarball.tgz', null, emptyRequestor), 'different tarball') t.notOk(depValid({ isLink: true, -}, '/path/to/tarball.tgz', null, {}), 'links are not tarballs') +}, '/path/to/tarball.tgz', null, emptyRequestor), 'links are not tarballs') t.ok(depValid({ package: { @@ -112,28 +117,28 @@ t.ok(depValid({ get version () { return this.package.version }, -}, './tarball.tgz', null, {}), 'probably the same-ish, hopefully') +}, './tarball.tgz', null, emptyRequestor), 'probably the same-ish, hopefully') t.notOk(depValid({ package: {}, get version () { return this.package.version }, -}, './tarball.tgz', null, {}), 'too uncertain, nope') +}, './tarball.tgz', null, emptyRequestor), 'too uncertain, nope') t.ok(depValid({ resolved: 'https://registry.npmjs.org/foo/foo-1.2.3.tgz', -}, 'latest', null, {}), 'tagged registry version needs remote tarball') +}, 'latest', null, emptyRequestor), 'tagged registry version needs remote tarball') t.notOk(depValid({ resolved: 'git+https://registry.npmjs.org/foo/foo-1.2.3.git', -}, 'latest', null, {}), 'tagged registry version needs remote tarball, not git') +}, 'latest', null, emptyRequestor), 'tagged registry version needs remote tarball, not git') -t.notOk(depValid({}, 'latest', null, {}), +t.notOk(depValid({}, 'latest', null, emptyRequestor), 'tagged registry version needs remote tarball resolution') t.test('unsupported dependency type', t => { - const requestor = { errors: [] } + const requestor = { errors: [], edgesOut: new Map() } const child = { name: 'kid' } const request = { type: 'not a type' } t.notOk(depValid(child, request, null, requestor)) @@ -148,7 +153,7 @@ t.test('unsupported dependency type', t => { }) t.test('invalid tag name', t => { - const requestor = { errors: [] } + const requestor = { errors: [], edgesOut: new Map() } const child = { name: 'kid' } const request = '!!@#$%!#@$!' t.notOk(depValid(child, request, null, requestor)) @@ -163,7 +168,7 @@ t.test('invalid tag name', t => { }) t.test('invalid request all together', t => { - const requestor = { errors: [] } + const requestor = { errors: [], edgesOut: new Map() } const child = { name: 'kid' } const request = null t.notOk(depValid(child, request, null, requestor)) @@ -178,7 +183,7 @@ t.test('invalid request all together', t => { }) t.test('installLinks makes Link nodes invalid', t => { - const requestor = { errors: [], installLinks: true } + const requestor = { errors: [], installLinks: true, edgesOut: new Map() } const child = { isLink: true, name: 'kid' } const request = { type: 'directory' } t.notOk(depValid(child, request, null, requestor)) diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index 9d1352351741e..f45cb87917702 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -71,6 +71,7 @@ const a = { }, isTop: false, parent: top, + root: top, resolve (n) { return n === 'aa' ? aa : this.parent.resolve(n) }, @@ -1008,6 +1009,7 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) return this.package.version }, parent: root, + root, resolve (n) { return n === 'bar' ? bar : this.parent.resolve(n) }, @@ -1036,6 +1038,7 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) return this.package.version }, parent: foo, + root, resolve (n) { return this.parent.resolve(n) }, diff --git a/workspaces/arborist/test/from-path.js b/workspaces/arborist/test/from-path.js index 5b5ea2084c498..3bfd0b6398a74 100644 --- a/workspaces/arborist/test/from-path.js +++ b/workspaces/arborist/test/from-path.js @@ -1,7 +1,7 @@ const t = require('tap') const { normalizePath } = require('./fixtures/utils.js') const fp = require('../lib/from-path.js') -const fromPath = obj => normalizePath(fp(obj)) +const fromPath = (node, edge) => normalizePath(fp(node, edge)) t.equal(fromPath({ realpath: '/some/path', @@ -21,3 +21,30 @@ t.equal(fromPath({ realpath: '/some/path/to/install/target', resolved: 'https://registry.com/package.tgz', }), '/some/path/to/install/target', 'use dirname if not dir or file type') + +t.equal(fromPath({ + root: { + realpath: '/some/root', + }, +}, { + name: 'foo', + overrides: { + name: 'foo', + value: 'foo@2', + }, +}), '/some/root', 'uses root realpath for overridden edges') + +t.equal(fromPath({ + sourceReference: { + root: { + realpath: '/some/root', + }, + }, + realpath: '/some/red/herring', +}, { + name: 'foo', + overrides: { + name: 'foo', + value: 'foo@2', + }, +}), '/some/root', 'uses sourceReferences root realpath for overridden edges')