From d8eb545c1e40c8d4897735929da73acdbf396cc5 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 24 Mar 2021 12:46:37 -0700 Subject: [PATCH 01/17] notes about filtered reification --- notes/filtered-reify.md | 93 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 notes/filtered-reify.md diff --git a/notes/filtered-reify.md b/notes/filtered-reify.md new file mode 100644 index 000000000..169f0a1f7 --- /dev/null +++ b/notes/filtered-reify.md @@ -0,0 +1,93 @@ +# filtered reify + +Use case: call `Arborist.reify()` but _only_ reify nodes that are the +dependencies of one specific node. + +Examples: + +- Global reification: we don't want to have to read the entire global space + to install a new package globally. Anything that is not in the + `explicitRequests` list should be ignored. Since `global` triggers + `globalStyle` bundling, this means that effectively any other top-level + node should be untouched. Then if it is present in the idealTree but not + in the actualTree, we don't end up deleting it, which has been the source + of some very unfortunate bugs. + +- Workspace reification: we want to be able to say `npm i -w foo -w bar` to + only install specific workspaces and their (possibly hoisted/shared) + dependencies. + +The goal here would be that if we accidentally have more nodes in the +actualTree that are outside of the filter, we don't accidentally remove +them, or if we have nodes in the idealTree that are outside the filter, we +don't accidentally add/change them. + +## approach 1 - limit actualTree and idealTree to filtered subtree + +This is closest to the current behavior with respect to global installs. +However, rather than relying on starting with a filtered actualTree, and +trusting that we only built the idealTree correctly, we'd add a step where +we explicitly filter/prune both trees to only include the dependencies of +the starting Node. + +Advantage is that reify and diff can remain unaware of dependency graphs. +Diff continues to just calculate the difference between trees, and reify +just makes the changes required to turn the existing actual nodes into the +ideal nodes. + +Would avoid previous global install problems by adding an extra step to +prevent any accidental inclusion of nodes that are outside of the expected +set. + +This would still require that the idealTree be built up in its entirety for +workspace installs, because some dependencies might be hoisted or shared, +and the idealTree that is _saved_ must still reflect these. + +So, how to save the idealTree in its entirety, but only Diff the filtered +set? Some kind of flag to tell Diff to ignore it? The value of this +approach is that Diff doesn't have to know about the dependency +relationships, so if we have to put duct tape on it to communicate those +relationships out of band, then that's not great. Maybe keep two +idealTree's around, one with the full set, and one that's filtered down to +just the bits we want to diff/install? + +## approach 2 - make Diff dependency-graph-aware + +This avoids having to maintain two idealTree instances, or do a subsequent +step to prune the trees before letting reify/diff operate on them, and +preserves the ability for reify() to remain dependency-agnostic. + +Instead of starting from the root and walking `children` and `fsChildren`, +the Diff algorithm would start from the root node, and walk only to the +target filter node, and then walk through the `edgesOut` to find nodes that +need to be diffed. + +### one pass + +Instead of walking the physical `children`/`fsChildren` tree, _only_ walk +the `edgesOut` and build up the diff object that way. + +The challenge here will be that we may end up several layers deep in the +physical tree, and then have to add a node that is shallower. So that +makes the diff walking algorithm a bit more complicated, because we're not +just slapping a new object into a child set, but may find ourselves +back-tracking up the tree as the graph wanders around. + +### two pass + +Make a first pass to create the set of nodes based on the edgesOut +traversal. Then do the current physical-tree based differencing walk, but +skipping any nodes that are not in the set. + +This is potentially expensive if the trees are very large (eg, a project +with thousands of workspaces), but is safe and very low complexity. + +Start with this. + +## approach 3 - make Reify dependency-graph-aware + +Given the existing complexity of reify(), making it have to look at the +dependency graph is a pretty big paradigm shift. Currently, the one thing +that makes it reasonable to reason about, is that while the algorithm is +somewhat involved, it really only has one job (turn one representation of a +tree into another). From d9e058df296e87e6b02e91f73a425e76e8b2b131 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 24 Mar 2021 12:46:05 -0700 Subject: [PATCH 02/17] Add filterNodes option to Diff.calculate --- lib/diff.js | 69 ++++++++-- tap-snapshots/test-diff.js-TAP.test.js | 177 +++++++++++++++++++++++++ test/diff.js | 125 +++++++++++++++++ 3 files changed, 361 insertions(+), 10 deletions(-) diff --git a/lib/diff.js b/lib/diff.js index ada67f816..84a8bae41 100644 --- a/lib/diff.js +++ b/lib/diff.js @@ -11,7 +11,8 @@ const {existsSync} = require('fs') const ssri = require('ssri') class Diff { - constructor ({actual, ideal}) { + constructor ({actual, ideal, filterSet}) { + this.filterSet = filterSet this.children = [] this.actual = actual this.ideal = ideal @@ -29,9 +30,54 @@ class Diff { this.removed = [] } - static calculate ({actual, ideal}) { + static calculate ({actual, ideal, filterNodes = []}) { + // if there's a filterNode, then: + // - get the path from the root to the filterNode. The root or + // root.target should have an edge either to the filterNode or + // a link to the filterNode. If not, abort. Add the path to the + // filterSet. + // - Add set of Nodes depended on by the filterNode to filterSet. + // - Anything outside of that set should be ignored by getChildren + const filterSet = new Set() + for (const filterNode of filterNodes) { + const { root } = filterNode + if (root !== ideal && root !== actual) + throw new Error('invalid filterNode: outside idealTree/actualTree') + const { target } = root + const rootTarget = target || root + const edge = [...rootTarget.edgesOut.values()].filter(e => { + return e.to && (e.to === filterNode || e.to.target === filterNode) + })[0] + filterSet.add(root) + filterSet.add(rootTarget) + filterSet.add(ideal) + filterSet.add(actual) + if (edge && edge.to) { + filterSet.add(edge.to) + if (edge.to.target) + filterSet.add(edge.to.target) + } + filterSet.add(filterNode) + + depth({ + tree: filterNode, + visit: node => filterSet.add(node), + getChildren: node => { + node = node.target || node + const loc = node.location + const idealNode = ideal.inventory.get(loc) + const ideals = !idealNode ? [] + : [...idealNode.edgesOut.values()].filter(e => e.to).map(e => e.to) + const actualNode = actual.inventory.get(loc) + const actuals = !actualNode ? [] + : [...actualNode.edgesOut.values()].filter(e => e.to).map(e => e.to) + return ideals.concat(actuals) + }, + }) + } + return depth({ - tree: new Diff({actual, ideal}), + tree: new Diff({actual, ideal, filterSet}), getChildren, leave, }) @@ -89,20 +135,20 @@ const allChildren = node => { // to create the diff tree const getChildren = diff => { const children = [] - const {unchanged, removed} = diff + const {actual, ideal, unchanged, removed, filterSet} = diff // Note: we DON'T diff fsChildren themselves, because they are either // included in the package contents, or part of some other project, and // will never appear in legacy shrinkwraps anyway. but we _do_ include the // child nodes of fsChildren, because those are nodes that we are typically // responsible for installing. - const actualKids = allChildren(diff.actual) - const idealKids = allChildren(diff.ideal) + const actualKids = allChildren(actual) + const idealKids = allChildren(ideal) const paths = new Set([...actualKids.keys(), ...idealKids.keys()]) for (const path of paths) { const actual = actualKids.get(path) const ideal = idealKids.get(path) - diffNode(actual, ideal, children, unchanged, removed) + diffNode(actual, ideal, children, unchanged, removed, filterSet) } if (diff.leaves && !children.length) @@ -111,7 +157,10 @@ const getChildren = diff => { return children } -const diffNode = (actual, ideal, children, unchanged, removed) => { +const diffNode = (actual, ideal, children, unchanged, removed, filterSet) => { + if (filterSet.size && !(filterSet.has(ideal) || filterSet.has(actual))) + return + const action = getAction({actual, ideal}) // if it's a match, then get its children @@ -119,7 +168,7 @@ const diffNode = (actual, ideal, children, unchanged, removed) => { if (action) { if (action === 'REMOVE') removed.push(actual) - children.push(new Diff({actual, ideal})) + children.push(new Diff({actual, ideal, filterSet})) } else { unchanged.push(ideal) // !*! Weird dirty hack warning !*! @@ -150,7 +199,7 @@ const diffNode = (actual, ideal, children, unchanged, removed) => { for (const node of bundledChildren) node.parent = ideal } - children.push(...getChildren({actual, ideal, unchanged, removed})) + children.push(...getChildren({actual, ideal, unchanged, removed, filterSet})) } } diff --git a/tap-snapshots/test-diff.js-TAP.test.js b/tap-snapshots/test-diff.js-TAP.test.js index 577c7e595..59f78d68d 100644 --- a/tap-snapshots/test-diff.js-TAP.test.js +++ b/tap-snapshots/test-diff.js-TAP.test.js @@ -267,6 +267,183 @@ Diff { } ` +exports[`test/diff.js TAP filtered diff > c excluded, a and b present 1`] = ` +Diff { + "action": null, + "actual": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "ideal": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/a", + "/project/path/node_modules/b", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [ + Diff { + "action": "ADD", + "actual": undefined, + "ideal": Link { + "name": "a", + "path": "/project/path/node_modules/a", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/a", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [], + }, + Diff { + "action": "ADD", + "actual": undefined, + "ideal": Node { + "name": "b", + "path": "/project/path/node_modules/b", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/b", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [], + }, + ], +} +` + +exports[`test/diff.js TAP filtered diff > d is removed 1`] = ` +Diff { + "action": null, + "actual": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "ideal": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/d", + ], + "unchanged": Array [ + "/project/path/node_modules/a", + "/project/path/node_modules/b", + ], + "removed": Array [ + "/project/path/node_modules/d", + ], + "children": Array [ + Diff { + "action": "REMOVE", + "actual": Node { + "name": "d", + "path": "/project/path/node_modules/d", + "integrity": null, + }, + "ideal": undefined, + "leaves": Array [ + "/project/path/node_modules/d", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [], + }, + ], +} +` + +exports[`test/diff.js TAP filtered diff > e is removed (extraneous) 1`] = ` +Diff { + "action": null, + "actual": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "ideal": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/e", + ], + "unchanged": Array [], + "removed": Array [ + "/project/path/node_modules/e", + ], + "children": Array [ + Diff { + "action": "REMOVE", + "actual": Node { + "name": "e", + "path": "/project/path/node_modules/e", + "integrity": null, + }, + "ideal": undefined, + "leaves": Array [ + "/project/path/node_modules/e", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [], + }, + ], +} +` + +exports[`test/diff.js TAP filtered diff > e is removed 1`] = ` +Diff { + "action": null, + "actual": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "ideal": Node { + "name": "path", + "path": "/project/path", + "integrity": null, + }, + "leaves": Array [ + "/project/path/node_modules/e", + ], + "unchanged": Array [], + "removed": Array [ + "/project/path/node_modules/e", + ], + "children": Array [ + Diff { + "action": "REMOVE", + "actual": Node { + "name": "e", + "path": "/project/path/node_modules/e", + "integrity": null, + }, + "ideal": undefined, + "leaves": Array [ + "/project/path/node_modules/e", + ], + "unchanged": Array [], + "removed": Array [], + "children": Array [], + }, + ], +} +` + exports[`test/diff.js TAP when a global root is a link, traverse the target children > correctly removes the child node 1`] = ` Diff { "action": null, diff --git a/test/diff.js b/test/diff.js index 90414d124..f497c038d 100644 --- a/test/diff.js +++ b/test/diff.js @@ -221,3 +221,128 @@ t.test('when a global root is a link, traverse the target children', async (t) = t.matchSnapshot(diff, 'correctly removes the child node') t.equal(diff.removed.length, 1, 'identifies the need to remove the child') }) + +t.test('filtered diff', async t => { + const ideal = new Node({ + path: '/project/path', + pkg: { + name: 'root', + dependencies: { a: 'file:a', c: '' }, + }, + }) + + new Link({ + parent: ideal, + pkg: {}, + realpath: '/project/path/a', + }) + const a = new Node({ + fsParent: ideal, + path: '/project/path/a', + pkg: { + name: 'a', + dependencies: { + b: '', + }, + }, + }) + new Node({ + parent: ideal, + pkg: { + name: 'b', + version: '1.2.3', + }, + }) + new Node({ + parent: ideal, + pkg: { + name: 'c', + version: '1.2.3', + }, + }) + + const actual = new Node({ + path: '/project/path', + pkg: { + name: 'root', + dependencies: { a: 'file:a', c: '' }, + }, + }) + + const cExcludedABPresent = Diff.calculate({actual, ideal, filterNodes: [a]}) + t.matchSnapshot(cExcludedABPresent, 'c excluded, a and b present') + + // make sure that *removing* something that *would* be depended on + // by the actual node in the filter set is also removed properly. + new Link({ + parent: actual, + pkg: {}, + realpath: '/project/path/a', + }) + new Node({ + fsParent: actual, + path: '/project/path/a', + pkg: { + name: 'a', + dependencies: { + b: '', + d: '', + }, + }, + }) + new Node({ + parent: actual, + pkg: { + name: 'b', + version: '1.2.3', + }, + }) + new Node({ + parent: actual, + pkg: { + name: 'd', + version: '1.2.3', + }, + }) + + const removeD = Diff.calculate({actual, ideal, filterNodes: [a]}) + t.matchSnapshot(removeD, 'd is removed') + + // removing a dependency, like we would with `npm rm -g foo` + actual.package = { + ...actual.package, + dependencies: { + ...actual.package.dependencies, + e: '*', + }, + } + const e = new Node({ + parent: actual, + pkg: { + name: 'e', + version: '1.2.3', + }, + }) + const eRemoved = Diff.calculate({actual, ideal, filterNodes: [e]}) + t.matchSnapshot(eRemoved, 'e is removed') + + // can't filter based on something that isn't there + t.throws(() => Diff.calculate({ + actual, + ideal, + filterNodes: [ + new Node({ + path: '/project/path/node_modules/x', + pkg: {name: 'x'}, + }), + ], + }), { + message: 'invalid filterNode: outside idealTree/actualTree', + }) + + // filtering an extraneous node is ok though + delete actual.package.dependencies.e + actual.package = { ...actual.package } + const eRemovedExtraneous = Diff.calculate({actual, ideal, filterNodes: [e]}) + t.matchSnapshot(eRemovedExtraneous, 'e is removed (extraneous)') +}) From c8234d4c3b3dc4f23bcd0dace55d315ea137c755 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 24 Mar 2021 12:46:59 -0700 Subject: [PATCH 03/17] bin: take -w/--workspace argument --- bin/lib/options.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bin/lib/options.js b/bin/lib/options.js index bf8e08ec2..a1b671962 100644 --- a/bin/lib/options.js +++ b/bin/lib/options.js @@ -33,7 +33,13 @@ for (const arg of process.argv.slice(2)) { options.omit.push(arg.substr('--omit='.length)) } else if (/^--before=/.test(arg)) options.before = new Date(arg.substr('--before='.length)) - else if (/^--[^=]+=/.test(arg)) { + else if (/^-w.+/.test(arg)) { + options.workspaces = options.workspaces || [] + options.workspaces.push(arg.replace(/^-w/, '')) + } else if (/^--workspace=/.test(arg)) { + options.workspaces = options.workspaces || [] + options.workspaces.push(arg.replace(/^--workspace=/, '')) + } else if (/^--[^=]+=/.test(arg)) { const [key, ...v] = arg.replace(/^--/, '').split('=') const val = v.join('=') options[key] = val === 'false' ? false : val === 'true' ? true : val From 5718c118eed4a14f4a7f8941e82957e1f174f8cb Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 24 Mar 2021 17:37:00 -0700 Subject: [PATCH 04/17] Add 'workspaces' list for reification filtering --- lib/arborist/reify.js | 93 ++++- .../test-arborist-reify.js-TAP.test.js | 318 ++++++++++++++++++ test/arborist/reify.js | 57 ++++ .../content/wrappy/-/wrappy-1.0.0.tgz | Bin 0 -> 1952 bytes 4 files changed, 459 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/registry-mocks/content/wrappy/-/wrappy-1.0.0.tgz diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index f0eae822a..c89985846 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -27,6 +27,7 @@ const retirePath = require('../retire-path.js') const promiseAllRejectLate = require('promise-all-reject-late') const optionalSet = require('../optional-set.js') const updateRootPackageJson = require('../update-root-package-json.js') +const calcDepFlags = require('../calc-dep-flags.js') const _retiredPaths = Symbol('retiredPaths') const _retiredUnchanged = Symbol('retiredUnchanged') @@ -37,6 +38,8 @@ const _retireShallowNodes = Symbol.for('retireShallowNodes') const _getBundlesByDepth = Symbol('getBundlesByDepth') const _registryResolved = Symbol('registryResolved') const _addNodeToTrashList = Symbol('addNodeToTrashList') +const _workspaces = Symbol('workspaces') + // shared by rebuild mixin const _trashList = Symbol.for('trashList') const _handleOptionalFailure = Symbol.for('handleOptionalFailure') @@ -97,8 +100,10 @@ module.exports = cls => class Reifier extends cls { packageLockOnly = false, dryRun = false, formatPackageLock = true, + workspaces = [], } = options + this[_workspaces] = workspaces this[_dryRun] = !!dryRun this[_packageLockOnly] = !!packageLockOnly this[_savePrefix] = savePrefix @@ -270,9 +275,35 @@ module.exports = cls => class Reifier extends cls { // to just invalidate the parts that changed, but avoid walking the // whole tree again. + const filterNodes = [] + if (this[_global] && this[_explicitRequests].size) { + const idealTree = this.idealTree.target || this.idealTree + const actualTree = this.actualTree.target || this.actualTree + // we ONLY are allowed to make changes in the global top-level + // children where there's an explicit request. + for (const name of this[_explicitRequests]) { + const ideal = idealTree.children.get(name) + if (ideal) + filterNodes.push(ideal) + const actual = actualTree.children.get(name) + if (actual) + filterNodes.push(actual) + } + } else { + for (const ws of this[_workspaces]) { + const ideal = this.idealTree.children.get(ws) + if (ideal) + filterNodes.push(ideal) + const actual = this.actualTree.children.get(ws) + if (actual) + filterNodes.push(actual) + } + } + // find all the nodes that need to change between the actual // and ideal trees. this.diff = Diff.calculate({ + filterNodes, actual: this.actualTree, ideal: this.idealTree, }) @@ -970,20 +1001,64 @@ module.exports = cls => class Reifier extends cls { return meta.save(saveOpt) } - [_copyIdealToActual] () { + async [_copyIdealToActual] () { + // clean up any trash that is still in the tree + for (const path of this[_trashList]) { + const loc = relpath(this.idealTree.realpath, path) + const node = this.idealTree.inventory.get(loc) + if (node && node.root === this.idealTree) + node.parent = null + } + + // if we filtered to only certain nodes, then anything ELSE needs + // to be untouched in the resulting actual tree, even if it differs + // in the idealTree. Copy over anything that was in the actual and + // was not changed, delete anything in the ideal and not actual. + // Then we move the entire idealTree over to this.actualTree, and + // save the hidden lockfile. + if (this.diff && this.diff.filterSet.size) { + const seen = new Set() + for (const [loc, ideal] of this.idealTree.inventory.entries()) { + if (seen.has(loc)) + continue + seen.add(loc) + + // if it's an ideal node from the filter set, then skip it + // because we already made whatever changes were necessary + if (this.diff.filterSet.has(ideal)) + continue + + // otherwise, if it's not in the actualTree, then it's not a thing + // that we actually added. And if it IS in the actualTree, then + // it's something that we left untouched, so we need to record + // that. + const actual = this.actualTree.inventory.get(loc) + if (!actual) + ideal.root = null + else { + // we may have already migrated over the target, but moving + // over a Link with a null target will delete it, so we have + // to make sure we don't drop that. + const { target: idealTarget } = ideal + actual.root = this.idealTree + if (idealTarget && actual.isLink) + actual.target = idealTarget + } + } + // need to calculate dep flags, since nodes may have been marked + // as extraneous or otherwise incorrect during transit. + calcDepFlags(this.idealTree) + } + // save the ideal's meta as a hidden lockfile after we actualize it this.idealTree.meta.filename = - this.path + '/node_modules/.package-lock.json' + this.idealTree.realpath + '/node_modules/.package-lock.json' this.idealTree.meta.hiddenLockfile = true + this.actualTree = this.idealTree this.idealTree = null - for (const path of this[_trashList]) { - const loc = relpath(this.path, path) - const node = this.actualTree.inventory.get(loc) - if (node && node.root === this.actualTree) - node.parent = null - } - return !this[_global] && this.actualTree.meta.save() + if (!this[_global]) + await this.actualTree.meta.save() } } diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index ed3803fca..f29e44a62 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -1619,6 +1619,324 @@ ArboristNode { } ` +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the a workspace after reifying c 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "type": "workspace", + }, + }, + "location": "node_modules/a", + "name": "a", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/a", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "resolved": "file:../packages/a", + "target": ArboristNode { + "location": "packages/a", + }, + "version": "1.2.3", + }, + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/c", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + "once" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/a", + "name": "once", + "spec": "*", + "type": "prod", + }, + }, + "edgesOut": Map { + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1", + "to": "node_modules/wrappy", + "type": "prod", + }, + }, + "location": "node_modules/once", + "name": "once", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/once", + "resolved": "https://registry.npmjs.org/once/-/once-1.4.0.tgz", + "version": "1.4.0", + }, + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/once", + "name": "wrappy", + "spec": "1", + "type": "prod", + }, + EdgeIn { + "from": "packages/a", + "name": "wrappy", + "spec": "1.0.2", + "type": "prod", + }, + EdgeIn { + "error": "INVALID", + "from": "packages/c", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz", + "version": "1.0.2", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "to": "node_modules/a", + "type": "workspace", + }, + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "edgesOut": Map { + "once" => EdgeOut { + "name": "once", + "spec": "*", + "to": "node_modules/once", + "type": "prod", + }, + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.2", + "to": "node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/a", + "name": "a", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "wrappy" => EdgeOut { + "error": "INVALID", + "name": "wrappy", + "spec": "1.0.0", + "to": "node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "reify-filtered-reification-in-workspaces", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", +} +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the c workspace from empty actual 1`] = ` +ArboristNode { + "children": Map { + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/c", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "error": "MISSING", + "name": "a", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "to": null, + "type": "workspace", + }, + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "children": Map { + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/c", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "packages/c/node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "version": "1.0.0", + }, + }, + "edgesOut": Map { + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.0", + "to": "packages/c/node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "reify-filtered-reification-in-workspaces", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", +} +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the workspaces, removing a and leaving c in place 1`] = ` +ArboristNode { + "children": Map { + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/c", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "children": Map { + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/c", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "packages/c/node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "version": "1.0.0", + }, + }, + "edgesOut": Map { + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.0", + "to": "packages/c/node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "reify-filtered-reification-in-workspaces", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", +} +` + exports[`test/arborist/reify.js TAP just the shrinkwrap cli-750-fresh > must match snapshot 1`] = ` { "name": "monorepo", diff --git a/test/arborist/reify.js b/test/arborist/reify.js index 29be07c6d..c75ddd53c 100644 --- a/test/arborist/reify.js +++ b/test/arborist/reify.js @@ -1503,3 +1503,60 @@ t.test('saving should not replace file: dep with version', async t => { t.equal(JSON.parse(pj2).dependencies.abbrev, 'file:abbrev', 'still a file: spec after a bare name install') }) + +t.test('filtered reification in workspaces', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: [ + 'packages/*', + ], + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.2.3', + dependencies: { + once: '', + wrappy: '1.0.2', + }, + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + dependencies: { + abbrev: '', + }, + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.2.3', + dependencies: { + wrappy: '1.0.0', + }, + }), + }, + }, + }) + + t.matchSnapshot(await printReified(path, { workspaces: ['c'] }), + 'reify the c workspace from empty actual') + + t.matchSnapshot(await printReified(path, { workspaces: ['a'] }), + 'reify the a workspace after reifying c') + + // now remove the a workspace + fs.writeFileSync(`${path}/package.json`, JSON.stringify({ + workspaces: [ + 'packages/b', + 'packages/c', + ], + })) + + t.matchSnapshot(await printReified(path, { workspaces: ['a', 'c'] }), + 'reify the workspaces, removing a and leaving c in place') +}) diff --git a/test/fixtures/registry-mocks/content/wrappy/-/wrappy-1.0.0.tgz b/test/fixtures/registry-mocks/content/wrappy/-/wrappy-1.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..7855ddc301be167c18bfc68133f57c212b84ab64 GIT binary patch literal 1952 zcmV;R2VeLfiwFP!000001MOICZ`(!^&S(CLsRL9}t)nPgmVsP-I7X&z0gIC7U@o~Qk^llvV@~2^Rjs5|=>K^ore!qWU9QI!s-9zJ$_@3L5Z7Y+hC(xBY zt0zAURL{T3`kw}{7kf*#_XhUvgqI|%y*ANqnUEomiQLr<-N>m^CIb;BX|Xf#qR7Xa z14=TC=a8jg6sCoar5DDvUc6g{l2EW99tDZ=@FYF~Op0YS7cuVT- z>4S|ziIv6C}SxT@MHMz@OP;%*}tGk z`cR!8@wuK8JFPr&Q5djTDmQF*kkkBZzIL#$aj6qZnX$FwF8Goq`0M|Q!`y3s^^M)F z=UfHucO1(+y|nb@^c&cC0{;(=jw<{&`bWoz|Ck1jcKH7i>vay@D^2?%mTTJU*KqBc z=a!~@{P^*cckAVNs@-~`5Qh^G?AHu|y;)|FmZo(&P_ZD!ofOQ?f^e3_0cDb$bLrhM zkeOiMLEu9uAwZAV6nGr1@K3;q%VoG9N&W^3ltggS!)1r|Va5fd3zU=z3&L3#KqeWg zkxaR&cq%7mg>`5KeoMh8@f0>sV1`y05(P^$5kt-0Dxfx;p`_w&6--smU8IJ#y+}%*3_;9<-oT6 zDQ6O5j>n7V>WeC`mX%xdNih~!6R_!bnkOX^b-XlQ)ZEav>2+H<-J+YfOFhrOO8g(% z18d}3-^9if_s4np%Zl(`fuP@2!uJ5S`e z?Zt>&nZUZ6!5!9~fHZpYEQJ|@KJqV^VEzi`h$|_Zwh35n!zqGoy6{r`gPFnSw@eGF zBOn^#xs=PwVp4lrZeb!qI1FRVDiOJj{e-S*lM4&n@!8~^=~x&axN^on+o#qk?3pgU z?}0fw1!6L|$@cPsq zohvE(@@i&XD*&0p2y&xrPKE1VI zXq#_`7MzV8%^dv#?v*vL&0!neb*#apjq>Gx$Uhh(0Q_`~x{wD>%}dPqE^Ux#@v6PM zFemO9EjoBA_j))X@6Vj^B@D+dIgFKpg$B%tNun4xbk=Rdy9*0lbI4JsuG^ z8l5;Mc`&lhhxWNO8dxMaR@{@ZgN)Z0RVB~{)3IH$IKG~cG@c6;qK=W3YgFN;7@!x* zzUAQIFHNQOY%~1&E}zu+U$yApz{V5!e`Fli_}}jtRR8x54tMzf5(~>TN7bhi%kHvJ zuL@BGtm47`RMoiDDo~L}3FI?5iLf0_u_DaNQ<-M7nMwm?G3;ropky*zdOnu-m?vh4 z?r~b_u9WGy6-0095M|lg zpG;Ie3swHuc+inZzbS;bYF3?y9dL}ufk`1nzY7EDOk-!IjI+Ib&- zK-NkdzN*dp$}&1ijaB@x50b5l1slJUT-lnfe{?@o9aj-LrD`K~6nLUiM9WV?YwUkw zL8{-dm84Ig^U_s$|E{;MIV@5wND z{v7Z){vR0K#UlC++yJT-gy!t~{l`O6n4FxzG@Up0lvf8qBER5!da)-lzu=Q4=Z#Q-B`ZG+KT z8v^a`Yq`otMm0eCiit-xpeaMPWklnv=?uVA^O>z28xxw(o$o5mPYrHrxouOMa$HlK zA>u08y#G+Y(-qiu6P9w`UPEq@)RRmuXan}4QO_jOu0eTLA_YT@;q^!TmV zrp=pCFI*{maB)y&anfdEhM%Lo#m7{a-jX#*bsOFT@ogNjos(*};X_L+a#7H1X@5XN mL8yEujrsSCr>GHC|J87`Tf4PeyR};{wEhppIkj~FBme-)K+$yo literal 0 HcmV?d00001 From dc03a68a74fa196acc581c131b00ceb03d0f5524 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 25 Mar 2021 09:54:36 -0700 Subject: [PATCH 05/17] printable: do not explode on links lacking a target --- lib/printable.js | 3 +++ tap-snapshots/test-printable.js-TAP.test.js | 23 +++++++++++++++++++++ test/printable.js | 17 +++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/lib/printable.js b/lib/printable.js index 588121dbc..b39a53cc5 100644 --- a/lib/printable.js +++ b/lib/printable.js @@ -126,6 +126,9 @@ class EdgeIn extends Edge { } const printableTree = (tree, path = []) => { + if (!tree) + return tree + const Cls = tree.isLink ? ArboristLink : tree.sourceReference ? ArboristVirtualNode : ArboristNode diff --git a/tap-snapshots/test-printable.js-TAP.test.js b/tap-snapshots/test-printable.js-TAP.test.js index 54df0f12d..b797d46eb 100644 --- a/tap-snapshots/test-printable.js-TAP.test.js +++ b/tap-snapshots/test-printable.js-TAP.test.js @@ -5,6 +5,29 @@ * Make sure to inspect the output below. Do not ignore changes! */ 'use strict' +exports[`test/printable.js TAP broken links dont break the printing > must match snapshot 1`] = ` +{ +"children":Map{ +"devnull" => ArboristLink{ +"dev":true, +"extraneous":true, +"location":"node_modules/devnull", +"name":"devnull", +"optional":true, +"path":"/home/user/projects/root/node_modules/devnull", +"peer":true, +"realpath":"/home/user/projects/root/no/thing/here", +"resolved":"file:../no/thing/here", +"target":null,},}, +"dev":true, +"extraneous":true, +"location":"", +"name":"root", +"optional":true, +"path":"/home/user/projects/root", +"peer":true,} +` + exports[`test/printable.js TAP printable Node do not recurse forever > must match snapshot 1`] = ` { name:'recursive', diff --git a/test/printable.js b/test/printable.js index de59b1ab7..016a93e59 100644 --- a/test/printable.js +++ b/test/printable.js @@ -226,3 +226,20 @@ t.test('virtual roots are shown with their sourceReference', t => { t.matchSnapshot(printable(virtual)) t.end() }) + +t.test('broken links dont break the printing', t => { + const tree = new Node({ + path: '/home/user/projects/root', + }) + + // a link with no target + const brokenLink = new Link({ + name: 'devnull', + realpath: '/home/user/projects/root/no/thing/here', + parent: tree, + }) + brokenLink.target.root = null + + t.matchSnapshot(printable(tree)) + t.end() +}) From c5d1b9fbff10af8fb13eba714c55a90fc9b49c84 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 25 Mar 2021 10:12:41 -0700 Subject: [PATCH 06/17] do not take a link's target when removing link from tree This causes major problems when moving links and targets between trees. When we move the link from one tree to another, it pulls its target, out, meaning that the target ends up becoming orphaned. Say there is a matching link and target in two different trees, A and B, and we want to replace the link and target in one tree with the link and target in another. If we move the target A to tree B first, then it replaces the target B, and link B is updated to point to it. Then, we move link A to tree B, replacing link B. To do that, we move link B out of tree B, which _takes target B with it_, leaving the ported link A without a target in tree B. --- lib/node.js | 7 +++-- .../test-arborist-reify.js-TAP.test.js | 26 +++++++++++++++++++ test/node.js | 5 ++-- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/node.js b/lib/node.js index fa39bed5e..197804e0c 100644 --- a/lib/node.js +++ b/lib/node.js @@ -685,6 +685,7 @@ class Node { ...this.children.values(), ...this.inventory.values(), ].filter(n => n !== this)) + for (const child of family) { if (child.root !== root) { child[_delistFromMeta]() @@ -704,12 +705,14 @@ class Node { } // if we had a target, and didn't find one in the new root, then bring - // it over as well. - if (this.isLink && target && !this.target) + // it over as well, but only if we're setting the link into a new root, + // as we don't want to lose the target any time we remove a link. + if (this.isLink && target && !this.target && root !== this) target.root = root // tree should always be valid upon root setter completion. treeCheck(this) + treeCheck(root) } get root () { diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index f29e44a62..ee59ba1e2 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -1899,6 +1899,32 @@ ArboristNode { }, }, "fsChildren": Set { + ArboristNode { + "dev": true, + "edgesOut": Map { + "once" => EdgeOut { + "error": "MISSING", + "name": "once", + "spec": "*", + "to": null, + "type": "prod", + }, + "wrappy" => EdgeOut { + "error": "MISSING", + "name": "wrappy", + "spec": "1.0.2", + "to": null, + "type": "prod", + }, + }, + "extraneous": true, + "location": "packages/a", + "name": "a", + "optional": true, + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "peer": true, + "version": "1.2.3", + }, ArboristNode { "children": Map { "wrappy" => ArboristNode { diff --git a/test/node.js b/test/node.js index fc19ac74c..588060413 100644 --- a/test/node.js +++ b/test/node.js @@ -591,8 +591,9 @@ t.test('load with a virtual filesystem parent', t => { t.equal(link.root, link, 'removed from parent, removed from root') t.equal(root.inventory.get(linkLoc), undefined, 'removed from root inventory') t.equal(link.inventory.has(link), true, 'link added to own inventory') - t.equal(link.target, linkTarget, 'target taken along for the ride') - t.equal(linkTarget.root, link, 'target rooted on link') + t.equal(link.target, null, 'target left behind when setting root to null') + linkTarget.root = link + t.equal(link.target, linkTarget, 'target set once roots match') t.equal(link.inventory.get(''), linkTarget) t.equal(root.edgesOut.get('link').error, 'MISSING') From a8a1e84f27094390d91005b7a67b9023ffad67df Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 25 Mar 2021 12:00:46 -0700 Subject: [PATCH 07/17] Correctly update actualTree after filtered reify This also corrects an issue where manually updating the package.json file would sometimes not be reified properly, if a new link target matches the one that was formerly reified. --- lib/arborist/build-ideal-tree.js | 12 +- lib/arborist/reify.js | 37 +- ...t-arborist-build-ideal-tree.js-TAP.test.js | 81 +++ .../test-arborist-reify.js-TAP.test.js | 496 +++++++++++++++++- test/arborist/build-ideal-tree.js | 71 +++ test/arborist/reify.js | 78 ++- 6 files changed, 743 insertions(+), 32 deletions(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index f7e5b7e32..b8f4a6468 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -1480,9 +1480,15 @@ This is a one-time fix-up, please be patient... if (target.children.has(edge.name)) { const current = target.children.get(edge.name) - // same thing = keep - if (dep.matches(current)) - return KEEP + // same thing = keep, UNLESS the current doesn't satisfy and new + // one does satisfy. This can happen if it's a link to a matching target + // at a different location, which satisfies a version dep, but not a + // file: dep. If neither of them satisfy, then we can replace it, + // because presumably it's better for a peer or something. + if (dep.matches(current)) { + if (current.satisfies(edge) || !dep.satisfies(edge)) + return KEEP + } const { version: curVer } = current const { version: newVer } = dep diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index c89985846..e563f6257 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -1017,6 +1017,7 @@ module.exports = cls => class Reifier extends cls { // Then we move the entire idealTree over to this.actualTree, and // save the hidden lockfile. if (this.diff && this.diff.filterSet.size) { + const { filterSet } = this.diff const seen = new Set() for (const [loc, ideal] of this.idealTree.inventory.entries()) { if (seen.has(loc)) @@ -1025,7 +1026,7 @@ module.exports = cls => class Reifier extends cls { // if it's an ideal node from the filter set, then skip it // because we already made whatever changes were necessary - if (this.diff.filterSet.has(ideal)) + if (filterSet.has(ideal)) continue // otherwise, if it's not in the actualTree, then it's not a thing @@ -1036,15 +1037,35 @@ module.exports = cls => class Reifier extends cls { if (!actual) ideal.root = null else { - // we may have already migrated over the target, but moving - // over a Link with a null target will delete it, so we have - // to make sure we don't drop that. - const { target: idealTarget } = ideal - actual.root = this.idealTree - if (idealTarget && actual.isLink) - actual.target = idealTarget + if ([...actual.linksIn].some(link => filterSet.has(link))) { + seen.add(actual.location) + continue + } + const { realpath, isLink } = actual + if (isLink && ideal.isLink && ideal.realpath === realpath) + continue + else + actual.root = this.idealTree } } + + // now find any actual nodes that may not be present in the ideal + // tree, but were left behind by virtue of not being in the filter + for (const [loc, actual] of this.actualTree.inventory.entries()) { + if (seen.has(loc)) + continue + seen.add(loc) + if (filterSet.has(actual)) + continue + actual.root = this.idealTree + } + + // prune out any tops that lack a linkIn + for (const top of this.idealTree.tops) { + if (top.linksIn.size === 0) + top.root = null + } + // need to calculate dep flags, since nodes may have been marked // as extraneous or otherwise incorrect during transit. calcDepFlags(this.idealTree) diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index 53c287896..c65c852ad 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -107277,6 +107277,87 @@ ArboristNode { } ` +exports[`test/arborist/build-ideal-tree.js TAP replace a link with a matching link when the current one is wrong > replace incorrect with correct 1`] = ` +ArboristNode { + "children": Map { + "x" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "x", + "spec": "file:correct/x", + "type": "prod", + }, + }, + "location": "node_modules/x", + "name": "x", + "path": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/node_modules/x", + "realpath": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/correct/x", + "resolved": "file:../correct/x", + "target": ArboristNode { + "location": "correct/x", + }, + "version": "1.2.3", + }, + "y" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "y", + "spec": "file:correct/x", + "type": "prod", + }, + }, + "location": "node_modules/y", + "name": "y", + "packageName": "x", + "path": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/node_modules/y", + "realpath": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/correct/x", + "resolved": "file:../correct/x", + "target": ArboristNode { + "location": "correct/x", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "x" => EdgeOut { + "name": "x", + "spec": "file:correct/x", + "to": "node_modules/x", + "type": "prod", + }, + "y" => EdgeOut { + "name": "y", + "spec": "file:correct/x", + "to": "node_modules/y", + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "location": "correct/x", + "name": "x", + "path": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/correct/x", + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "incorrect/x", + "name": "x", + "optional": true, + "path": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong/incorrect/x", + "peer": true, + "version": "1.2.3", + }, + }, + "location": "", + "name": "build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong", + "path": "{CWD}/test/arborist/build-ideal-tree-replace-a-link-with-a-matching-link-when-the-current-one-is-wrong", +} +` + exports[`test/arborist/build-ideal-tree.js TAP respect the yarn.lock file > expect resolving Promise 1`] = ` ArboristNode { "children": Map { diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index ee59ba1e2..8ee6f71ab 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -1619,6 +1619,186 @@ ArboristNode { } ` +exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden lockfile - c 1`] = ` +{ + "name": "reify-filtered-reification-in-workspaces", + "lockfileVersion": 2, + "requires": true, + "packages": { + "node_modules/c": { + "resolved": "packages/c", + "link": true + }, + "packages/c": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.0" + } + }, + "packages/c/node_modules/wrappy": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "integrity": "sha1-iq5PxrTNa+MqRVOYW88ys+4THk4=" + } + } +} + +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden lockfile - c, old x, removed a 1`] = ` +{ + "name": "reify-filtered-reification-in-workspaces", + "lockfileVersion": 2, + "requires": true, + "packages": { + "apps/x": { + "version": "1.2.3" + }, + "node_modules/c": { + "resolved": "packages/c", + "link": true + }, + "node_modules/x": { + "resolved": "apps/x", + "link": true + }, + "packages/c": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.0" + } + }, + "packages/c/node_modules/wrappy": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "integrity": "sha1-iq5PxrTNa+MqRVOYW88ys+4THk4=" + } + } +} + +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden lockfile - c, x 1`] = ` +{ + "name": "reify-filtered-reification-in-workspaces", + "lockfileVersion": 2, + "requires": true, + "packages": { + "apps/x": { + "version": "1.2.3" + }, + "node_modules/c": { + "resolved": "packages/c", + "link": true + }, + "node_modules/x": { + "resolved": "apps/x", + "link": true + }, + "packages/c": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.0" + } + } + } +} + +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden lockfile - c, x, a 1`] = ` +{ + "name": "reify-filtered-reification-in-workspaces", + "lockfileVersion": 2, + "requires": true, + "packages": { + "apps/x": { + "version": "1.2.3" + }, + "node_modules/a": { + "resolved": "packages/a", + "link": true + }, + "node_modules/c": { + "resolved": "packages/c", + "link": true + }, + "node_modules/once": { + "version": "1.4.0", + "resolved": "https://registry.npmjs.org/once/-/once-1.4.0.tgz", + "integrity": "sha1-WDsap3WWHUsROsF9nFC6753Xa9E=", + "dependencies": { + "wrappy": "1" + } + }, + "node_modules/wrappy": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz", + "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8=" + }, + "node_modules/x": { + "resolved": "apps/x", + "link": true + }, + "packages/a": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.2", + "once": "" + } + }, + "packages/c": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.0" + } + } + } +} + +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden lockfile - foo/x linked, c, old x, removed a 1`] = ` +{ + "name": "reify-filtered-reification-in-workspaces", + "lockfileVersion": 2, + "requires": true, + "packages": { + "apps/x": { + "version": "1.2.3" + }, + "foo/x": { + "version": "1.2.3", + "extraneous": true + }, + "node_modules/c": { + "resolved": "packages/c", + "link": true + }, + "node_modules/foox": { + "resolved": "foo/x", + "link": true + }, + "node_modules/x": { + "resolved": "apps/x", + "link": true + }, + "packages/c": { + "version": "1.2.3", + "dependencies": { + "wrappy": "1.0.0" + } + }, + "packages/c/node_modules/wrappy": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "integrity": "sha1-iq5PxrTNa+MqRVOYW88ys+4THk4=" + } + } +} + +` + exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the a workspace after reifying c 1`] = ` ArboristNode { "children": Map { @@ -1711,6 +1891,25 @@ ArboristNode { "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz", "version": "1.0.2", }, + "x" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "type": "workspace", + }, + }, + "location": "node_modules/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/x", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "resolved": "file:../apps/x", + "target": ArboristNode { + "location": "apps/x", + }, + "version": "1.2.3", + }, }, "edgesOut": Map { "a" => EdgeOut { @@ -1732,8 +1931,20 @@ ArboristNode { "to": "node_modules/c", "type": "workspace", }, + "x" => EdgeOut { + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "to": "node_modules/x", + "type": "workspace", + }, }, "fsChildren": Set { + ArboristNode { + "location": "apps/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "version": "1.2.3", + }, ArboristNode { "edgesOut": Map { "once" => EdgeOut { @@ -1776,7 +1987,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the c workspace from empty actual 1`] = ` +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the c workspace only 1`] = ` ArboristNode { "children": Map { "c" => ArboristLink { @@ -1820,6 +2031,13 @@ ArboristNode { "to": "node_modules/c", "type": "workspace", }, + "x" => EdgeOut { + "error": "MISSING", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "to": null, + "type": "workspace", + }, }, "fsChildren": Set { ArboristNode { @@ -1860,7 +2078,7 @@ ArboristNode { } ` -exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the workspaces, removing a and leaving c in place 1`] = ` +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the workspaces, foo/x linked, c, old x, removed a 1`] = ` ArboristNode { "children": Map { "c" => ArboristLink { @@ -1882,6 +2100,42 @@ ArboristNode { }, "version": "1.2.3", }, + "foox" => ArboristLink { + "dev": true, + "extraneous": true, + "location": "node_modules/foox", + "name": "foox", + "optional": true, + "packageName": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/foox", + "peer": true, + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "resolved": "file:../foo/x", + "target": ArboristNode { + "location": "foo/x", + }, + "version": "1.2.3", + }, + "x" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "error": "INVALID", + "from": "", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "type": "workspace", + }, + }, + "location": "node_modules/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/x", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "resolved": "file:../apps/x", + "target": ArboristNode { + "location": "apps/x", + }, + "version": "1.2.3", + }, }, "edgesOut": Map { "b" => EdgeOut { @@ -1897,32 +2151,139 @@ ArboristNode { "to": "node_modules/c", "type": "workspace", }, + "x" => EdgeOut { + "error": "INVALID", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "to": "node_modules/x", + "type": "workspace", + }, }, "fsChildren": Set { + ArboristNode { + "location": "apps/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "version": "1.2.3", + }, ArboristNode { "dev": true, - "edgesOut": Map { - "once" => EdgeOut { - "error": "MISSING", - "name": "once", - "spec": "*", - "to": null, - "type": "prod", + "extraneous": true, + "location": "foo/x", + "name": "x", + "optional": true, + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "children": Map { + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/c", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "packages/c/node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "version": "1.0.0", }, + }, + "edgesOut": Map { "wrappy" => EdgeOut { - "error": "MISSING", "name": "wrappy", - "spec": "1.0.2", - "to": null, + "spec": "1.0.0", + "to": "packages/c/node_modules/wrappy", "type": "prod", }, }, - "extraneous": true, - "location": "packages/a", - "name": "a", - "optional": true, - "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", - "peer": true, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "reify-filtered-reification-in-workspaces", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", +} +` + +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the workspaces, removing a and leaving c and old x in place 1`] = ` +ArboristNode { + "children": Map { + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/c", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + "x" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "error": "INVALID", + "from": "", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "type": "workspace", + }, + }, + "location": "node_modules/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/x", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "resolved": "file:../apps/x", + "target": ArboristNode { + "location": "apps/x", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + "x" => EdgeOut { + "error": "INVALID", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/foo/x", + "to": "node_modules/x", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "location": "apps/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", "version": "1.2.3", }, ArboristNode { @@ -1963,6 +2324,105 @@ ArboristNode { } ` +exports[`test/arborist/reify.js TAP filtered reification in workspaces > reify the x workspace after reifying c 1`] = ` +ArboristNode { + "children": Map { + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/c", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + "x" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "type": "workspace", + }, + }, + "location": "node_modules/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/node_modules/x", + "realpath": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "resolved": "file:../apps/x", + "target": ArboristNode { + "location": "apps/x", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "error": "MISSING", + "name": "a", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/a", + "to": null, + "type": "workspace", + }, + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + "x" => EdgeOut { + "name": "x", + "spec": "file:{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "to": "node_modules/x", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "location": "apps/x", + "name": "x", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/apps/x", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "wrappy" => EdgeOut { + "error": "MISSING", + "name": "wrappy", + "spec": "1.0.0", + "to": null, + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "reify-filtered-reification-in-workspaces", + "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", +} +` + exports[`test/arborist/reify.js TAP just the shrinkwrap cli-750-fresh > must match snapshot 1`] = ` { "name": "monorepo", diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index 208fae135..4ac0eef5c 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -2642,3 +2642,74 @@ t.test('allow a link dep to satisfy a peer dep', async t => { // avoids if the link dep is unmet t.matchSnapshot(await printIdeal(path + '/main', { add }), 'reified link avoids conflict') }) + +t.test('replace a link with a matching link when the current one is wrong', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + dependencies: { + // testing what happens when a user hand-edits the + // package.json to point to a different target with + // a matching package. + x: 'file:correct/x', + y: 'file:correct/x', + }, + }), + correct: { + x: { + 'package.json': JSON.stringify({ + name: 'x', + version: '1.2.3', + }), + }, + }, + incorrect: { + x: { + 'package.json': JSON.stringify({ + name: 'x', + version: '1.2.3', + }), + }, + }, + node_modules: { + x: t.fixture('symlink', '../incorrect/x'), + y: t.fixture('symlink', '../correct/x'), + }, + 'package-lock.json': JSON.stringify({ + lockfileVersion: 2, + requires: true, + packages: { + '': { + dependencies: { + x: 'file:incorrect/x', + y: 'file:correct/x', + }, + }, + 'incorrect/x': { + version: '1.2.3', + name: 'x', + }, + 'node_modules/y': { + resolved: 'correct/x', + link: true, + }, + 'correct/x': { + version: '1.2.3', + name: 'x', + }, + 'node_modules/x': { + resolved: 'incorrect/x', + link: true, + }, + }, + dependencies: { + y: { + version: 'file:correct/x', + }, + x: { + version: 'file:incorrect/x', + }, + }, + }), + }) + t.matchSnapshot(await printIdeal(path), 'replace incorrect with correct') +}) diff --git a/test/arborist/reify.js b/test/arborist/reify.js index c75ddd53c..1a8fa52b9 100644 --- a/test/arborist/reify.js +++ b/test/arborist/reify.js @@ -1508,9 +1508,31 @@ t.test('filtered reification in workspaces', async t => { const path = t.testdir({ 'package.json': JSON.stringify({ workspaces: [ + 'apps/*', 'packages/*', ], }), + // 'apps' comes ahead of 'node_modules' alphabetically, + // included in the test so that we ensure that the copy + // over works properly in both directions. + apps: { + x: { + 'package.json': JSON.stringify({ + name: 'x', + version: '1.2.3', + }), + }, + }, + // this is going to be a workspace that we switch to in the ideal + // tree, but the actual tree will still be lagging behind. + foo: { + x: { + 'package.json': JSON.stringify({ + name: 'x', + version: '1.2.3', + }), + }, + }, packages: { a: { 'package.json': JSON.stringify({ @@ -1543,20 +1565,70 @@ t.test('filtered reification in workspaces', async t => { }, }) + const hiddenLock = resolve(path, 'node_modules/.package-lock.json') + t.matchSnapshot(await printReified(path, { workspaces: ['c'] }), - 'reify the c workspace from empty actual') + 'reify the c workspace only') + + t.matchSnapshot(fs.readFileSync(hiddenLock, 'utf8'), + 'hidden lockfile - c') + + t.matchSnapshot(await printReified(path, { workspaces: ['x'] }), + 'reify the x workspace after reifying c') + + t.matchSnapshot(fs.readFileSync(hiddenLock, 'utf8'), + 'hidden lockfile - c, x') t.matchSnapshot(await printReified(path, { workspaces: ['a'] }), 'reify the a workspace after reifying c') - // now remove the a workspace + t.matchSnapshot(fs.readFileSync(hiddenLock, 'utf8'), + 'hidden lockfile - c, x, a') + + // now remove the a workspace, and move x to a new target location, + // but we will not reify the apps->foo change. fs.writeFileSync(`${path}/package.json`, JSON.stringify({ workspaces: [ + 'foo/*', 'packages/b', 'packages/c', ], })) t.matchSnapshot(await printReified(path, { workspaces: ['a', 'c'] }), - 'reify the workspaces, removing a and leaving c in place') + 'reify the workspaces, removing a and leaving c and old x in place') + + t.matchSnapshot(fs.readFileSync(hiddenLock, 'utf8'), + 'hidden lockfile - c, old x, removed a') + + // Same thing, BUT, we now have a reason to already have the foo/x + // in fsChildren. fully reify with this package.json, then change + // the root package.json back to the test above. This exercises an + // edge case where the actual and ideal trees are somewhat out of sync, + // by virtue of the actualTree being generated with a package.json + // which has changed, but where only PART of the idealTree is reified + // over it. + fs.writeFileSync(`${path}/package.json`, JSON.stringify({ + dependencies: { + foox: 'file:foo/x', + }, + workspaces: [ + 'apps/x', + 'packages/a', + 'packages/c', + ], + })) + await reify(path) + fs.writeFileSync(`${path}/package.json`, JSON.stringify({ + workspaces: [ + 'foo/*', + 'packages/b', + 'packages/c', + ], + })) + t.matchSnapshot(await printReified(path, { workspaces: ['a', 'c'] }), + 'reify the workspaces, foo/x linked, c, old x, removed a') + + t.matchSnapshot(fs.readFileSync(hiddenLock, 'utf8'), + 'hidden lockfile - foo/x linked, c, old x, removed a') }) From b1240fa8e85dd93460b3de62b287d441d1218b6c Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 26 Mar 2021 11:50:56 -0700 Subject: [PATCH 08/17] json-stringify-nice@1.1.2 Fixes sorting incorrectly when a dependency is set to an empty string --- package-lock.json | 14 +++++++------- package.json | 2 +- tap-snapshots/test-arborist-reify.js-TAP.test.js | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index 15b2a80f1..c5fe8cef8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -20,7 +20,7 @@ "cacache": "^15.0.3", "common-ancestor-path": "^1.0.1", "json-parse-even-better-errors": "^2.3.1", - "json-stringify-nice": "^1.1.1", + "json-stringify-nice": "^1.1.2", "mkdirp-infer-owner": "^2.0.0", "npm-install-checks": "^4.0.0", "npm-package-arg": "^8.1.0", @@ -2919,9 +2919,9 @@ "dev": true }, "node_modules/json-stringify-nice": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/json-stringify-nice/-/json-stringify-nice-1.1.1.tgz", - "integrity": "sha512-aHOgcSoOLvmFZQMvZ27rFw68r4e9OlQtH7YEcF2u5amVYbF/D3cKBXKCvl5EGhQz2NwJZ6RPfgRX6yNQ+UBKJw==", + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/json-stringify-nice/-/json-stringify-nice-1.1.2.tgz", + "integrity": "sha512-mc0EsmCq4Ru6jTdKtKvzKzGJPa7eUHXe5/WAprXwyYYR1iY2qTcvaw3QBkPKGfJYvRr5vXoaIvMtttM+/f1xOA==", "funding": { "url": "https://github.com/sponsors/isaacs" } @@ -9893,9 +9893,9 @@ "dev": true }, "json-stringify-nice": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/json-stringify-nice/-/json-stringify-nice-1.1.1.tgz", - "integrity": "sha512-aHOgcSoOLvmFZQMvZ27rFw68r4e9OlQtH7YEcF2u5amVYbF/D3cKBXKCvl5EGhQz2NwJZ6RPfgRX6yNQ+UBKJw==" + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/json-stringify-nice/-/json-stringify-nice-1.1.2.tgz", + "integrity": "sha512-mc0EsmCq4Ru6jTdKtKvzKzGJPa7eUHXe5/WAprXwyYYR1iY2qTcvaw3QBkPKGfJYvRr5vXoaIvMtttM+/f1xOA==" }, "json-stringify-safe": { "version": "5.0.1", diff --git a/package.json b/package.json index e745be2c7..fb835d4a6 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "cacache": "^15.0.3", "common-ancestor-path": "^1.0.1", "json-parse-even-better-errors": "^2.3.1", - "json-stringify-nice": "^1.1.1", + "json-stringify-nice": "^1.1.2", "mkdirp-infer-owner": "^2.0.0", "npm-install-checks": "^4.0.0", "npm-package-arg": "^8.1.0", diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index 8ee6f71ab..7ff27319f 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -1743,8 +1743,8 @@ exports[`test/arborist/reify.js TAP filtered reification in workspaces > hidden "packages/a": { "version": "1.2.3", "dependencies": { - "wrappy": "1.0.2", - "once": "" + "once": "", + "wrappy": "1.0.2" } }, "packages/c": { From de7b6b8ac5675ac91284da46bd3197381f308c19 Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 29 Mar 2021 13:19:59 -0700 Subject: [PATCH 09/17] Make explicitRequests a set of Edges, rather than strings This is in preparation for the use case where explicitRequests includes edges from nodes other than the project root, so that we can add deps to workspaces. --- lib/arborist/build-ideal-tree.js | 54 ++++++++----- lib/arborist/reify.js | 33 +++++--- ...t-arborist-build-ideal-tree.js-TAP.test.js | 2 +- test/arborist/build-ideal-tree.js | 75 ++++++++++++++++--- 4 files changed, 126 insertions(+), 38 deletions(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index b8f4a6468..1f73f15fa 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -44,6 +44,7 @@ const _currentDep = Symbol('currentDep') const _updateAll = Symbol('updateAll') const _mutateTree = Symbol('mutateTree') const _flagsSuspect = Symbol.for('flagsSuspect') +const _workspaces = Symbol.for('workspaces') const _prune = Symbol('prune') const _preferDedupe = Symbol('preferDedupe') const _legacyBundling = Symbol('legacyBundling') @@ -109,7 +110,7 @@ const _peerSetSource = Symbol.for('peerSetSource') // used by Reify mixin const _force = Symbol.for('force') -const _explicitRequests = Symbol.for('explicitRequests') +const _explicitRequests = Symbol('explicitRequests') const _global = Symbol.for('global') const _idealTreePrune = Symbol.for('idealTreePrune') @@ -130,8 +131,10 @@ module.exports = cls => class IdealTreeBuilder extends cls { force = false, packageLock = true, strictPeerDeps = false, + workspaces = [], } = options + this[_workspaces] = workspaces || [] this[_force] = !!force this[_strictPeerDeps] = !!strictPeerDeps @@ -143,6 +146,9 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_globalStyle] = this[_global] || globalStyle this[_follow] = !!follow + if (this[_workspaces].length && this[_global]) + throw new Error('Cannot operate on workspaces in global mode') + this[_explicitRequests] = new Set() this[_preferDedupe] = false this[_legacyBundling] = false @@ -157,6 +163,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_manifests] = new Map() this[_peerConflict] = null this[_edgesOverridden] = new Set() + this[_resolvedAdd] = [] // a map of each module in a peer set to the thing that depended on // that set of peers in the first place. Use a WeakMap so that we @@ -266,6 +273,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_preferDedupe] = !!options.preferDedupe this[_legacyBundling] = !!options.legacyBundling this[_updateNames] = update.names + this[_updateAll] = update.all // we prune by default unless explicitly set to boolean false this[_prune] = options.prune !== false @@ -387,6 +395,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { async [_applyUserRequests] (options) { process.emit('time', 'idealTree:userRequests') const tree = this.idealTree.target || this.idealTree + // If we have a list of package names to update, and we know it's // going to update them wherever they are, add any paths into those // named nodes to the buildIdealTree queue. @@ -395,32 +404,45 @@ module.exports = cls => class IdealTreeBuilder extends cls { // global updates only update the globalTop nodes, but we need to know // that they're there, and not reinstall the world unnecessarily. + const globalExplicitUpdateNames = [] if (this[_global] && (this[_updateAll] || this[_updateNames].length)) { const nm = resolve(this.path, 'node_modules') for (const name of await readdir(nm).catch(() => [])) { - if (this[_updateNames].includes(name)) - this[_explicitRequests].add(name) tree.package.dependencies = tree.package.dependencies || {} - if (this[_updateAll] || this[_updateNames].includes(name)) + const updateName = this[_updateNames].includes(name) + if (this[_updateAll] || updateName) { + if (updateName) + globalExplicitUpdateNames.push(name) tree.package.dependencies[name] = '*' + } } } if (this.auditReport && this.auditReport.size > 0) this[_queueVulnDependents](options) - if (options.rm && options.rm.length) { - addRmPkgDeps.rm(tree.package, options.rm) - for (const name of options.rm) - this[_explicitRequests].add(name) + const { add, rm } = options + + if (rm && rm.length) { + addRmPkgDeps.rm(tree.package, rm) + for (const name of rm) + this[_explicitRequests].add({ from: tree, name, action: 'DELETE' }) } - if (options.add) + if (add && add.length) await this[_add](options) - // triggers a refresh of all edgesOut - if (options.add && options.add.length || options.rm && options.rm.length || this[_global]) + // triggers a refresh of all edgesOut. this has to be done BEFORE + // adding the edges to explicitRequests, because the package setter + // resets all edgesOut. + if (add && add.length || rm && rm.length || this[_global]) tree.package = tree.package + + for (const spec of this[_resolvedAdd]) + this[_explicitRequests].add(tree.edgesOut.get(spec.name)) + for (const name of globalExplicitUpdateNames) + this[_explicitRequests].add(tree.edgesOut.get(name)) + process.emit('timeEnd', 'idealTree:userRequests') } @@ -438,7 +460,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { .then(add => this[_updateFilePath](add)) .then(add => this[_followSymlinkPath](add)) })).then(add => { - this[_resolvedAdd] = add + this[_resolvedAdd].push(...add) // now add is a list of spec objects with names. // find a home for each of them! const tree = this.idealTree.target || this.idealTree @@ -449,8 +471,6 @@ module.exports = cls => class IdealTreeBuilder extends cls { saveType, path: this.path, }) - for (const spec of add) - this[_explicitRequests].add(spec.name) }) } @@ -991,7 +1011,7 @@ This is a one-time fix-up, please be patient... // if it's peerOptional and not explicitly requested. if (!edge.to) { return edge.type !== 'peerOptional' || - this[_explicitRequests].has(edge.name) + this[_explicitRequests].has(edge) } // If the edge has an error, there's a problem. @@ -1007,7 +1027,7 @@ This is a one-time fix-up, please be patient... return true // If the user has explicitly asked to install this package, it's a problem. - if (node.isProjectRoot && this[_explicitRequests].has(edge.name)) + if (node.isProjectRoot && this[_explicitRequests].has(edge)) return true // No problems! @@ -1190,7 +1210,7 @@ This is a one-time fix-up, please be patient... [_placeDep] (dep, node, edge, peerEntryEdge = null, peerPath = []) { if (edge.to && !edge.error && - !this[_explicitRequests].has(edge.name) && + !this[_explicitRequests].has(edge) && !this[_updateNames].includes(edge.name) && !this[_isVulnerable](edge.to)) return [] diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index e563f6257..aaaa3d61c 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -38,7 +38,7 @@ const _retireShallowNodes = Symbol.for('retireShallowNodes') const _getBundlesByDepth = Symbol('getBundlesByDepth') const _registryResolved = Symbol('registryResolved') const _addNodeToTrashList = Symbol('addNodeToTrashList') -const _workspaces = Symbol('workspaces') +const _workspaces = Symbol.for('workspaces') // shared by rebuild mixin const _trashList = Symbol.for('trashList') @@ -86,7 +86,6 @@ const _global = Symbol.for('global') // defined by Ideal mixin const _pruneBundledMetadeps = Symbol.for('pruneBundledMetadeps') -const _explicitRequests = Symbol.for('explicitRequests') const _resolvedAdd = Symbol.for('resolvedAdd') const _usePackageLock = Symbol.for('usePackageLock') const _formatPackageLock = Symbol.for('formatPackageLock') @@ -100,10 +99,8 @@ module.exports = cls => class Reifier extends cls { packageLockOnly = false, dryRun = false, formatPackageLock = true, - workspaces = [], } = options - this[_workspaces] = workspaces this[_dryRun] = !!dryRun this[_packageLockOnly] = !!packageLockOnly this[_savePrefix] = savePrefix @@ -246,9 +243,25 @@ module.exports = cls => class Reifier extends cls { const actualOpt = this[_global] ? { ignoreMissing: true, global: true, - filter: (node, kid) => - this[_explicitRequests].size === 0 || !node.isProjectRoot ? true - : (this.idealTree.edgesOut.has(kid) || this[_explicitRequests].has(kid)), + filter: (node, kid) => { + // if it's not the project root, and we have no explicit requests, + // then we're already into a nested dep, so we keep it + if (this.explicitRequests.size === 0 || !node.isProjectRoot) + return true + + // if we added it as an edgeOut, then we want it + if (this.idealTree.edgesOut.has(kid)) + return true + + // if it's an explicit request, then we want it + const hasExplicit = [...this.explicitRequests] + .some(edge => edge.name === kid) + if (hasExplicit) + return true + + // ignore the rest of the global install folder + return false + }, } : { ignoreMissing: true } if (!this[_global]) { @@ -276,12 +289,12 @@ module.exports = cls => class Reifier extends cls { // whole tree again. const filterNodes = [] - if (this[_global] && this[_explicitRequests].size) { + if (this[_global] && this.explicitRequests.size) { const idealTree = this.idealTree.target || this.idealTree const actualTree = this.actualTree.target || this.actualTree // we ONLY are allowed to make changes in the global top-level // children where there's an explicit request. - for (const name of this[_explicitRequests]) { + for (const { name } of this.explicitRequests) { const ideal = idealTree.children.get(name) if (ideal) filterNodes.push(ideal) @@ -921,7 +934,7 @@ module.exports = cls => class Reifier extends cls { // to things like git repos and tarball file/urls. However, if the // user requested 'foo@', and we have a foo@file:../foo, then we should // end up saving the spec we actually used, not whatever they gave us. - if (this[_resolvedAdd]) { + if (this[_resolvedAdd].length) { const root = this.idealTree const pkg = root.package for (const { name } of this[_resolvedAdd]) { diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index c65c852ad..b0916af55 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -173,7 +173,7 @@ ArboristNode { } ` -exports[`test/arborist/build-ideal-tree.js TAP a workspace with a conflicted nested duplicated dep > expect resolving Promise 1`] = ` +exports[`test/arborist/build-ideal-tree.js TAP a workspace with a conflicted nested duplicated dep > must match snapshot 1`] = ` ArboristNode { "children": Map { "bar" => ArboristLink { diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index 4ac0eef5c..a5c919a9b 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -114,9 +114,9 @@ t.test('no options', t => { t.end() }) -t.test('a workspace with a conflicted nested duplicated dep', t => - t.resolveMatchSnapshot(printIdeal( - resolve(fixtures, 'workspace4')))) +t.test('a workspace with a conflicted nested duplicated dep', async t => { + t.matchSnapshot(await printIdeal(resolve(fixtures, 'workspace4'))) +}) t.test('a tree with an outdated dep, missing dep, no lockfile', async t => { const path = resolve(fixtures, 'outdated-no-lockfile') @@ -293,9 +293,8 @@ t.test('expose explicitRequest', async t => { const path = resolve(fixtures, 'simple') const arb = new Arborist({...OPT, path}) await arb.buildIdealTree({ add: ['abbrev'] }) - t.ok(arb.explicitRequests, 'exposes the explicit request') - t.strictSame(arb.explicitRequests, new Set(['abbrev'])) - t.ok(arb.explicitRequests.has('abbrev'), 'should contain explicit item') + t.match(arb.explicitRequests, Set, 'exposes the explicit request Set') + t.strictSame([...arb.explicitRequests].map(e => e.name), ['abbrev']) t.end() }) @@ -2159,9 +2158,11 @@ t.test('carbonium eslint conflicts', async t => { t.test('peerOptionals that are devDeps or explicit request', async t => { const path = resolve(fixtures, 'peer-optional-installs') - t.matchSnapshot(await printIdeal(path, { - add: ['abbrev'], - }), 'should install the abbrev dep') + const arb = new Arborist({ path, ...OPT }) + const tree = await arb.buildIdealTree({ add: ['abbrev'] }) + t.matchSnapshot(printTree(tree), 'should install the abbrev dep') + t.ok(tree.children.get('abbrev'), 'should install abbrev dep') + t.matchSnapshot(await printIdeal(path, { add: ['wrappy'], saveType: 'dev', @@ -2711,5 +2712,59 @@ t.test('replace a link with a matching link when the current one is wrong', asyn }, }), }) - t.matchSnapshot(await printIdeal(path), 'replace incorrect with correct') + t.matchSnapshot(await printIdeal(path, { + workspaces: null, // also test that a null workspaces is ignored. + }), 'replace incorrect with correct') +}) + +t.test('cannot do workspaces in global mode', t => { + t.throws(() => printIdeal(t.testdir(), { + workspaces: ['a', 'b', 'c'], + global: true, + }), { message: 'Cannot operate on workspaces in global mode' }) + t.end() +}) + +t.todo('add packages to workspaces, not root', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + dependencies: { + wrappy: '1.0.0', + }, + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.2.3', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.2.3', + }), + }, + }, + }) + + const tree = await buildIdeal(path, { + add: ['wrappy@1.0.1'], + workspaces: ['a', 'c'], + }) + t.match(tree.edgesOut.get('wrappy'), { spec: '1.0.0' }) + const a = tree.children.get('a').target + const b = tree.children.get('b').target + const c = tree.children.get('c').target + t.match(a.edgesOut.get('wrappy'), { spec: '1.0.1' }) + t.equal(b.edgesOut.get('wrappy'), undefined) + t.match(c.edgesOut.get('wrappy'), { spec: '1.0.1' }) + t.matchSnapshot(printTree(tree)) }) From 51237be4c91031cb7ccd28c6f73a1180cc7a53a9 Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 29 Mar 2021 16:40:33 -0700 Subject: [PATCH 10/17] loadActual: load workspace targets when links missing --- lib/arborist/load-actual.js | 27 ++- ...t-arborist-build-ideal-tree.js-TAP.test.js | 28 ++- .../test-arborist-load-actual.js-TAP.test.js | 174 ++++++++++++++++++ test/arborist/load-actual.js | 79 +++++++- 4 files changed, 303 insertions(+), 5 deletions(-) diff --git a/lib/arborist/load-actual.js b/lib/arborist/load-actual.js index 49e76e265..eeed570d5 100644 --- a/lib/arborist/load-actual.js +++ b/lib/arborist/load-actual.js @@ -32,6 +32,7 @@ const _loadActual = Symbol('loadActual') const _loadActualVirtually = Symbol('loadActualVirtually') const _loadActualActually = Symbol('loadActualActually') const _loadWorkspaces = Symbol.for('loadWorkspaces') +const _loadWorkspaceTargets = Symbol('loadWorkspaceTargets') const _actualTreePromise = Symbol('actualTreePromise') const _actualTree = Symbol('actualTree') const _transplant = Symbol('transplant') @@ -156,12 +157,13 @@ module.exports = cls => class ActualLoader extends cls { async [_loadActualActually] ({ root, ignoreMissing, global }) { await this[_loadFSTree](this[_actualTree]) + await this[_loadWorkspaces](this[_actualTree]) + await this[_loadWorkspaceTargets](this[_actualTree]) if (!ignoreMissing) await this[_findMissingEdges]() this[_findFSParents]() this[_transplant](root) - await this[_loadWorkspaces](this[_actualTree]) if (global) { // need to depend on the children, or else all of them // will end up being flagged as extraneous, since the @@ -178,16 +180,37 @@ module.exports = cls => class ActualLoader extends cls { return this[_actualTree] } + // if there are workspace targets without Link nodes created, load + // the targets, so that we know what they are. + async [_loadWorkspaceTargets] (tree) { + if (!tree.workspaces || !tree.workspaces.size) + return + + const promises = [] + for (const path of tree.workspaces.values()) { + if (!this[_cache].has(path)) { + const p = this[_loadFSNode]({ path, root: this[_actualTree] }) + .then(node => this[_loadFSTree](node)) + promises.push(p) + } + } + await Promise.all(promises) + } + [_transplant] (root) { if (!root || root === this[_actualTree]) return + this[_actualTree][_changePath](root.path) for (const node of this[_actualTree].children.values()) { if (!this[_transplantFilter](node)) - node.parent = null + node.root = null } root.replace(this[_actualTree]) + for (const node of this[_actualTree].fsChildren) + node.root = this[_transplantFilter](node) ? root : null + this[_actualTree] = root } diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index b0916af55..36f743a68 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -153246,6 +153246,28 @@ ArboristNode { }, "fsChildren": Set { ArboristNode { + "children": Map { + "c" => ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/a/node_modules/c", + "name": "c", + "optional": true, + "path": "{CWD}/test/fixtures/workspaces-ignore-nm/packages/a/node_modules/c", + "peer": true, + "version": "1.0.0", + }, + "nested-workspace" => ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/a/node_modules/nested-workspace", + "name": "nested-workspace", + "optional": true, + "path": "{CWD}/test/fixtures/workspaces-ignore-nm/packages/a/node_modules/nested-workspace", + "peer": true, + "version": "1.0.0", + }, + }, "location": "packages/a", "name": "a", "path": "{CWD}/test/fixtures/workspaces-ignore-nm/packages/a", @@ -153423,13 +153445,15 @@ ArboristNode { }, }, "location": "packages/a", - "name": "@ruyadorno/scoped-a", + "name": "a", + "packageName": "@ruyadorno/scoped-a", "path": "{CWD}/test/fixtures/workspaces-scoped-pkg/packages/a", "version": "1.0.0", }, ArboristNode { "location": "packages/b", - "name": "@ruyadorno/scoped-b", + "name": "b", + "packageName": "@ruyadorno/scoped-b", "path": "{CWD}/test/fixtures/workspaces-scoped-pkg/packages/b", "version": "1.0.0", }, diff --git a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js index d11d521f5..fa4f726ee 100644 --- a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js +++ b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js @@ -3764,6 +3764,76 @@ ArboristNode { } ` +exports[`test/arborist/load-actual.js TAP load workspace targets, even if links not present > must match snapshot 1`] = ` +ArboristNode { + "edgesOut": Map { + "a" => EdgeOut { + "error": "MISSING", + "name": "a", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspace-targets-even-if-links-not-present/packages/a", + "to": null, + "type": "workspace", + }, + "b" => EdgeOut { + "error": "MISSING", + "name": "b", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspace-targets-even-if-links-not-present/packages/b", + "to": null, + "type": "workspace", + }, + "c" => EdgeOut { + "error": "MISSING", + "name": "c", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspace-targets-even-if-links-not-present/packages/c", + "to": null, + "type": "workspace", + }, + "wrappy" => EdgeOut { + "error": "MISSING", + "name": "wrappy", + "spec": "1.0.0", + "to": null, + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/a", + "name": "a", + "optional": true, + "path": "load-actual-load-workspace-targets-even-if-links-not-present/packages/a", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/b", + "name": "b", + "optional": true, + "path": "load-actual-load-workspace-targets-even-if-links-not-present/packages/b", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/c", + "name": "c", + "optional": true, + "path": "load-actual-load-workspace-targets-even-if-links-not-present/packages/c", + "peer": true, + "version": "1.2.3", + }, + }, + "location": "", + "name": "load-actual-load-workspace-targets-even-if-links-not-present", + "path": "load-actual-load-workspace-targets-even-if-links-not-present", +} +` + exports[`test/arborist/load-actual.js TAP look for missing deps by default external-dep/root > "dep" should have missing deps, "link" should not 1`] = ` ArboristNode { "edgesOut": Map { @@ -6441,6 +6511,110 @@ ArboristNode { } ` +exports[`test/arborist/load-actual.js TAP transplant workspace targets, even if links not present > do not transplant node named "a" 1`] = ` +ArboristNode { + "dev": true, + "edgesOut": Map { + "wrappy" => EdgeOut { + "error": "MISSING", + "name": "wrappy", + "spec": "1.0.0", + "to": null, + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/a", + "name": "a", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/a", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/b", + "name": "b", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/b", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/c", + "name": "c", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/c", + "peer": true, + "version": "1.2.3", + }, + }, + "location": "", + "name": "load-actual-transplant-workspace-targets-even-if-links-not-present", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present", + "peer": true, +} +` + +exports[`test/arborist/load-actual.js TAP transplant workspace targets, even if links not present > transplant everything 1`] = ` +ArboristNode { + "dev": true, + "edgesOut": Map { + "wrappy" => EdgeOut { + "error": "MISSING", + "name": "wrappy", + "spec": "1.0.0", + "to": null, + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/a", + "name": "a", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/a", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/b", + "name": "b", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/b", + "peer": true, + "version": "1.2.3", + }, + ArboristNode { + "dev": true, + "extraneous": true, + "location": "packages/c", + "name": "c", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present/packages/c", + "peer": true, + "version": "1.2.3", + }, + }, + "location": "", + "name": "load-actual-transplant-workspace-targets-even-if-links-not-present", + "optional": true, + "path": "load-actual-transplant-workspace-targets-even-if-links-not-present", + "peer": true, +} +` + exports[`test/arborist/load-actual.js TAP workspace > loaded tree 1`] = ` ArboristNode { "children": Map { diff --git a/test/arborist/load-actual.js b/test/arborist/load-actual.js index 575d27ec2..50ee463a5 100644 --- a/test/arborist/load-actual.js +++ b/test/arborist/load-actual.js @@ -40,7 +40,7 @@ t.cleanSnapshot = s => s.split(cwd).join('{CWD}') t.formatSnapshot = tree => format(defixture(printTree(tree)), { sort: true }) -const loadActual = (path, opts) => new Arborist({path}).loadActual(opts) +const loadActual = (path, opts) => new Arborist({path, ...opts}).loadActual(opts) roots.forEach(path => { const dir = resolve(fixtures, path) @@ -218,3 +218,80 @@ t.test('workspaces', t => { t.end() }) + +t.test('load workspace targets, even if links not present', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + dependencies: { + wrappy: '1.0.0', + }, + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.2.3', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.2.3', + }), + }, + }, + }) + t.matchSnapshot(await loadActual(path)) +}) + +t.test('transplant workspace targets, even if links not present', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + dependencies: { + wrappy: '1.0.0', + }, + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.2.3', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.2.3', + }), + }, + }, + }) + const root = new Node({ + path, + pkg: { + workspaces: ['packages/*'], + dependencies: { + wrappy: '1.0.0', + }, + }, + }) + t.matchSnapshot(await loadActual(path, { root }), 'transplant everything') + t.matchSnapshot(await loadActual(path, { + root, + transplantFilter: node => node.name !== 'a', + }), 'do not transplant node named "a"') +}) From 1902aad6a2456b713423a32e933f5d3c9a876ff3 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 30 Mar 2021 11:19:39 -0700 Subject: [PATCH 11/17] loadActual: load workspaces when using hidden lock --- lib/arborist/load-actual.js | 3 + .../test-arborist-load-actual.js-TAP.test.js | 76 +++++++++++++++++++ test/arborist/load-actual.js | 63 +++++++++++++++ 3 files changed, 142 insertions(+) diff --git a/lib/arborist/load-actual.js b/lib/arborist/load-actual.js index eeed570d5..d9e7fb46d 100644 --- a/lib/arborist/load-actual.js +++ b/lib/arborist/load-actual.js @@ -151,6 +151,9 @@ module.exports = cls => class ActualLoader extends cls { await new this.constructor({...this.options}).loadVirtual({ root: this[_actualTree], }) + await this[_loadWorkspaces](this[_actualTree]) + if (this[_actualTree].workspaces && this[_actualTree].workspaces.size) + calcDepFlags(this[_actualTree], !root) this[_transplant](root) return this[_actualTree] } diff --git a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js index fa4f726ee..485181c82 100644 --- a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js +++ b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js @@ -3834,6 +3834,82 @@ ArboristNode { } ` +exports[`test/arborist/load-actual.js TAP load workspaces when loading from hidding lockfile > actual tree 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/a", + "type": "workspace", + }, + }, + "location": "node_modules/a", + "name": "a", + "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/node_modules/a", + "realpath": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/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/load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/b", + "type": "workspace", + }, + }, + "location": "node_modules/b", + "name": "b", + "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/node_modules/b", + "realpath": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/b", + "resolved": "file:../packages/b", + "target": ArboristNode { + "location": "packages/b", + }, + "version": "1.2.3", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/a", + "to": "node_modules/a", + "type": "workspace", + }, + "b" => EdgeOut { + "name": "b", + "spec": "file:{CWD}/test/arborist/load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/b", + "to": "node_modules/b", + "type": "workspace", + }, + }, + "fsChildren": Set { + ArboristNode { + "location": "packages/a", + "name": "a", + "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "location": "packages/b", + "name": "b", + "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile/packages/b", + "version": "1.2.3", + }, + }, + "location": "", + "name": "load-actual-load-workspaces-when-loading-from-hidding-lockfile", + "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile", +} +` + exports[`test/arborist/load-actual.js TAP look for missing deps by default external-dep/root > "dep" should have missing deps, "link" should not 1`] = ` ArboristNode { "edgesOut": Map { diff --git a/test/arborist/load-actual.js b/test/arborist/load-actual.js index 50ee463a5..72d325d05 100644 --- a/test/arborist/load-actual.js +++ b/test/arborist/load-actual.js @@ -5,6 +5,7 @@ const Arborist = require('../../lib/arborist') const { resolve } = require('path') const Node = require('../../lib/node.js') const Shrinkwrap = require('../../lib/shrinkwrap.js') +const fs = require('fs') const { fixtures, @@ -295,3 +296,65 @@ t.test('transplant workspace targets, even if links not present', async t => { transplantFilter: node => node.name !== 'a', }), 'do not transplant node named "a"') }) + +t.test('load workspaces when loading from hidding lockfile', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + }), + node_modules: { + a: t.fixture('symlink', '../packages/a'), + b: t.fixture('symlink', '../packages/b'), + '.package-lock.json': JSON.stringify({ + name: 'workspace-abc', + lockfileVersion: 2, + requires: true, + packages: { + 'node_modules/a': { + resolved: 'packages/a', + link: true, + }, + 'node_modules/b': { + resolved: 'packages/b', + link: true, + }, + 'packages/a': { + version: '1.0.0', + }, + 'packages/b': { + version: '1.2.3', + }, + }, + }), + }, + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + // note: version changed since reifying + version: '1.2.3', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + }), + }, + }, + }) + const hidden = resolve(path, 'node_modules/.package-lock.json') + const then = Date.now() + 10000 + fs.utimesSync(hidden, new Date(then), new Date(then)) + const tree = await loadActual(path) + const aLink = tree.children.get('a') + const bLink = tree.children.get('b') + t.notOk(aLink.extraneous, 'a link not be extraneous') + t.notOk(bLink.extraneous, 'b link not be extraneous') + const aTarget = aLink.target + const bTarget = bLink.target + t.notOk(aTarget.extraneous, 'a target not be extraneous') + t.notOk(bTarget.extraneous, 'b target not be extraneous') + t.equal(aTarget.version, '1.2.3', 'updated a target version') + t.matchSnapshot(tree, 'actual tree') +}) From 4c5db920f157ddab507122908af934656c5b1b7b Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 29 Mar 2021 16:48:09 -0700 Subject: [PATCH 12/17] Show terse workspaces in printable output This is really important for debugging when "have workspaces been loaded yet" is a relevant consideration. --- lib/printable.js | 6 ++ ...t-arborist-build-ideal-tree.js-TAP.test.js | 42 ++++++++++ .../test-arborist-load-actual.js-TAP.test.js | 17 ++++ .../test-arborist-load-virtual.js-TAP.test.js | 38 +++++++++ .../test-arborist-reify.js-TAP.test.js | 39 +++++++++ tap-snapshots/test-printable.js-TAP.test.js | 83 +++++++++++++++++++ test/printable.js | 45 ++++++++++ 7 files changed, 270 insertions(+) diff --git a/lib/printable.js b/lib/printable.js index b39a53cc5..169984fcf 100644 --- a/lib/printable.js +++ b/lib/printable.js @@ -2,6 +2,7 @@ // of the current node and its descendents const util = require('util') +const relpath = require('./relpath.js') class ArboristNode { constructor (tree, path) { @@ -47,6 +48,11 @@ class ArboristNode { .map(edge => new EdgeIn(edge))) } + if (tree.workspaces && tree.workspaces.size) { + this.workspaces = new Map([...tree.workspaces.entries()] + .map(([name, path]) => [name, relpath(tree.root.realpath, path)])) + } + // fsChildren sorted by path if (tree.fsChildren.size) { this.fsChildren = new Set([...tree.fsChildren] diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index 36f743a68..69bc5d16c 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -153278,6 +153278,9 @@ ArboristNode { "name": "workspaces-ignore-nm", "packageName": "workspace-ignore-nm", "path": "{CWD}/test/fixtures/workspaces-ignore-nm", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -153369,6 +153372,10 @@ ArboristNode { "name": "workspaces-simple", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/workspaces-simple", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -153462,6 +153469,10 @@ ArboristNode { "name": "workspaces-scoped-pkg", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/workspaces-scoped-pkg", + "workspaces": Map { + "@ruyadorno/scoped-a" => "packages/a", + "@ruyadorno/scoped-b" => "packages/b", + }, } ` @@ -153611,6 +153622,10 @@ ArboristNode { "packageName": "workspaces-conflicting-deps", "path": "{CWD}/test/fixtures/workspaces-conflicting-versions", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, } ` @@ -153713,6 +153728,10 @@ ArboristNode { "name": "workspaces-version-unsatisfied", "path": "{CWD}/test/fixtures/workspaces-version-unsatisfied", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "abbrev" => "packages/abbrev", + }, } ` @@ -153881,6 +153900,11 @@ ArboristNode { "name": "workspaces-shared-deps", "path": "{CWD}/test/fixtures/workspaces-shared-deps", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -153975,6 +153999,9 @@ ArboristNode { "name": "workspaces-transitive-deps", "path": "{CWD}/test/fixtures/workspaces-transitive-deps", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -154021,6 +154048,9 @@ ArboristNode { "name": "workspaces-top-level-link", "path": "{CWD}/test/fixtures/workspaces-top-level-link", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -154112,6 +154142,10 @@ ArboristNode { "name": "workspaces-prefer-linking", "path": "{CWD}/test/fixtures/workspaces-prefer-linking", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "abbrev" => "packages/abbrev", + }, } ` @@ -154203,6 +154237,10 @@ ArboristNode { "name": "workspaces-simple", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/workspaces-simple", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -154369,5 +154407,9 @@ ArboristNode { "location": "", "name": "workspaces-with-files-spec", "path": "{CWD}/test/fixtures/workspaces-with-files-spec", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, } ` diff --git a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js index 485181c82..da2ac11b5 100644 --- a/tap-snapshots/test-arborist-load-actual.js-TAP.test.js +++ b/tap-snapshots/test-arborist-load-actual.js-TAP.test.js @@ -3831,6 +3831,11 @@ ArboristNode { "location": "", "name": "load-actual-load-workspace-targets-even-if-links-not-present", "path": "load-actual-load-workspace-targets-even-if-links-not-present", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -3907,6 +3912,10 @@ ArboristNode { "location": "", "name": "load-actual-load-workspaces-when-loading-from-hidding-lockfile", "path": "load-actual-load-workspaces-when-loading-from-hidding-lockfile", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, } ` @@ -7542,6 +7551,10 @@ ArboristNode { "name": "workspaces-simple", "packageName": "workspace-simple", "path": "workspaces-simple", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -7633,6 +7646,10 @@ ArboristNode { "name": "workspaces-simple", "packageName": "workspace-simple", "path": "workspaces-simple", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` diff --git a/tap-snapshots/test-arborist-load-virtual.js-TAP.test.js b/tap-snapshots/test-arborist-load-virtual.js-TAP.test.js index f59247c11..db326336e 100644 --- a/tap-snapshots/test-arborist-load-virtual.js-TAP.test.js +++ b/tap-snapshots/test-arborist-load-virtual.js-TAP.test.js @@ -717,6 +717,10 @@ ArboristNode { "name": "workspaces-changed", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/edit-package-json/workspaces-changed", + "workspaces": Map { + "a" => "a", + "c" => "c", + }, } ` @@ -808,6 +812,10 @@ ArboristNode { "name": "workspaces-simple-virtual", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/workspaces-simple-virtual", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -15804,6 +15812,10 @@ ArboristNode { "name": "workspaces-simple-virtual", "packageName": "workspace-simple", "path": "{CWD}/test/fixtures/workspaces-simple-virtual", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -15953,6 +15965,10 @@ ArboristNode { "packageName": "workspaces-conflicting-deps", "path": "{CWD}/test/fixtures/workspaces-conflicting-versions-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, } ` @@ -16056,6 +16072,10 @@ ArboristNode { "packageName": "workspaces-version-unsatisfied", "path": "{CWD}/test/fixtures/workspaces-version-unsatisfied-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "abbrev" => "packages/abbrev", + }, } ` @@ -16102,6 +16122,9 @@ ArboristNode { "name": "workspaces-ignore-nm-virtual", "packageName": "workspace-ignore-nm", "path": "{CWD}/test/fixtures/workspaces-ignore-nm-virtual", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -16197,6 +16220,9 @@ ArboristNode { "packageName": "workspaces-transitive-deps", "path": "{CWD}/test/fixtures/workspaces-transitive-deps-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -16244,6 +16270,9 @@ ArboristNode { "packageName": "workspaces-top-level-link", "path": "{CWD}/test/fixtures/workspaces-top-level-link-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + }, } ` @@ -16336,6 +16365,10 @@ ArboristNode { "packageName": "workspaces-prefer-linking", "path": "{CWD}/test/fixtures/workspaces-prefer-linking-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "abbrev" => "packages/abbrev", + }, } ` @@ -16505,5 +16538,10 @@ ArboristNode { "packageName": "workspaces-shared-deps", "path": "{CWD}/test/fixtures/workspaces-shared-deps-virtual", "version": "1.0.0", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js index 7ff27319f..9e0bdfb78 100644 --- a/tap-snapshots/test-arborist-reify.js-TAP.test.js +++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js @@ -1984,6 +1984,12 @@ ArboristNode { "location": "", "name": "reify-filtered-reification-in-workspaces", "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", + "workspaces": Map { + "x" => "apps/x", + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -2075,6 +2081,12 @@ ArboristNode { "location": "", "name": "reify-filtered-reification-in-workspaces", "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", + "workspaces": Map { + "x" => "apps/x", + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -2211,6 +2223,11 @@ ArboristNode { "location": "", "name": "reify-filtered-reification-in-workspaces", "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", + "workspaces": Map { + "x" => "foo/x", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -2321,6 +2338,11 @@ ArboristNode { "location": "", "name": "reify-filtered-reification-in-workspaces", "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", + "workspaces": Map { + "x" => "foo/x", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -2420,6 +2442,12 @@ ArboristNode { "location": "", "name": "reify-filtered-reification-in-workspaces", "path": "{CWD}/test/arborist/reify-filtered-reification-in-workspaces", + "workspaces": Map { + "x" => "apps/x", + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, } ` @@ -49873,6 +49901,9 @@ ArboristNode { "name": "reify-workspaces-reify-from-an-actual-loaded-workspace-env", "packageName": "workspaces-non-simplistic", "path": "{CWD}/test/arborist/reify-workspaces-reify-from-an-actual-loaded-workspace-env", + "workspaces": Map { + "pkg-a" => "a", + }, } ` @@ -49964,6 +49995,10 @@ ArboristNode { "name": "reify-workspaces-reify-simple-workspaces", "packageName": "workspace-simple", "path": "{CWD}/test/arborist/reify-workspaces-reify-simple-workspaces", + "workspaces": Map { + "a" => "a", + "b" => "b", + }, } ` @@ -50041,6 +50076,10 @@ ArboristNode { "name": "reify-workspaces-reify-workspaces-bin-files", "packageName": "workspace-duplicate", "path": "{CWD}/test/arborist/reify-workspaces-reify-workspaces-bin-files", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + }, } ` diff --git a/tap-snapshots/test-printable.js-TAP.test.js b/tap-snapshots/test-printable.js-TAP.test.js index b797d46eb..2437623ec 100644 --- a/tap-snapshots/test-printable.js-TAP.test.js +++ b/tap-snapshots/test-printable.js-TAP.test.js @@ -218,6 +218,89 @@ peer:true, target:{location:'c'}}}} ` +exports[`test/printable.js TAP show workspaces in printable node output > must match snapshot 1`] = ` +{ +"children":Map{ +"a" => ArboristLink{ +"dev":true, +"edgesIn":Set{ +EdgeIn{ +"from":"", +"name":"a", +"spec":"file:/home/user/projects/root/packages/a", +"type":"workspace",},}, +"extraneous":true, +"location":"node_modules/a", +"name":"a", +"optional":true, +"path":"/home/user/projects/root/node_modules/a", +"peer":true, +"realpath":"/home/user/projects/root/packages/a", +"resolved":"file:../packages/a", +"target":{ +"location":"packages/a",}, +"version":"1.2.3",}, +"b" => ArboristLink{ +"dev":true, +"edgesIn":Set{ +EdgeIn{ +"from":"", +"name":"b", +"spec":"file:/home/user/projects/root/packages/b", +"type":"workspace",},}, +"extraneous":true, +"location":"node_modules/b", +"name":"b", +"optional":true, +"path":"/home/user/projects/root/node_modules/b", +"peer":true, +"realpath":"/home/user/projects/root/packages/b", +"resolved":"file:../packages/b", +"target":{ +"location":"packages/b",}, +"version":"1.2.3",},}, +"dev":true, +"edgesOut":Map{ +"a" => EdgeOut{ +"name":"a", +"spec":"file:/home/user/projects/root/packages/a", +"to":"node_modules/a", +"type":"workspace",}, +"b" => EdgeOut{ +"name":"b", +"spec":"file:/home/user/projects/root/packages/b", +"to":"node_modules/b", +"type":"workspace",},}, +"extraneous":true, +"fsChildren":Set{ +{ +"dev":true, +"extraneous":true, +"location":"packages/a", +"name":"a", +"optional":true, +"path":"/home/user/projects/root/packages/a", +"peer":true, +"version":"1.2.3",}, +{ +"dev":true, +"extraneous":true, +"location":"packages/b", +"name":"b", +"optional":true, +"path":"/home/user/projects/root/packages/b", +"peer":true, +"version":"1.2.3",},}, +"location":"", +"name":"root", +"optional":true, +"path":"/home/user/projects/root", +"peer":true, +"workspaces":Map{ +"a" => "packages/a", +"b" => "packages/b",},} +` + exports[`test/printable.js TAP virtual roots are shown with their sourceReference > must match snapshot 1`] = ` ArboristVirtualNode{ "dev":true, diff --git a/test/printable.js b/test/printable.js index 016a93e59..b0dee2a6e 100644 --- a/test/printable.js +++ b/test/printable.js @@ -1,6 +1,7 @@ const t = require('tap') const Node = require('../lib/node.js') const Link = require('../lib/link.js') +const Edge = require('../lib/edge.js') const printable = require('../lib/printable.js') const util = require('util') @@ -243,3 +244,47 @@ t.test('broken links dont break the printing', t => { t.matchSnapshot(printable(tree)) t.end() }) + +t.test('show workspaces in printable node output', t => { + const tree = new Node({ + path: '/home/user/projects/root', + pkg: { + workspaces: ['packages/*'], + }, + }) + new Edge({ + from: tree, + type: 'workspace', + name: 'a', + spec: 'file:/home/user/projects/root/packages/a', + }) + new Edge({ + from: tree, + type: 'workspace', + name: 'b', + spec: 'file:/home/user/projects/root/packages/b', + }) + new Link({ + pkg: { + name: 'a', + version: '1.2.3', + }, + realpath: '/home/user/projects/root/packages/a', + parent: tree, + }) + new Link({ + pkg: { + name: 'b', + version: '1.2.3', + }, + realpath: '/home/user/projects/root/packages/b', + parent: tree, + }) + tree.workspaces = new Map([ + ['a', tree.children.get('a').realpath], + ['b', tree.children.get('b').realpath], + ]) + + t.matchSnapshot(printable(tree)) + t.end() +}) From a98f255f2d432782a719dcd0af0f9dd6639c1916 Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 29 Mar 2021 17:32:44 -0700 Subject: [PATCH 13/17] idealTree: apply user requests immediately before buildDeps --- lib/arborist/build-ideal-tree.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index 1f73f15fa..b4213d3c8 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -211,8 +211,8 @@ module.exports = cls => class IdealTreeBuilder extends cls { try { await this[_initTree]() - await this[_applyUserRequests](options) await this[_inflateAncientLockfile]() + await this[_applyUserRequests](options) await this[_buildDeps]() await this[_fixDepFlags]() await this[_pruneFailedOptional]() From 59df588d4dc04e88d1284c2c3748f5c3768e1579 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 30 Mar 2021 10:39:40 -0700 Subject: [PATCH 14/17] idealTree: Add/remove to/from workspaces when set, not root --- lib/arborist/build-ideal-tree.js | 43 +- ...t-arborist-build-ideal-tree.js-TAP.test.js | 398 ++++++++++++++++++ test/arborist/build-ideal-tree.js | 91 +++- 3 files changed, 520 insertions(+), 12 deletions(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index b4213d3c8..d30f49260 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -51,6 +51,7 @@ const _legacyBundling = Symbol('legacyBundling') const _parseSettings = Symbol('parseSettings') const _initTree = Symbol('initTree') const _applyUserRequests = Symbol('applyUserRequests') +const _applyUserRequestsToNode = Symbol('applyUserRequestsToNode') const _inflateAncientLockfile = Symbol('inflateAncientLockfile') const _buildDeps = Symbol('buildDeps') const _buildDepStep = Symbol('buildDepStep') @@ -396,6 +397,41 @@ module.exports = cls => class IdealTreeBuilder extends cls { process.emit('time', 'idealTree:userRequests') const tree = this.idealTree.target || this.idealTree + if (!this[_workspaces].length) { + return this[_applyUserRequestsToNode](tree, options).then(() => + process.emit('timeEnd', 'idealTree:userRequests')) + } + + const wsMap = tree.workspaces + if (!wsMap) { + this.log.warn('idealTree', 'Workspace filter set, but no workspaces present') + return + } + + const promises = [] + for (const name of this[_workspaces]) { + const path = wsMap.get(name) + if (!path) { + this.log.warn('idealTree', `Workspace ${name} in filter set, but not in workspaces`) + continue + } + const loc = relpath(tree.realpath, path) + const node = tree.inventory.get(loc) + + /* istanbul ignore if - should be impossible */ + if (!node) { + this.log.warn('idealTree', `Workspace ${name} in filter set, but no workspace folder present`) + continue + } + + promises.push(this[_applyUserRequestsToNode](node, options)) + } + + return Promise.all(promises).then(() => + process.emit('timeEnd', 'idealTree:userRequests')) + } + + async [_applyUserRequestsToNode] (tree, options) { // If we have a list of package names to update, and we know it's // going to update them wherever they are, add any paths into those // named nodes to the buildIdealTree queue. @@ -430,7 +466,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { } if (add && add.length) - await this[_add](options) + await this[_add](tree, options) // triggers a refresh of all edgesOut. this has to be done BEFORE // adding the edges to explicitRequests, because the package setter @@ -442,13 +478,11 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_explicitRequests].add(tree.edgesOut.get(spec.name)) for (const name of globalExplicitUpdateNames) this[_explicitRequests].add(tree.edgesOut.get(name)) - - process.emit('timeEnd', 'idealTree:userRequests') } // This returns a promise because we might not have the name yet, // and need to call pacote.manifest to find the name. - [_add] ({add, saveType = null, saveBundle = false}) { + [_add] (tree, {add, saveType = null, saveBundle = false}) { // get the name for each of the specs in the list. // ie, doing `foo@bar` we just return foo // but if it's a url or git, we don't know the name until we @@ -463,7 +497,6 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_resolvedAdd].push(...add) // now add is a list of spec objects with names. // find a home for each of them! - const tree = this.idealTree.target || this.idealTree addRmPkgDeps.add({ pkg: tree.package, add, diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index 69bc5d16c..c089b90ee 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -332,6 +332,404 @@ ArboristNode { } ` +exports[`test/arborist/build-ideal-tree.js TAP add packages to workspaces, not root > tree with abbrev removed from a and b 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "type": "workspace", + }, + }, + "location": "node_modules/a", + "name": "a", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/a", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "resolved": "file:../packages/a", + "target": ArboristNode { + "location": "packages/a", + }, + "version": "1.2.3", + }, + "abbrev" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "abbrev", + "spec": "*", + "type": "prod", + }, + EdgeIn { + "from": "packages/c", + "name": "abbrev", + "spec": "*", + "type": "prod", + }, + }, + "location": "node_modules/abbrev", + "name": "abbrev", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/abbrev", + "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", + "version": "1.1.1", + }, + "b" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "b", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "type": "workspace", + }, + }, + "location": "node_modules/b", + "name": "b", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/b", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "resolved": "file:../packages/b", + "target": ArboristNode { + "location": "packages/b", + }, + "version": "1.2.3", + }, + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/c", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "version": "1.0.0", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "to": "node_modules/a", + "type": "workspace", + }, + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + "b" => EdgeOut { + "name": "b", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "to": "node_modules/b", + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.0", + "to": "node_modules/wrappy", + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "location": "packages/a", + "name": "a", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "location": "packages/b", + "name": "b", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "build-ideal-tree-add-packages-to-workspaces-not-root", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, +} +` + +exports[`test/arborist/build-ideal-tree.js TAP add packages to workspaces, not root > tree with wrappy added to a and c 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "type": "workspace", + }, + }, + "location": "node_modules/a", + "name": "a", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/a", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "resolved": "file:../packages/a", + "target": ArboristNode { + "location": "packages/a", + }, + "version": "1.2.3", + }, + "abbrev" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "abbrev", + "spec": "*", + "type": "prod", + }, + EdgeIn { + "from": "packages/b", + "name": "abbrev", + "spec": "*", + "type": "prod", + }, + EdgeIn { + "from": "packages/c", + "name": "abbrev", + "spec": "*", + "type": "prod", + }, + }, + "location": "node_modules/abbrev", + "name": "abbrev", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/abbrev", + "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", + "version": "1.1.1", + }, + "b" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "b", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "type": "workspace", + }, + }, + "location": "node_modules/b", + "name": "b", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/b", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "resolved": "file:../packages/b", + "target": ArboristNode { + "location": "packages/b", + }, + "version": "1.2.3", + }, + "c" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "c", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "type": "workspace", + }, + }, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/c", + "realpath": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "resolved": "file:../packages/c", + "target": ArboristNode { + "location": "packages/c", + }, + "version": "1.2.3", + }, + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "wrappy", + "spec": "1.0.0", + "type": "prod", + }, + }, + "location": "node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.0.tgz", + "version": "1.0.0", + }, + }, + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "to": "node_modules/a", + "type": "workspace", + }, + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + "b" => EdgeOut { + "name": "b", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "to": "node_modules/b", + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "to": "node_modules/c", + "type": "workspace", + }, + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.0", + "to": "node_modules/wrappy", + "type": "prod", + }, + }, + "fsChildren": Set { + ArboristNode { + "children": Map { + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/a", + "name": "wrappy", + "spec": "1.0.1", + "type": "prod", + }, + }, + "location": "packages/a/node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.1.tgz", + "version": "1.0.1", + }, + }, + "edgesOut": Map { + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.1", + "to": "packages/a/node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/a", + "name": "a", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + }, + "location": "packages/b", + "name": "b", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/b", + "version": "1.2.3", + }, + ArboristNode { + "children": Map { + "wrappy" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "packages/c", + "name": "wrappy", + "spec": "1.0.1", + "type": "prod", + }, + }, + "location": "packages/c/node_modules/wrappy", + "name": "wrappy", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c/node_modules/wrappy", + "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.1.tgz", + "version": "1.0.1", + }, + }, + "edgesOut": Map { + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + "wrappy" => EdgeOut { + "name": "wrappy", + "spec": "1.0.1", + "to": "packages/c/node_modules/wrappy", + "type": "prod", + }, + }, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root/packages/c", + "version": "1.2.3", + }, + }, + "location": "", + "name": "build-ideal-tree-add-packages-to-workspaces-not-root", + "path": "{CWD}/test/arborist/build-ideal-tree-add-packages-to-workspaces-not-root", + "workspaces": Map { + "a" => "packages/a", + "b" => "packages/b", + "c" => "packages/c", + }, +} +` + exports[`test/arborist/build-ideal-tree.js TAP add symlink that points to a symlink > should follow symlinks to find final realpath destination 1`] = ` ArboristNode { "children": Map { diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index a5c919a9b..653b05473 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -2725,12 +2725,13 @@ t.test('cannot do workspaces in global mode', t => { t.end() }) -t.todo('add packages to workspaces, not root', async t => { +t.test('add packages to workspaces, not root', async t => { const path = t.testdir({ 'package.json': JSON.stringify({ workspaces: ['packages/*'], dependencies: { wrappy: '1.0.0', + abbrev: '', }, }), packages: { @@ -2744,27 +2745,103 @@ t.todo('add packages to workspaces, not root', async t => { 'package.json': JSON.stringify({ name: 'b', version: '1.2.3', + dependencies: { + abbrev: '', + }, }), }, c: { 'package.json': JSON.stringify({ name: 'c', version: '1.2.3', + dependencies: { + abbrev: '', + }, }), }, }, }) - const tree = await buildIdeal(path, { + const addTree = await buildIdeal(path, { add: ['wrappy@1.0.1'], workspaces: ['a', 'c'], }) - t.match(tree.edgesOut.get('wrappy'), { spec: '1.0.0' }) - const a = tree.children.get('a').target - const b = tree.children.get('b').target - const c = tree.children.get('c').target + t.match(addTree.edgesOut.get('wrappy'), { spec: '1.0.0' }) + const a = addTree.children.get('a').target + const b = addTree.children.get('b').target + const c = addTree.children.get('c').target t.match(a.edgesOut.get('wrappy'), { spec: '1.0.1' }) t.equal(b.edgesOut.get('wrappy'), undefined) t.match(c.edgesOut.get('wrappy'), { spec: '1.0.1' }) - t.matchSnapshot(printTree(tree)) + t.matchSnapshot(printTree(addTree), 'tree with wrappy added to a and c') + + const rmTree = await buildIdeal(path, { + rm: ['abbrev'], + workspaces: ['a', 'b'], + }) + t.match(rmTree.edgesOut.get('abbrev'), { spec: '' }) + t.equal(rmTree.children.get('a').target.edgesOut.get('abbrev'), undefined) + t.equal(rmTree.children.get('b').target.edgesOut.get('abbrev'), undefined) + t.match(rmTree.children.get('c').target.edgesOut.get('abbrev'), { spec: '' }) + t.matchSnapshot(printTree(rmTree), 'tree with abbrev removed from a and b') +}) + +t.test('workspace error handling', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + workspaces: ['packages/*'], + dependencies: { + wrappy: '1.0.0', + abbrev: '', + }, + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.2.3', + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.2.3', + dependencies: { + abbrev: '', + }, + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.2.3', + dependencies: { + abbrev: '', + }, + }), + }, + }, + }) + t.test('set filter, but no workspaces present', async t => { + const logs = warningTracker() + await buildIdeal(resolve(path, 'packages/a'), { + workspaces: ['a'], + }) + t.strictSame(logs(), [[ + 'warn', + 'idealTree', + 'Workspace filter set, but no workspaces present', + ]], 'got warning') + }) + t.test('set filter for workspace that is not present', async t => { + const logs = warningTracker() + await buildIdeal(path, { + workspaces: ['not-here'], + }) + t.strictSame(logs(), [[ + 'warn', + 'idealTree', + 'Workspace not-here in filter set, but not in workspaces', + ]], 'got warning') + }) }) From 0d401ad286eace33fdf70f25244641857e1a2ca7 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 31 Mar 2021 17:58:40 -0700 Subject: [PATCH 15/17] Link nodes to the root should have the root as a target --- lib/link.js | 20 +++++++++++++------- test/link.js | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/lib/link.js b/lib/link.js index 2394c6e41..4d15428d8 100644 --- a/lib/link.js +++ b/lib/link.js @@ -23,13 +23,19 @@ class Link extends Node { : null), }) - this.target = target || new Node({ - ...options, - path: realpath, - parent: null, - fsParent: null, - root: this.root, - }) + if (target) + this.target = target + else if (this.realpath === this.root.path) + this.target = this.root + else { + this.target = new Node({ + ...options, + path: realpath, + parent: null, + fsParent: null, + root: this.root, + }) + } } get version () { diff --git a/test/link.js b/test/link.js index f9fb1abfc..1d84fccf8 100644 --- a/test/link.js +++ b/test/link.js @@ -220,3 +220,22 @@ t.test('link gets version from target', t => { t.equal(link.version, '1.2.3') t.end() }) + +t.test('link to root path gets root as target', t => { + const root = new Node({ + path: '/project/root', + pkg: { + name: 'root', + dependencies: { + root: 'file:.', + }, + }, + }) + const link = new Link({ + parent: root, + realpath: root.path, + pkg: { ...root.package }, + }) + t.equal(link.target, root) + t.end() +}) From 1b499cbd2db8f1434fbb68b779f68c261e8bf6cf Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 31 Mar 2021 17:59:10 -0700 Subject: [PATCH 16/17] tree-check: add check for nodes with same path as root --- lib/tree-check.js | 47 ++++++++++++++++++++++++++++++++++++++++- test/tree-check.js | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/lib/tree-check.js b/lib/tree-check.js index 00b43296f..a7e8d9c01 100644 --- a/lib/tree-check.js +++ b/lib/tree-check.js @@ -1,6 +1,8 @@ const debug = require('./debug.js') const checkTree = (tree, checkUnreachable = true) => { + const log = [['START TREE CHECK', tree.path]] + // this can only happen in tests where we have a "tree" object // that isn't actually a tree. if (!tree.root || !tree.root.inventory) @@ -9,8 +11,21 @@ const checkTree = (tree, checkUnreachable = true) => { const { inventory } = tree.root const seen = new Set() const check = (node, via = tree, viaType = 'self') => { + log.push([ + 'CHECK', + node && node.location, + via && via.location, + viaType, + 'seen=' + seen.has(node), + 'promise=' + !!(node && node.then), + 'root=' + !!(node && node.isRoot), + ]) + if (!node || seen.has(node) || node.then) return + + seen.add(node) + if (node.isRoot && node !== tree.root) { throw Object.assign(new Error('double root'), { node: node.path, @@ -19,6 +34,7 @@ const checkTree = (tree, checkUnreachable = true) => { root: tree.root.path, via: via.path, viaType, + log, }) } @@ -31,6 +47,7 @@ const checkTree = (tree, checkUnreachable = true) => { via: via.path, viaType, otherRoot: node.root && node.root.path, + log, }) } @@ -43,6 +60,7 @@ const checkTree = (tree, checkUnreachable = true) => { viaType, inventory: [...node.inventory.values()].map(node => [node.path, node.location]), + log, }) } @@ -53,6 +71,7 @@ const checkTree = (tree, checkUnreachable = true) => { root: tree.root.path, via: via.path, viaType, + log, }) } @@ -65,14 +84,38 @@ const checkTree = (tree, checkUnreachable = true) => { via: via.path, viaType, devEdges: devEdges.map(e => [e.type, e.name, e.spec, e.error]), + log, + }) + } + + if (node.path === tree.root.path && node !== tree.root) { + throw Object.assign(new Error('node with same path as root'), { + node: node.path, + tree: tree.path, + root: tree.root.path, + via: via.path, + viaType, + log, + }) + } + + if (!node.isLink && node.path !== node.realpath) { + throw Object.assign(new Error('non-link with mismatched path/realpath'), { + node: node.path, + tree: tree.path, + realpath: node.realpath, + root: tree.root.path, + via: via.path, + viaType, + log, }) } const { parent, fsParent, target } = node - seen.add(node) check(parent, node, 'parent') check(fsParent, node, 'fsParent') check(target, node, 'target') + log.push(['CHILDREN', node.location, ...node.children.keys()]) for (const kid of node.children.values()) check(kid, node, 'children') for (const kid of node.fsChildren) @@ -81,6 +124,7 @@ const checkTree = (tree, checkUnreachable = true) => { check(link, node, 'linksIn') for (const top of node.tops) check(top, node, 'tops') + log.push(['DONE', node.location]) } check(tree) if (checkUnreachable) { @@ -92,6 +136,7 @@ const checkTree = (tree, checkUnreachable = true) => { location: node.location, root: tree.root.path, tree: tree.path, + log, }) } } diff --git a/test/tree-check.js b/test/tree-check.js index a290a0b1d..0e0d98246 100644 --- a/test/tree-check.js +++ b/test/tree-check.js @@ -69,6 +69,7 @@ t.test('break some stuff', t => { realpath: resolve('/some/path/node_modules/disowned'), location: '', name: 'Error', + log: Array, }) Map.prototype.delete.call(tree.inventory, 'xyz') @@ -78,6 +79,7 @@ t.test('break some stuff', t => { message: 'not in inventory', node: resolve('/some/path/node_modules/disowned'), name: 'Error', + log: Array, }) disowned.root = null @@ -88,6 +90,7 @@ t.test('break some stuff', t => { realpath: resolve('/some/path/node_modules/disowned'), tree: tree.path, name: 'Error', + log: Array, }) const otherTree = new Node({ @@ -101,6 +104,7 @@ t.test('break some stuff', t => { realpath: resolve('/some/path/node_modules/disowned/node_modules/other'), tree: tree.path, name: 'Error', + log: Array, }) tree.children.delete('wtf') @@ -111,6 +115,7 @@ t.test('break some stuff', t => { tree: otherTree.path, inventory: [[disowned.path, disowned.location]], name: 'Error', + log: Array, }) Map.prototype.delete.call(tree.inventory, 'othertree') @@ -160,6 +165,53 @@ t.test('tree with dev edges on a nested dep node', t => { via: tree.path, viaType: 'children', devEdges: [['dev', 'x', '', 'MISSING']], + log: Array, + }) + t.end() +}) + +t.test('node with same path as root', t => { + const root = new Node({ + path: '/path/to/root', + pkg: { + dependencies: { + foo: '1', + }, + }, + }) + const tree = new Node({ + parent: root, + pkg: { + name: 'foo', + version: '1.2.3', + }, + }) + const child = new Node({ + parent: tree, + pkg: { + name: 'not-root', + version: '1.2.3', + }, + }) + child.path = root.path + t.throws(() => treeCheck(tree), { + message: 'node with same path as root', + node: child.path, + tree: tree.path, + root: root.path, + via: tree.path, + viaType: 'children', + log: Array, + }) + child.path = root.path + '/some/where/else' + t.throws(() => treeCheck(tree), { + message: 'non-link with mismatched path/realpath', + node: child.path, + tree: tree.path, + root: root.path, + via: tree.path, + viaType: 'children', + log: Array, }) t.end() }) From 44522250e5d3e5e7b89fb915d7383970d631a170 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 31 Mar 2021 18:21:44 -0700 Subject: [PATCH 17/17] Provide currentEdge in ERESOLVE if known This provides another fail-safe for the CLI to report _something_ useful to the user when an ERESOLVE happens, if by some brokenness in our tree handling we don't have the node in the tree somehow to provide a current or peerConflict. --- lib/arborist/build-ideal-tree.js | 14 +- ...t-arborist-build-ideal-tree.js-TAP.test.js | 227 ++++++++++++++++++ test/arborist/build-ideal-tree.js | 25 ++ 3 files changed, 261 insertions(+), 5 deletions(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index d30f49260..f836fc04d 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -1184,7 +1184,7 @@ This is a one-time fix-up, please be patient... continue // problem - this[_failPeerConflict](edge) + this[_failPeerConflict](edge, parentEdge) } } @@ -1200,17 +1200,17 @@ This is a one-time fix-up, please be patient... continue // ok, it's the root, or we're in unforced strict mode, so this is bad - this[_failPeerConflict](edge) + this[_failPeerConflict](edge, parentEdge) } return node } - [_failPeerConflict] (edge) { - const expl = this[_explainPeerConflict](edge) + [_failPeerConflict] (edge, currentEdge) { + const expl = this[_explainPeerConflict](edge, currentEdge) throw Object.assign(new Error('unable to resolve dependency tree'), expl) } - [_explainPeerConflict] (edge) { + [_explainPeerConflict] (edge, currentEdge) { const node = edge.from const curNode = node.resolve(edge.name) const pc = this[_peerConflict] || { peer: null, current: null } @@ -1219,6 +1219,10 @@ This is a one-time fix-up, please be patient... return { code: 'ERESOLVE', current, + // it SHOULD be impossible to get here without a current node in place, + // but this at least gives us something report on when bugs creep into + // the tree handling logic. + currentEdge: currentEdge ? currentEdge.explain() : null, edge: edge.explain(), peerConflict, strictPeerDeps: this[_strictPeerDeps], diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index c089b90ee..9cfc76e53 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -65782,6 +65782,230 @@ ArboristNode { } ` +exports[`test/arborist/build-ideal-tree.js TAP more peer dep conflicts conflict on root edge, order 1 > force result 1`] = ` +ArboristNode { + "children": Map { + "@isaacs/testing-peer-dep-conflict-chain-a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "file:.", + "type": "prod", + }, + EdgeIn { + "error": "INVALID", + "from": "node_modules/@isaacs/testing-peer-dep-conflict-chain-e", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "1", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1/node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "realpath": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1", + "resolved": "file:../..", + "target": ArboristNode { + "location": "", + }, + "version": "2.3.4", + }, + "@isaacs/testing-peer-dep-conflict-chain-d" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "1", + "type": "prod", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-e" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-e", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-e", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1/node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-dep-conflict-chain-d/-/testing-peer-dep-conflict-chain-d-1.0.0.tgz", + "version": "1.0.0", + }, + "@isaacs/testing-peer-dep-conflict-chain-e" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "name": "@isaacs/testing-peer-dep-conflict-chain-e", + "spec": "1", + "type": "peer", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-a" => EdgeOut { + "error": "INVALID", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-e", + "name": "@isaacs/testing-peer-dep-conflict-chain-e", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1/node_modules/@isaacs/testing-peer-dep-conflict-chain-e", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-dep-conflict-chain-e/-/testing-peer-dep-conflict-chain-e-1.0.0.tgz", + "version": "1.0.0", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-a" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "file:.", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "type": "prod", + }, + "@isaacs/testing-peer-dep-conflict-chain-d" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "type": "prod", + }, + }, + "location": "", + "name": "build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1", + "packageName": "@isaacs/testing-peer-dep-conflict-chain-a", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-1", + "version": "2.3.4", +} +` + +exports[`test/arborist/build-ideal-tree.js TAP more peer dep conflicts conflict on root edge, order 2 > force result 1`] = ` +ArboristNode { + "children": Map { + "@isaacs/testing-peer-dep-conflict-chain-a" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "1", + "type": "prod", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-b" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-b", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-b", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2/node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-dep-conflict-chain-a/-/testing-peer-dep-conflict-chain-a-1.0.0.tgz", + "version": "1.0.0", + }, + "@isaacs/testing-peer-dep-conflict-chain-b" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "name": "@isaacs/testing-peer-dep-conflict-chain-b", + "spec": "1", + "type": "peer", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-c" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-c", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-c", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-b", + "name": "@isaacs/testing-peer-dep-conflict-chain-b", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2/node_modules/@isaacs/testing-peer-dep-conflict-chain-b", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-dep-conflict-chain-b/-/testing-peer-dep-conflict-chain-b-1.0.0.tgz", + "version": "1.0.0", + }, + "@isaacs/testing-peer-dep-conflict-chain-c" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-dep-conflict-chain-b", + "name": "@isaacs/testing-peer-dep-conflict-chain-c", + "spec": "1", + "type": "peer", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-d" => EdgeOut { + "error": "INVALID", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-c", + "name": "@isaacs/testing-peer-dep-conflict-chain-c", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2/node_modules/@isaacs/testing-peer-dep-conflict-chain-c", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-dep-conflict-chain-c/-/testing-peer-dep-conflict-chain-c-1.0.0.tgz", + "version": "1.0.0", + }, + "@isaacs/testing-peer-dep-conflict-chain-d" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "file:.", + "type": "prod", + }, + EdgeIn { + "error": "INVALID", + "from": "node_modules/@isaacs/testing-peer-dep-conflict-chain-c", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "1", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2/node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "realpath": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2", + "resolved": "file:../..", + "target": ArboristNode { + "location": "", + }, + "version": "2.3.4", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-dep-conflict-chain-a" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-a", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-a", + "type": "prod", + }, + "@isaacs/testing-peer-dep-conflict-chain-d" => EdgeOut { + "name": "@isaacs/testing-peer-dep-conflict-chain-d", + "spec": "file:.", + "to": "node_modules/@isaacs/testing-peer-dep-conflict-chain-d", + "type": "prod", + }, + }, + "location": "", + "name": "build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2", + "packageName": "@isaacs/testing-peer-dep-conflict-chain-d", + "path": "{CWD}/test/arborist/build-ideal-tree-more-peer-dep-conflicts-conflict-on-root-edge-order-2", + "version": "2.3.4", +} +` + exports[`test/arborist/build-ideal-tree.js TAP more peer dep conflicts dep indirectly on conflicted peer > force result 1`] = ` ArboristNode { "children": Map { @@ -68948,6 +69172,7 @@ Array [ "name": "@isaacs/testing-peer-dep-conflict-chain-a", "version": "1.0.0", }, + "currentEdge": null, "edge": Object { "error": "INVALID", "from": Object { @@ -69625,6 +69850,7 @@ Array [ "name": "@isaacs/testing-peer-dep-conflict-chain-b", "version": "2.0.0", }, + "currentEdge": null, "edge": Object { "error": "INVALID", "from": Object { @@ -69695,6 +69921,7 @@ Array [ "name": "@isaacs/testing-peer-dep-conflict-chain-a", "version": "1.0.0", }, + "currentEdge": null, "edge": Object { "error": "INVALID", "from": Object { diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index 653b05473..6b195cbd8 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -1765,6 +1765,31 @@ t.test('more peer dep conflicts', t => { error: true, resolvable: false, }, + + 'conflict on root edge, order 1': { + pkg: { + name: '@isaacs/testing-peer-dep-conflict-chain-a', + version: '2.3.4', + dependencies: { + '@isaacs/testing-peer-dep-conflict-chain-a': 'file:.', + '@isaacs/testing-peer-dep-conflict-chain-d': '1', + }, + }, + error: true, + resolvable: false, + }, + 'conflict on root edge, order 2': { + pkg: { + name: '@isaacs/testing-peer-dep-conflict-chain-d', + version: '2.3.4', + dependencies: { + '@isaacs/testing-peer-dep-conflict-chain-a': '1', + '@isaacs/testing-peer-dep-conflict-chain-d': 'file:.', + }, + }, + error: true, + resolvable: false, + }, }) t.jobs = cases.length