From adb7dcb02d7cd10e4324cfe751372c1a9ba7aeba Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 23 Aug 2022 08:23:20 -0700 Subject: [PATCH 1/2] fix: inline single-use functions --- workspaces/arborist/lib/add-rm-pkg-deps.js | 131 ++++++++-------- .../arborist/lib/arborist/build-ideal-tree.js | 148 ++++++------------ 2 files changed, 112 insertions(+), 167 deletions(-) diff --git a/workspaces/arborist/lib/add-rm-pkg-deps.js b/workspaces/arborist/lib/add-rm-pkg-deps.js index f59df359e9456..7b43c38e2492b 100644 --- a/workspaces/arborist/lib/add-rm-pkg-deps.js +++ b/workspaces/arborist/lib/add-rm-pkg-deps.js @@ -4,8 +4,67 @@ const log = require('proc-log') const localeCompare = require('@isaacs/string-locale-compare')('en') const add = ({ pkg, add, saveBundle, saveType }) => { - for (const spec of add) { - addSingle({ pkg, spec, saveBundle, saveType }) + for (const { name, rawSpec } of add) { + // if the user does not give us a type, we infer which type(s) + // to keep based on the same order of priority we do when + // building the tree as defined in the _loadDeps method of + // the node class. + if (!saveType) { + saveType = inferSaveType(pkg, name) + } + + if (saveType === 'prod') { + // a production dependency can only exist as production (rpj ensures it + // doesn't coexist w/ optional) + deleteSubKey(pkg, 'devDependencies', name, 'dependencies') + deleteSubKey(pkg, 'peerDependencies', name, 'dependencies') + } else if (saveType === 'dev') { + // a dev dependency may co-exist as peer, or optional, but not production + deleteSubKey(pkg, 'dependencies', name, 'devDependencies') + } else if (saveType === 'optional') { + // an optional dependency may co-exist as dev (rpj ensures it doesn't + // coexist w/ prod) + deleteSubKey(pkg, 'peerDependencies', name, 'optionalDependencies') + } else { // peer or peerOptional is all that's left + // a peer dependency may coexist as dev + deleteSubKey(pkg, 'dependencies', name, 'peerDependencies') + deleteSubKey(pkg, 'optionalDependencies', name, 'peerDependencies') + } + + const depType = saveTypeMap.get(saveType) + + pkg[depType] = pkg[depType] || {} + if (rawSpec !== '' || pkg[depType][name] === undefined) { + pkg[depType][name] = rawSpec || '*' + } + if (saveType === 'optional') { + // Affordance for previous npm versions that require this behaviour + pkg.dependencies = pkg.dependencies || {} + pkg.dependencies[name] = pkg.optionalDependencies[name] + } + + if (saveType === 'peer' || saveType === 'peerOptional') { + const pdm = pkg.peerDependenciesMeta || {} + if (saveType === 'peer' && pdm[name] && pdm[name].optional) { + pdm[name].optional = false + } else if (saveType === 'peerOptional') { + pdm[name] = pdm[name] || {} + pdm[name].optional = true + pkg.peerDependenciesMeta = pdm + } + // peerDeps are often also a devDep, so that they can be tested when + // using package managers that don't auto-install peer deps + if (pkg.devDependencies && pkg.devDependencies[name] !== undefined) { + pkg.devDependencies[name] = pkg.peerDependencies[name] + } + } + + if (saveBundle && saveType !== 'peer' && saveType !== 'peerOptional') { + // keep it sorted, keep it unique + const bd = new Set(pkg.bundleDependencies || []) + bd.add(name) + pkg.bundleDependencies = [...bd].sort(localeCompare) + } } return pkg @@ -21,71 +80,6 @@ const saveTypeMap = new Map([ ['peer', 'peerDependencies'], ]) -const addSingle = ({ pkg, spec, saveBundle, saveType }) => { - const { name, rawSpec } = spec - - // if the user does not give us a type, we infer which type(s) - // to keep based on the same order of priority we do when - // building the tree as defined in the _loadDeps method of - // the node class. - if (!saveType) { - saveType = inferSaveType(pkg, spec.name) - } - - if (saveType === 'prod') { - // a production dependency can only exist as production (rpj ensures it - // doesn't coexist w/ optional) - deleteSubKey(pkg, 'devDependencies', name, 'dependencies') - deleteSubKey(pkg, 'peerDependencies', name, 'dependencies') - } else if (saveType === 'dev') { - // a dev dependency may co-exist as peer, or optional, but not production - deleteSubKey(pkg, 'dependencies', name, 'devDependencies') - } else if (saveType === 'optional') { - // an optional dependency may co-exist as dev (rpj ensures it doesn't - // coexist w/ prod) - deleteSubKey(pkg, 'peerDependencies', name, 'optionalDependencies') - } else { // peer or peerOptional is all that's left - // a peer dependency may coexist as dev - deleteSubKey(pkg, 'dependencies', name, 'peerDependencies') - deleteSubKey(pkg, 'optionalDependencies', name, 'peerDependencies') - } - - const depType = saveTypeMap.get(saveType) - - pkg[depType] = pkg[depType] || {} - if (rawSpec !== '' || pkg[depType][name] === undefined) { - pkg[depType][name] = rawSpec || '*' - } - if (saveType === 'optional') { - // Affordance for previous npm versions that require this behaviour - pkg.dependencies = pkg.dependencies || {} - pkg.dependencies[name] = pkg.optionalDependencies[name] - } - - if (saveType === 'peer' || saveType === 'peerOptional') { - const pdm = pkg.peerDependenciesMeta || {} - if (saveType === 'peer' && pdm[name] && pdm[name].optional) { - pdm[name].optional = false - } else if (saveType === 'peerOptional') { - pdm[name] = pdm[name] || {} - pdm[name].optional = true - pkg.peerDependenciesMeta = pdm - } - // peerDeps are often also a devDep, so that they can be tested when - // using package managers that don't auto-install peer deps - if (pkg.devDependencies && pkg.devDependencies[name] !== undefined) { - pkg.devDependencies[name] = pkg.peerDependencies[name] - } - } - - if (saveBundle && saveType !== 'peer' && saveType !== 'peerOptional') { - // keep it sorted, keep it unique - const bd = new Set(pkg.bundleDependencies || []) - bd.add(spec.name) - pkg.bundleDependencies = [...bd].sort(localeCompare) - } -} - // Finds where the package is already in the spec and infers saveType from that const inferSaveType = (pkg, name) => { for (const saveType of saveTypeMap.keys()) { @@ -103,9 +97,8 @@ const inferSaveType = (pkg, name) => { return 'prod' } -const { hasOwnProperty } = Object.prototype const hasSubKey = (pkg, depType, name) => { - return pkg[depType] && hasOwnProperty.call(pkg[depType], name) + return pkg[depType] && Object.prototype.hasOwnProperty.call(pkg[depType], name) } // Removes a subkey and warns about it if it's being replaced diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 31a4e8c821a8c..64d72bedacb1c 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -81,18 +81,11 @@ const _linkNodes = Symbol('linkNodes') const _follow = Symbol('follow') const _globalStyle = Symbol('globalStyle') const _globalRootNode = Symbol('globalRootNode') -const _isVulnerable = Symbol.for('isVulnerable') const _usePackageLock = Symbol.for('usePackageLock') const _rpcache = Symbol.for('realpathCache') const _stcache = Symbol.for('statCache') -const _updateFilePath = Symbol('updateFilePath') -const _followSymlinkPath = Symbol('followSymlinkPath') -const _getRelpathSpec = Symbol('getRelpathSpec') -const _retrieveSpecName = Symbol('retrieveSpecName') const _strictPeerDeps = Symbol('strictPeerDeps') const _checkEngineAndPlatform = Symbol('checkEngineAndPlatform') -const _checkEngine = Symbol('checkEngine') -const _checkPlatform = Symbol('checkPlatform') const _virtualRoots = Symbol('virtualRoots') const _virtualRoot = Symbol('virtualRoot') const _includeWorkspaceRoot = Symbol.for('includeWorkspaceRoot') @@ -228,34 +221,22 @@ module.exports = cls => class IdealTreeBuilder extends cls { } async [_checkEngineAndPlatform] () { + const { engineStrict, npmVersion, nodeVersion } = this.options for (const node of this.idealTree.inventory.values()) { if (!node.optional) { - this[_checkEngine](node) - this[_checkPlatform](node) - } - } - } - - [_checkPlatform] (node) { - checkPlatform(node.package, this[_force]) - } - - [_checkEngine] (node) { - const { engineStrict, npmVersion, nodeVersion } = this.options - const c = () => - checkEngine(node.package, npmVersion, nodeVersion, this[_force]) - - if (engineStrict) { - c() - } else { - try { - c() - } catch (er) { - log.warn(er.code, er.message, { - package: er.pkgid, - required: er.required, - current: er.current, - }) + try { + checkEngine(node.package, npmVersion, nodeVersion, this[_force]) + } catch (err) { + if (engineStrict) { + throw err + } + log.warn(err.code, err.message, { + package: err.pkgid, + required: err.required, + current: err.current, + }) + } + checkPlatform(node.package, this[_force]) } } } @@ -533,82 +514,57 @@ Try using the package name instead, e.g: // This returns a promise because we might not have the name yet, // and need to call pacote.manifest to find the name. async [_add] (tree, { add, saveType = null, saveBundle = false }) { + const path = this.idealTree.target.path // 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 // fetch it and look in its manifest. - const resolvedAdd = await Promise.all(add.map(async rawSpec => { + await Promise.all(add.map(async rawSpec => { // We do NOT provide the path to npa here, because user-additions // need to be resolved relative to the CWD the user is in. - const spec = await this[_retrieveSpecName](npa(rawSpec)) - .then(spec => this[_updateFilePath](spec)) - .then(spec => this[_followSymlinkPath](spec)) + let spec = npa(rawSpec) + + // if it's just @'' then we reload whatever's there, or get latest + // if it's an explicit tag, we need to install that specific tag version + const isTag = spec.rawSpec && spec.type === 'tag' + + // look up the names of file/directory/git specs + if (!spec.name || isTag) { + const mani = await pacote.manifest(spec, { ...this.options }) + if (isTag) { + // translate tag to a version + spec = npa(`${mani.name}@${mani.version}`) + } + spec.name = mani.name + } + + const { name } = spec + if (spec.type === 'file') { + spec = npa(`file:${relpath(path, spec.fetchSpec).replace(/#/g, '%23')}`, path) + spec.name = name + } else if (spec.type === 'directory') { + try { + const real = await realpath(spec.fetchSpec, this[_rpcache], this[_stcache]) + spec = npa(`file:${relpath(path, real).replace(/#/g, '%23')}`, path) + spec.name = name + } catch { + // TODO: create synthetic test case to simulate realpath failure + } + } spec.tree = tree - return spec + this[_resolvedAdd].push(spec) })) - this[_resolvedAdd].push(...resolvedAdd) - // now resolvedAdd is a list of spec objects with names. + + // now this._resolvedAdd is a list of spec objects with names. // find a home for each of them! addRmPkgDeps.add({ pkg: tree.package, - add: resolvedAdd, + add: this[_resolvedAdd], saveBundle, saveType, - path: this.path, }) } - async [_retrieveSpecName] (spec) { - // if it's just @'' then we reload whatever's there, or get latest - // if it's an explicit tag, we need to install that specific tag version - const isTag = spec.rawSpec && spec.type === 'tag' - - if (spec.name && !isTag) { - return spec - } - - const mani = await pacote.manifest(spec, { ...this.options }) - // if it's a tag type, then we need to run it down to an actual version - if (isTag) { - return npa(`${mani.name}@${mani.version}`) - } - - spec.name = mani.name - return spec - } - - async [_updateFilePath] (spec) { - if (spec.type === 'file') { - return this[_getRelpathSpec](spec, spec.fetchSpec) - } - - return spec - } - - async [_followSymlinkPath] (spec) { - if (spec.type === 'directory') { - const real = await ( - realpath(spec.fetchSpec, this[_rpcache], this[_stcache]) - // TODO: create synthetic test case to simulate realpath failure - .catch(/* istanbul ignore next */() => null) - ) - - return this[_getRelpathSpec](spec, real) - } - return spec - } - - [_getRelpathSpec] (spec, filepath) { - /* istanbul ignore else - should also be covered by realpath failure */ - if (filepath) { - const { name } = spec - const tree = this.idealTree.target - spec = npa(`file:${relpath(tree.path, filepath).replace(/#/g, '%23')}`, tree.path) - spec.name = name - } - return spec - } - // TODO: provide a way to fix bundled deps by exposing metadata about // what's in the bundle at each published manifest. Without that, we // can't possibly fix bundled deps without breaking a ton of other stuff, @@ -686,10 +642,6 @@ Try using the package name instead, e.g: } } - [_isVulnerable] (node) { - return this.auditReport && this.auditReport.isVulnerable(node) - } - [_avoidRange] (name) { if (!this.auditReport) { return null @@ -1234,7 +1186,7 @@ This is a one-time fix-up, please be patient... } // fixing a security vulnerability with this package, problem - if (this[_isVulnerable](edge.to)) { + if (this.auditReport && this.auditReport.isVulnerable(edge.to)) { return true } From d30586e67284c4bae0c4fc8639e82ab92ebbcaef Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 24 Aug 2022 07:19:31 -0700 Subject: [PATCH 2/2] fix: create links relative to the target Added link deps need to be relative to the package they're being added to, not the project root. In the past the project root was the only place you could add things but workspaces changed this. --- .../arborist/lib/arborist/build-ideal-tree.js | 17 +- .../arborist/build-ideal-tree.js.test.cjs | 201 ++++++++++++++++++ .../test/arborist/build-ideal-tree.js | 13 ++ 3 files changed, 223 insertions(+), 8 deletions(-) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 64d72bedacb1c..e9a8720d7322d 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -511,17 +511,18 @@ Try using the package name instead, e.g: this[_depsQueue].push(tree) } - // This returns a promise because we might not have the name yet, - // and need to call pacote.manifest to find the name. + // This returns a promise because we might not have the name yet, and need to + // call pacote.manifest to find the name. async [_add] (tree, { add, saveType = null, saveBundle = false }) { - const path = this.idealTree.target.path + // If we have a link it will need to be added relative to the target's path + const path = tree.target.path + // 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 - // fetch it and look in its manifest. + // ie, doing `foo@bar` we just return foo but if it's a url or git, we + // don't know the name until we fetch it and look in its manifest. await Promise.all(add.map(async rawSpec => { - // We do NOT provide the path to npa here, because user-additions - // need to be resolved relative to the CWD the user is in. + // We do NOT provide the path to npa here, because user-additions need to + // be resolved relative to the tree being added to. let spec = npa(rawSpec) // if it's just @'' then we reload whatever's there, or get latest diff --git a/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs index 1b1e2d55da5de..42327a9db1924 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs @@ -1470,6 +1470,207 @@ ArboristNode { } ` +exports[`test/arborist/build-ideal-tree.js TAP add one workspace to another > tree with workspace a added to workspace c 1`] = ` +ArboristNode { + "children": Map { + "a" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "a", + "spec": "file:{CWD}/test/fixtures/workspaces-not-root/packages/a", + "type": "workspace", + }, + EdgeIn { + "from": "packages/c", + "name": "a", + "spec": "file:../a", + "type": "prod", + }, + }, + "isWorkspace": true, + "location": "node_modules/a", + "name": "a", + "path": "{CWD}/test/fixtures/workspaces-not-root/node_modules/a", + "realpath": "{CWD}/test/fixtures/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/fixtures/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/fixtures/workspaces-not-root/packages/b", + "type": "workspace", + }, + }, + "isWorkspace": true, + "location": "node_modules/b", + "name": "b", + "path": "{CWD}/test/fixtures/workspaces-not-root/node_modules/b", + "realpath": "{CWD}/test/fixtures/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/fixtures/workspaces-not-root/packages/c", + "type": "workspace", + }, + }, + "isWorkspace": true, + "location": "node_modules/c", + "name": "c", + "path": "{CWD}/test/fixtures/workspaces-not-root/node_modules/c", + "realpath": "{CWD}/test/fixtures/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/fixtures/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/fixtures/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/fixtures/workspaces-not-root/packages/b", + "to": "node_modules/b", + "type": "workspace", + }, + "c" => EdgeOut { + "name": "c", + "spec": "file:{CWD}/test/fixtures/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 { + "isWorkspace": true, + "location": "packages/a", + "name": "a", + "path": "{CWD}/test/fixtures/workspaces-not-root/packages/a", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + }, + "isWorkspace": true, + "location": "packages/b", + "name": "b", + "path": "{CWD}/test/fixtures/workspaces-not-root/packages/b", + "version": "1.2.3", + }, + ArboristNode { + "edgesOut": Map { + "a" => EdgeOut { + "name": "a", + "spec": "file:../a", + "to": "node_modules/a", + "type": "prod", + }, + "abbrev" => EdgeOut { + "name": "abbrev", + "spec": "*", + "to": "node_modules/abbrev", + "type": "prod", + }, + }, + "isWorkspace": true, + "location": "packages/c", + "name": "c", + "path": "{CWD}/test/fixtures/workspaces-not-root/packages/c", + "version": "1.2.3", + }, + }, + "isProjectRoot": true, + "location": "", + "name": "workspaces-not-root", + "path": "{CWD}/test/fixtures/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 abbrev removed from a and b 1`] = ` ArboristNode { "children": Map { diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 87783086b65c3..0f7c5fecf4fd9 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -2674,6 +2674,19 @@ t.test('add packages to workspaces, not root', async t => { t.matchSnapshot(printTree(rmTree), 'tree with abbrev removed from a and b') }) +t.test('add one workspace to another', async t => { + const path = resolve(__dirname, '../fixtures/workspaces-not-root') + const packageA = resolve(path, 'packages/a') + + const addTree = await buildIdeal(path, { + add: [packageA], + workspaces: ['c'], + }) + const c = addTree.children.get('c').target + t.match(c.edgesOut.get('a'), { spec: 'file:../a' }) + t.matchSnapshot(printTree(addTree), 'tree with workspace a added to workspace c') +}) + t.test('workspace error handling', async t => { const path = t.testdir({ 'package.json': JSON.stringify({