From 5d2624fb31a0cd77b31659f3c5cf1adf92a18a56 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Tue, 22 Jun 2021 19:46:08 -0400 Subject: [PATCH] chore: @npmcli/package-json refactor Removes lib/update-root-package-json.js in favor of using @npmcli/package-json for reading and modifying package.json during reify. Relates to: https://github.com/npm/statusboard/issues/368 PR-URL: https://github.com/npm/arborist/pull/295 Credit: @ruyadorno Close: #295 Reviewed-by: @wraithgar, @nlf --- lib/arborist/reify.js | 23 +- lib/update-root-package-json.js | 95 ------ package-lock.json | 17 + package.json | 1 + tap-snapshots/test/arborist/reify.js.test.cjs | 2 +- test/update-root-package-json.js | 297 ------------------ 6 files changed, 40 insertions(+), 395 deletions(-) delete mode 100644 lib/update-root-package-json.js delete mode 100644 test/update-root-package-json.js diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index 55360538b..f259a69b5 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -15,6 +15,7 @@ const mkdirp = require('mkdirp-infer-owner') const justMkdirp = require('mkdirp') const moveFile = require('@npmcli/move-file') const rimraf = promisify(require('rimraf')) +const PackageJson = require('@npmcli/package-json') const packageContents = require('@npmcli/installed-package-contents') const { checkEngine, checkPlatform } = require('npm-install-checks') @@ -24,7 +25,6 @@ const Diff = require('../diff.js') 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 { saveTypeMap, hasSubKey } = require('../add-rm-pkg-deps.js') @@ -1029,6 +1029,25 @@ module.exports = cls => class Reifier extends cls { const promises = [this[_saveLockFile](saveOpt)] + const updatePackageJson = async (tree) => { + const pkgJson = await PackageJson.load(tree.path) + .catch(() => new PackageJson(tree.path)) + const { + dependencies = {}, + devDependencies = {}, + optionalDependencies = {}, + peerDependencies = {}, + } = tree.package + + pkgJson.update({ + dependencies, + devDependencies, + optionalDependencies, + peerDependencies, + }) + await pkgJson.save() + } + // grab any from explicitRequests that had deps removed for (const { from: tree } of this.explicitRequests) updatedTrees.add(tree) @@ -1036,7 +1055,7 @@ module.exports = cls => class Reifier extends cls { for (const tree of updatedTrees) { // refresh the edges so they have the correct specs tree.package = tree.package - promises.push(updateRootPackageJson(tree)) + promises.push(updatePackageJson(tree)) } await Promise.all(promises) diff --git a/lib/update-root-package-json.js b/lib/update-root-package-json.js deleted file mode 100644 index 57ec41424..000000000 --- a/lib/update-root-package-json.js +++ /dev/null @@ -1,95 +0,0 @@ -const fs = require('fs') -const promisify = require('util').promisify -const readFile = promisify(fs.readFile) -const writeFile = promisify(fs.writeFile) -const {resolve} = require('path') - -const parseJSON = require('json-parse-even-better-errors') - -const depTypes = new Set([ - 'dependencies', - 'optionalDependencies', - 'devDependencies', - 'peerDependencies', -]) - -// sort alphabetically all types of deps for a given package -const orderDeps = (pkg) => { - for (const type of depTypes) { - if (pkg && pkg[type]) { - pkg[type] = Object.keys(pkg[type]) - .sort((a, b) => a.localeCompare(b, 'en')) - .reduce((res, key) => { - res[key] = pkg[type][key] - return res - }, {}) - } - } - return pkg -} -const parseJsonSafe = json => { - try { - return parseJSON(json) - } catch (er) { - return null - } -} - -const updateRootPackageJson = async tree => { - const filename = resolve(tree.path, 'package.json') - const originalJson = await readFile(filename, 'utf8').catch(() => null) - const originalContent = parseJsonSafe(originalJson) - - const depsData = orderDeps({ - ...tree.package, - }) - - // optionalDependencies don't need to be repeated in two places - if (depsData.dependencies) { - if (depsData.optionalDependencies) { - for (const name of Object.keys(depsData.optionalDependencies)) - delete depsData.dependencies[name] - } - if (Object.keys(depsData.dependencies).length === 0) - delete depsData.dependencies - } - - // if there's no package.json, just use internal pkg info as source of truth - // 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, - [Symbol.for('newline')]: newline, - } = tree.package - const format = indent === undefined ? ' ' : indent - const eol = newline === undefined ? '\n' : newline - const content = (JSON.stringify(packageJsonContent, null, format) + '\n') - .replace(/\n/g, eol) - - if (content !== originalJson) - return writeFile(filename, content) -} - -module.exports = updateRootPackageJson diff --git a/package-lock.json b/package-lock.json index 7a353f02e..ed656ecce 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,6 +15,7 @@ "@npmcli/move-file": "^1.1.0", "@npmcli/name-from-folder": "^1.0.1", "@npmcli/node-gyp": "^1.0.1", + "@npmcli/package-json": "^1.0.1", "@npmcli/run-script": "^1.8.2", "bin-links": "^2.2.1", "cacache": "^15.0.3", @@ -653,6 +654,14 @@ "resolved": "https://registry.npmjs.org/@npmcli/node-gyp/-/node-gyp-1.0.2.tgz", "integrity": "sha512-yrJUe6reVMpktcvagumoqD9r08fH1iRo01gn1u0zoCApa9lnZGEigVKUd2hzsCId4gdtkZZIVscLhNxMECKgRg==" }, + "node_modules/@npmcli/package-json": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/@npmcli/package-json/-/package-json-1.0.1.tgz", + "integrity": "sha512-y6jnu76E9C23osz8gEMBayZmaZ69vFOIk8vR1FJL/wbEJ54+9aVG9rLTjQKSXfgYZEr50nw1txBBFfBZZe+bYg==", + "dependencies": { + "json-parse-even-better-errors": "^2.3.1" + } + }, "node_modules/@npmcli/promise-spawn": { "version": "1.3.2", "resolved": "https://registry.npmjs.org/@npmcli/promise-spawn/-/promise-spawn-1.3.2.tgz", @@ -8105,6 +8114,14 @@ "resolved": "https://registry.npmjs.org/@npmcli/node-gyp/-/node-gyp-1.0.2.tgz", "integrity": "sha512-yrJUe6reVMpktcvagumoqD9r08fH1iRo01gn1u0zoCApa9lnZGEigVKUd2hzsCId4gdtkZZIVscLhNxMECKgRg==" }, + "@npmcli/package-json": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/@npmcli/package-json/-/package-json-1.0.1.tgz", + "integrity": "sha512-y6jnu76E9C23osz8gEMBayZmaZ69vFOIk8vR1FJL/wbEJ54+9aVG9rLTjQKSXfgYZEr50nw1txBBFfBZZe+bYg==", + "requires": { + "json-parse-even-better-errors": "^2.3.1" + } + }, "@npmcli/promise-spawn": { "version": "1.3.2", "resolved": "https://registry.npmjs.org/@npmcli/promise-spawn/-/promise-spawn-1.3.2.tgz", diff --git a/package.json b/package.json index bd27e4bbf..4abfe6b36 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "@npmcli/move-file": "^1.1.0", "@npmcli/name-from-folder": "^1.0.1", "@npmcli/node-gyp": "^1.0.1", + "@npmcli/package-json": "^1.0.1", "@npmcli/run-script": "^1.8.2", "bin-links": "^2.2.1", "cacache": "^15.0.3", diff --git a/tap-snapshots/test/arborist/reify.js.test.cjs b/tap-snapshots/test/arborist/reify.js.test.cjs index 5b99e7eec..bb7e5bbc9 100644 --- a/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/tap-snapshots/test/arborist/reify.js.test.cjs @@ -31952,7 +31952,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin ` exports[`test/arborist/reify.js TAP save-prod, with optional > must match snapshot 1`] = ` -{"optionalDependencies":{},"dependencies":{"abbrev":"^1.1.1"}} +{"dependencies":{"abbrev":"^1.1.1"}} ` exports[`test/arborist/reify.js TAP saving the ideal tree save some stuff > lock after save 1`] = ` diff --git a/test/update-root-package-json.js b/test/update-root-package-json.js deleted file mode 100644 index f017928b4..000000000 --- a/test/update-root-package-json.js +++ /dev/null @@ -1,297 +0,0 @@ -const {resolve} = require('path') -const {statSync} = require('fs') - -const t = require('tap') - -const updateRootPackageJson = require('../lib/update-root-package-json.js') - -t.test('missing package.json', async t => { - const path = t.testdir({}) - await updateRootPackageJson({ - path, - package: { - name: 'missing-package-json-test', - version: '1.0.0', - dependencies: { - abbrev: '^1.0.0', - }, - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'missing-package-json-test', - version: '1.0.0', - dependencies: { - abbrev: '^1.0.0', - }, - }, - 'should write new package.json with tree data' - ) -}) - -t.test('invalid package.json', async t => { - const path = t.testdir({ - 'package.json': 'this! is! not! json!', - }) - await updateRootPackageJson({ - path, - package: { - name: 'invalid-package-json-test', - version: '1.0.0', - dependencies: { - abbrev: '^1.0.0', - }, - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'invalid-package-json-test', - version: '1.0.0', - dependencies: { - abbrev: '^1.0.0', - }, - }, - 'should write new package.json with tree data' - ) -}) - -t.test('existing package.json', async t => { - const path = t.testdir({ - 'package.json': JSON.stringify({ - name: 'existing-package-json-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: { - abbrev: '^1.0.0', - }, - }), - }) - await updateRootPackageJson({ - path, - package: { - name: 'missing-package-json-test', - version: '1.0.0', - dependencies: {}, - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'existing-package-json-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: undefined, - }, - 'should write new package.json with tree data' - ) -}) - -t.test('unchanged package.json', async t => { - const path = t.testdir({ - 'package.json': JSON.stringify({ - name: 'existing-package-json-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: { - abbrev: '^1.0.0', - }, - }, null, 2) + '\n', - }) - const { mtime } = statSync(path + '/package.json') - await updateRootPackageJson({ - path, - package: { - name: 'existing-package-json-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: { - abbrev: '^1.0.0', - }, - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'existing-package-json-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: { - abbrev: '^1.0.0', - }, - }, - 'should write new package.json with tree data' - ) - const { mtime: newMtime } = statSync(path + '/package.json') - t.equal(newMtime.toISOString(), mtime.toISOString(), 'mtime not changed') -}) - -t.test('existing package.json with optionalDependencies', async t => { - const path = t.testdir({ - 'package.json': JSON.stringify({ - name: 'existing-package-json-optional-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: {}, - }), - }) - await updateRootPackageJson({ - path, - package: { - name: 'missing-package-json-optional-test', - version: '1.0.0', - dependencies: { - abbrev: '^1.0.0', - }, - optionalDependencies: { - abbrev: '^1.0.0', - }, - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'existing-package-json-optional-test', - version: '1.0.0', - bin: './file.js', - funding: 'http://example.com', - dependencies: undefined, - optionalDependencies: { - abbrev: '^1.0.0', - }, - }, - 'should write new package.json with tree data' - ) -}) - -t.test('custom formatting', async t => { - const path = t.testdir({ - 'package.json': JSON.stringify({ - name: 'custom-formatting-test', - version: '1.0.0', - }), - }) - await updateRootPackageJson({ - path, - package: { - name: 'custom-formatting-test', - version: '1.0.0', - [Symbol.for('indent')]: 4, - [Symbol.for('newline')]: '', - }, - }) - t.match( - require(resolve(path, 'package.json')), - { - name: 'custom-formatting-test', - version: '1.0.0', - }, - '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') -}) - -t.test('deps are alphabetized', async t => { - const path = t.testdir({}) - await updateRootPackageJson({ - path, - package: { - name: 'unordered-deps', - version: '1.0.0', - dependencies: { - b: '1.0.0', - a: '1.0.0', - d: '1.0.0', - c: '1.0.0', - }, - }, - }) - t.strictSame( - require(resolve(path, 'package.json')), - { - name: 'unordered-deps', - version: '1.0.0', - dependencies: { - a: '1.0.0', - b: '1.0.0', - d: '1.0.0', - c: '1.0.0', - }, - }, - 'should write new package.json with ordered deps' - ) -})