From b0997c91083f1ebd1c4e8d4e996b7aa5e4c39ad2 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 17 Feb 2021 12:40:57 -0800 Subject: [PATCH] Prefer peer over prod dep, if specified in both Also, preserve the duplication if we update or modify the package.json file. Fix: https://github.com/npm/rfcs/issues/324 PR-URL: https://github.com/npm/arborist/pull/235 Credit: @isaacs Close: #235 Reviewed-by: @nlf --- lib/node.js | 7 ++- lib/update-root-package-json.js | 19 +++++- tap-snapshots/test-node.js-TAP.test.js | 60 +++++++++--------- test/node.js | 87 ++++++++++++++++++++++++++ test/update-root-package-json.js | 80 +++++++++++++++++++++-- 5 files changed, 214 insertions(+), 39 deletions(-) diff --git a/lib/node.js b/lib/node.js index 9a6b86e40..fa39bed5e 100644 --- a/lib/node.js +++ b/lib/node.js @@ -731,7 +731,6 @@ class Node { // Note the subtle breaking change from v6: it is no longer possible // to have a different spec for a devDep than production dep. this[_loadDepType](this.package.optionalDependencies, 'optional') - this[_loadDepType](this.package.dependencies, 'prod') // Linked targets that are disconnected from the tree are tops, // but don't have a 'path' field, only a 'realpath', because we @@ -755,6 +754,8 @@ class Node { this[_loadDepType](peerDependencies, 'peer') this[_loadDepType](peerOptional, 'peerOptional') } + + this[_loadDepType](this.package.dependencies, 'prod') } [_loadDepType] (obj, type) { @@ -763,8 +764,10 @@ class Node { for (const [name, spec] of Object.entries(obj || {})) { const accept = ad[name] // if it's already set, then we keep the existing edge + // Prod deps should not be marked as dev, however. // NB: the Edge ctor adds itself to from.edgesOut - if (!this.edgesOut.get(name)) + const current = this.edgesOut.get(name) + if (!current || current.dev && type === 'prod') new Edge({ from, name, spec, accept, type }) } } diff --git a/lib/update-root-package-json.js b/lib/update-root-package-json.js index 56965e26d..aba561492 100644 --- a/lib/update-root-package-json.js +++ b/lib/update-root-package-json.js @@ -43,12 +43,29 @@ const updateRootPackageJson = async tree => { } // if there's no package.json, just use internal pkg info as source of truth - const packageJsonContent = originalContent || depsData + // clone the object though, so we can still refer to what it originally was + const packageJsonContent = !originalContent ? depsData + : Object.assign({}, originalContent) // loop through all types of dependencies and update package json content for (const type of depTypes) packageJsonContent[type] = depsData[type] + // if original package.json had dep in peerDeps AND deps, preserve that. + const { dependencies: origProd, peerDependencies: origPeer } = + originalContent || {} + const { peerDependencies: newPeer } = packageJsonContent + if (origProd && origPeer && newPeer) { + // we have original prod/peer deps, and new peer deps + // copy over any that were in both in the original + for (const name of Object.keys(origPeer)) { + if (origProd[name] !== undefined && newPeer[name] !== undefined) { + packageJsonContent.dependencies = packageJsonContent.dependencies || {} + packageJsonContent.dependencies[name] = newPeer[name] + } + } + } + // format content const { [Symbol.for('indent')]: indent, diff --git a/tap-snapshots/test-node.js-TAP.test.js b/tap-snapshots/test-node.js-TAP.test.js index 57fd780f9..37953a8f9 100644 --- a/tap-snapshots/test-node.js-TAP.test.js +++ b/tap-snapshots/test-node.js-TAP.test.js @@ -670,8 +670,8 @@ exports[`test/node.js TAP testing with dep tree with meta > add new meta under p Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -917,10 +917,10 @@ exports[`test/node.js TAP testing with dep tree with meta > add new meta under p "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -993,8 +993,8 @@ exports[`test/node.js TAP testing with dep tree with meta > add new meta under p Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -1421,8 +1421,8 @@ exports[`test/node.js TAP testing with dep tree with meta > initial load with so Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -1615,10 +1615,10 @@ exports[`test/node.js TAP testing with dep tree with meta > initial load with so "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -1667,8 +1667,8 @@ exports[`test/node.js TAP testing with dep tree with meta > initial load with so Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -1964,8 +1964,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move meta to top lev Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -2189,10 +2189,10 @@ exports[`test/node.js TAP testing with dep tree with meta > move meta to top lev "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -2210,8 +2210,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move meta to top lev Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -2508,8 +2508,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -2780,10 +2780,10 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -2801,8 +2801,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -3169,8 +3169,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -3441,10 +3441,10 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -3462,8 +3462,8 @@ exports[`test/node.js TAP testing with dep tree with meta > move new meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -3885,8 +3885,8 @@ exports[`test/node.js TAP testing with dep tree without meta > add new meta unde Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -4132,10 +4132,10 @@ exports[`test/node.js TAP testing with dep tree without meta > add new meta unde "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -4208,8 +4208,8 @@ exports[`test/node.js TAP testing with dep tree without meta > add new meta unde Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -4636,8 +4636,8 @@ exports[`test/node.js TAP testing with dep tree without meta > initial load with Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -4830,10 +4830,10 @@ exports[`test/node.js TAP testing with dep tree without meta > initial load with "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -4882,8 +4882,8 @@ exports[`test/node.js TAP testing with dep tree without meta > initial load with Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -5179,8 +5179,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -5404,10 +5404,10 @@ exports[`test/node.js TAP testing with dep tree without meta > move meta to top "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -5425,8 +5425,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move meta to top Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -5723,8 +5723,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -5995,10 +5995,10 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -6016,8 +6016,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -6384,8 +6384,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -6656,10 +6656,10 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to "optional" => Edge {}, "overlap" => Edge {}, "optMissing" => Edge {}, + "dev" => Edge {}, "prod" => Edge {}, "bundled" => Edge {}, "missing" => Edge {}, - "dev" => Edge {}, }, "errors": Array [], "extraneous": true, @@ -6677,8 +6677,8 @@ exports[`test/node.js TAP testing with dep tree without meta > move new meta to Edge {}, }, "edgesOut": Map { - "meta" => Edge {}, "peer" => Edge {}, + "meta" => Edge {}, }, "errors": Array [], "extraneous": true, diff --git a/test/node.js b/test/node.js index 69edab7f2..57cf46f04 100644 --- a/test/node.js +++ b/test/node.js @@ -2181,3 +2181,90 @@ t.test('globaTop set for children of global link root target', async t => { }) t.equal(gtop.globalTop, true) }) + +t.test('duplicated dependencies', t => { + // the specific logic here is justifiable at all steps, but gets weird + // in the "specified in all three" case, even though that's the logical + // outcome of the other rules. at least we have a test showing what + // actually happens. + + t.test('prefer peer over prod', async t => { + const n = new Node({ + path: '/path/to/project', + pkg: { + dependencies: { + foo: '1.x', + }, + peerDependencies: { + foo: '>=1', + }, + }, + }) + t.match(n.edgesOut.get('foo'), { type: 'peer', spec: '>=1' }) + }) + + t.test('prefer dev over peer', async t => { + const n = new Node({ + path: '/path/to/project', + pkg: { + devDependencies: { + foo: '1.x', + }, + peerDependencies: { + foo: '>=1', + }, + }, + }) + t.match(n.edgesOut.get('foo'), { type: 'dev', spec: '1.x' }) + }) + + t.test('prefer prod over dev', async t => { + const n = new Node({ + path: '/path/to/project', + pkg: { + devDependencies: { + foo: '1.x', + }, + dependencies: { + foo: '>=1', + }, + }, + }) + t.match(n.edgesOut.get('foo'), { type: 'prod', spec: '>=1' }) + }) + + t.test('prefer prod over dev', async t => { + const n = new Node({ + path: '/path/to/project', + pkg: { + devDependencies: { + foo: '1.x', + }, + dependencies: { + foo: '>=1', + }, + }, + }) + t.match(n.edgesOut.get('foo'), { type: 'prod', spec: '>=1' }) + }) + + t.test('if in all three, use prod', async t => { + const n = new Node({ + path: '/path/to/project', + pkg: { + devDependencies: { + foo: '1.x', + }, + dependencies: { + foo: '2', + }, + peerDependencies: { + foo: '>=1', + }, + }, + }) + t.match(n.edgesOut.get('foo'), { type: 'prod', spec: '2' }) + }) + + t.end() +}) diff --git a/test/update-root-package-json.js b/test/update-root-package-json.js index be18d0c8a..cadacdfa8 100644 --- a/test/update-root-package-json.js +++ b/test/update-root-package-json.js @@ -8,7 +8,7 @@ const updateRootPackageJson = require('../lib/update-root-package-json.js') t.test('missing package.json', async t => { const path = t.testdir({}) await updateRootPackageJson({ - path: path, + path, package: { name: 'missing-package-json-test', version: '1.0.0', @@ -35,7 +35,7 @@ t.test('invalid package.json', async t => { 'package.json': 'this! is! not! json!', }) await updateRootPackageJson({ - path: path, + path, package: { name: 'invalid-package-json-test', version: '1.0.0', @@ -70,7 +70,7 @@ t.test('existing package.json', async t => { }), }) await updateRootPackageJson({ - path: path, + path, package: { name: 'missing-package-json-test', version: '1.0.0', @@ -104,7 +104,7 @@ t.test('unchanged package.json', async t => { }) const { mtime } = statSync(path + '/package.json') await updateRootPackageJson({ - path: path, + path, package: { name: 'existing-package-json-test', version: '1.0.0', @@ -143,7 +143,7 @@ t.test('existing package.json with optionalDependencies', async t => { }), }) await updateRootPackageJson({ - path: path, + path, package: { name: 'missing-package-json-optional-test', version: '1.0.0', @@ -179,7 +179,7 @@ t.test('custom formatting', async t => { }), }) await updateRootPackageJson({ - path: path, + path, package: { name: 'custom-formatting-test', version: '1.0.0', @@ -196,3 +196,71 @@ t.test('custom formatting', async t => { 'should write new package.json with tree data' ) }) + +t.test('preserve deps duplicated in peer and prod', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'duplicated-peer', + version: '1.0.0', + dependencies: { + foo: '1.2.3', // being weirdly tricky + }, + peerDependencies: { + foo: '1.x', + }, + }), + }) + await updateRootPackageJson({ + path, + package: { + name: 'duplicated-peer', + version: '1.0.0', + peerDependencies: { + foo: '2.x', + }, + }, + }) + t.match(require(path + '/package.json'), { + name: 'duplicated-peer', + version: '1.0.0', + peerDependencies: { + foo: '2.x', // now they match. + }, + dependencies: { + foo: '2.x', + }, + }, 'peer/prod duplication preserved') +}) + +t.test('remove peer/prod dupes from both if removed from peer', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'duplicated-peer', + version: '1.0.0', + dependencies: { + foo: '1.2.3', // being weirdly tricky + }, + peerDependencies: { + foo: '1.x', + }, + }), + }) + await updateRootPackageJson({ + path, + package: { + name: 'duplicated-peer', + version: '1.0.0', + peerDependencies: { + bar: '1.x', + }, + }, + }) + t.strictSame(require(path + '/package.json'), { + name: 'duplicated-peer', + version: '1.0.0', + // no dupe + peerDependencies: { + bar: '1.x', + }, + }, 'peer/prod duplication preserved') +})