From 3acaf77e8ddabba8efc6e0c6d41e33033d80ec74 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Tue, 22 Jun 2021 19:56:19 -0400 Subject: [PATCH] chore: @npmcli/package-json refactor Refactor set-script and init to use @npmcli/package-json as a uniformed way to update and save package.json files. Fixes: https://github.com/npm/cli/issues/3234 Relates to: https://github.com/npm/statusboard/issues/368 --- lib/init.js | 37 ++---- lib/set-script.js | 50 ++++----- node_modules/@npmcli/package-json/LICENSE | 18 +++ .../@npmcli/package-json/lib/index.js | 106 ++++++++++++++++++ .../package-json/lib/update-dependencies.js | 72 ++++++++++++ .../package-json/lib/update-scripts.js | 28 +++++ .../package-json/lib/update-workspaces.js | 25 +++++ .../@npmcli/package-json/package.json | 34 ++++++ package-lock.json | 17 +++ package.json | 1 + test/lib/init.js | 30 ++--- test/lib/set-script.js | 25 +---- 12 files changed, 341 insertions(+), 102 deletions(-) create mode 100644 node_modules/@npmcli/package-json/LICENSE create mode 100644 node_modules/@npmcli/package-json/lib/index.js create mode 100644 node_modules/@npmcli/package-json/lib/update-dependencies.js create mode 100644 node_modules/@npmcli/package-json/lib/update-scripts.js create mode 100644 node_modules/@npmcli/package-json/lib/update-workspaces.js create mode 100644 node_modules/@npmcli/package-json/package.json diff --git a/lib/init.js b/lib/init.js index d34f92b882b32..e4bd20b7210e8 100644 --- a/lib/init.js +++ b/lib/init.js @@ -5,8 +5,8 @@ const initJson = require('init-package-json') const npa = require('npm-package-arg') const rpj = require('read-package-json-fast') const libexec = require('libnpmexec') -const parseJSON = require('json-parse-even-better-errors') const mapWorkspaces = require('@npmcli/map-workspaces') +const PackageJson = require('@npmcli/package-json') const getLocationMsg = require('./exec/get-workspace-location-msg.js') const BaseCommand = require('./base-command.js') @@ -199,35 +199,16 @@ class Init extends BaseCommand { return } - let manifest - try { - manifest = - fs.readFileSync(resolve(this.npm.localPrefix, 'package.json'), 'utf-8') - } catch (error) { - throw new Error('package.json not found') - } - - try { - manifest = parseJSON(manifest) - } catch (error) { - throw new Error(`Invalid package.json: ${error}`) - } + const pkgJson = await PackageJson.load(this.npm.localPrefix) - if (!manifest.workspaces) - manifest.workspaces = [] - - manifest.workspaces.push(relative(this.npm.localPrefix, workspacePath)) - - // format content - const { - [Symbol.for('indent')]: indent, - [Symbol.for('newline')]: newline, - } = manifest - - const content = (JSON.stringify(manifest, null, indent) + '\n') - .replace(/\n/g, newline) + pkgJson.update({ + workspaces: [ + ...(pkgJson.content.workspaces || []), + relative(this.npm.localPrefix, workspacePath), + ], + }) - fs.writeFileSync(resolve(this.npm.localPrefix, 'package.json'), content) + await pkgJson.save() } } diff --git a/lib/set-script.js b/lib/set-script.js index cd01e28b56b06..24e4d8f20f666 100644 --- a/lib/set-script.js +++ b/lib/set-script.js @@ -1,8 +1,7 @@ +const { resolve } = require('path') const log = require('npmlog') -const fs = require('fs') -const parseJSON = require('json-parse-even-better-errors') const rpj = require('read-package-json-fast') -const { resolve } = require('path') +const PackageJson = require('@npmcli/package-json') const BaseCommand = require('./base-command.js') class SetScript extends BaseCommand { @@ -51,7 +50,7 @@ class SetScript extends BaseCommand { async setScript (args) { this.validate(args) - const warn = this.doSetScript(this.npm.localPrefix, args[0], args[1]) + const warn = await this.doSetScript(this.npm.localPrefix, args[0], args[1]) if (warn) log.warn('set-script', `Script "${args[0]}" was overwritten`) } @@ -66,7 +65,7 @@ class SetScript extends BaseCommand { for (const [name, path] of this.workspaces) { try { - const warn = this.doSetScript(path, args[0], args[1]) + const warn = await this.doSetScript(path, args[0], args[1]) if (warn) { log.warn('set-script', `Script "${args[0]}" was overwritten`) log.warn(` in workspace: ${name}`) @@ -84,39 +83,28 @@ class SetScript extends BaseCommand { // returns a Boolean that will be true if // the requested script was overwritten // and false if it was set as a new script - doSetScript (path, name, value) { - // Set the script - let manifest + async doSetScript (path, name, value) { let warn = false - try { - manifest = fs.readFileSync(resolve(path, 'package.json'), 'utf-8') - } catch (error) { - throw new Error('package.json not found') - } - - try { - manifest = parseJSON(manifest) - } catch (error) { - throw new Error(`Invalid package.json: ${error}`) - } + const pkgJson = await PackageJson.load(path) + const { scripts } = pkgJson.content - if (!manifest.scripts) - manifest.scripts = {} + const overwriting = + scripts + && scripts[name] + && scripts[name] !== value - if (manifest.scripts[name] && manifest.scripts[name] !== value) + if (overwriting) warn = true - manifest.scripts[name] = value - // format content - const { - [Symbol.for('indent')]: indent, - [Symbol.for('newline')]: newline, - } = manifest + pkgJson.update({ + scripts: { + ...scripts, + [name]: value, + }, + }) - const content = (JSON.stringify(manifest, null, indent) + '\n') - .replace(/\n/g, newline) - fs.writeFileSync(resolve(path, 'package.json'), content) + await pkgJson.save() return warn } diff --git a/node_modules/@npmcli/package-json/LICENSE b/node_modules/@npmcli/package-json/LICENSE new file mode 100644 index 0000000000000..6a1f3708f6d70 --- /dev/null +++ b/node_modules/@npmcli/package-json/LICENSE @@ -0,0 +1,18 @@ +ISC License + +Copyright GitHub Inc. + +Permission to use, copy, modify, and/or distribute this +software for any purpose with or without fee is hereby +granted, provided that the above copyright notice and this +permission notice appear in all copies. + +THE SOFTWARE IS PROVIDED "AS IS" AND NPM DISCLAIMS ALL +WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO +EVENT SHALL NPM BE LIABLE FOR ANY SPECIAL, DIRECT, +INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, +WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER +TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE +USE OR PERFORMANCE OF THIS SOFTWARE. diff --git a/node_modules/@npmcli/package-json/lib/index.js b/node_modules/@npmcli/package-json/lib/index.js new file mode 100644 index 0000000000000..87c3a63093688 --- /dev/null +++ b/node_modules/@npmcli/package-json/lib/index.js @@ -0,0 +1,106 @@ +const fs = require('fs') +const promisify = require('util').promisify +const readFile = promisify(fs.readFile) +const writeFile = promisify(fs.writeFile) +const { resolve } = require('path') +const updateDeps = require('./update-dependencies.js') +const updateScripts = require('./update-scripts.js') +const updateWorkspaces = require('./update-workspaces.js') + +const parseJSON = require('json-parse-even-better-errors') + +const _filename = Symbol('filename') +const _manifest = Symbol('manifest') +const _readFileContent = Symbol('readFileContent') + +// a list of handy specialized helper functions that take +// care of special cases that are handled by the npm cli +const knownSteps = new Set([ + updateDeps, + updateScripts, + updateWorkspaces, +]) + +// list of all keys that are handled by "knownSteps" helpers +const knownKeys = new Set([ + ...updateDeps.knownKeys, + 'scripts', + 'workspaces', +]) + +class PackageJson { + static async load (path) { + return await new PackageJson(path).load() + } + + constructor (path) { + this[_filename] = resolve(path, 'package.json') + this[_manifest] = {} + this[_readFileContent] = '' + } + + async load () { + try { + this[_readFileContent] = + await readFile(this[_filename], 'utf8') + } catch (err) { + throw new Error('package.json not found') + } + + try { + this[_manifest] = + parseJSON(this[_readFileContent]) + } catch (err) { + throw new Error(`Invalid package.json: ${err}`) + } + + return this + } + + get content () { + return this[_manifest] + } + + update (content) { + // validates both current manifest and content param + const invalidContent = + typeof this[_manifest] !== 'object' + || typeof content !== 'object' + if (invalidContent) { + throw Object.assign( + new Error(`Can't update invalid package.json data`), + { code: 'EPACKAGEJSONUPDATE' } + ) + } + + for (const step of knownSteps) + this[_manifest] = step({ content, originalContent: this[_manifest] }) + + // unknown properties will just be overwitten + for (const [key, value] of Object.entries(content)) { + if (!knownKeys.has(key)) + this[_manifest][key] = value + } + + return this + } + + async save () { + const { + [Symbol.for('indent')]: indent, + [Symbol.for('newline')]: newline, + } = this[_manifest] + + const format = indent === undefined ? ' ' : indent + const eol = newline === undefined ? '\n' : newline + const fileContent = `${ + JSON.stringify(this[_manifest], null, format) + }\n` + .replace(/\n/g, eol) + + if (fileContent.trim() !== this[_readFileContent].trim()) + return await writeFile(this[_filename], fileContent) + } +} + +module.exports = PackageJson diff --git a/node_modules/@npmcli/package-json/lib/update-dependencies.js b/node_modules/@npmcli/package-json/lib/update-dependencies.js new file mode 100644 index 0000000000000..dac45a8bed7bf --- /dev/null +++ b/node_modules/@npmcli/package-json/lib/update-dependencies.js @@ -0,0 +1,72 @@ +const depTypes = new Set([ + 'dependencies', + 'optionalDependencies', + 'devDependencies', + 'peerDependencies', +]) + +// sort alphabetically all types of deps for a given package +const orderDeps = (content) => { + for (const type of depTypes) { + if (content && content[type]) { + content[type] = Object.keys(content[type]) + .sort((a, b) => a.localeCompare(b, 'en')) + .reduce((res, key) => { + res[key] = content[type][key] + return res + }, {}) + } + } + return content +} + +const updateDependencies = ({ content, originalContent }) => { + const pkg = orderDeps({ + ...content, + }) + + // optionalDependencies don't need to be repeated in two places + if (pkg.dependencies) { + if (pkg.optionalDependencies) { + for (const name of Object.keys(pkg.optionalDependencies)) + delete pkg.dependencies[name] + } + } + + const result = { ...originalContent } + + // loop through all types of dependencies and update package json pkg + for (const type of depTypes) { + if (pkg[type]) + result[type] = pkg[type] + + // prune empty type props from resulting object + const emptyDepType = + pkg[type] + && typeof pkg === 'object' + && Object.keys(pkg[type]).length === 0 + if (emptyDepType) + delete result[type] + } + + // if original package.json had dep in peerDeps AND deps, preserve that. + const { dependencies: origProd, peerDependencies: origPeer } = + originalContent || {} + const { peerDependencies: newPeer } = result + 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) { + result.dependencies = result.dependencies || {} + result.dependencies[name] = newPeer[name] + } + } + } + + return result +} + +updateDependencies.knownKeys = depTypes + +module.exports = updateDependencies diff --git a/node_modules/@npmcli/package-json/lib/update-scripts.js b/node_modules/@npmcli/package-json/lib/update-scripts.js new file mode 100644 index 0000000000000..3a88d3e9a17a8 --- /dev/null +++ b/node_modules/@npmcli/package-json/lib/update-scripts.js @@ -0,0 +1,28 @@ +const updateScripts = ({ content, originalContent = {} }) => { + const newScripts = content.scripts + + if (!newScripts) + return originalContent + + // validate scripts content being appended + const hasInvalidScripts = () => + Object.entries(newScripts) + .some(([key, value]) => + typeof key !== 'string' || typeof value !== 'string') + if (hasInvalidScripts()) { + throw Object.assign( + new TypeError( + 'package.json scripts should be a key-value pair of strings.'), + { code: 'ESCRIPTSINVALID' } + ) + } + + return { + ...originalContent, + scripts: { + ...newScripts, + }, + } +} + +module.exports = updateScripts diff --git a/node_modules/@npmcli/package-json/lib/update-workspaces.js b/node_modules/@npmcli/package-json/lib/update-workspaces.js new file mode 100644 index 0000000000000..207dd94a236d7 --- /dev/null +++ b/node_modules/@npmcli/package-json/lib/update-workspaces.js @@ -0,0 +1,25 @@ +const updateWorkspaces = ({ content, originalContent = {} }) => { + const newWorkspaces = content.workspaces + + if (!newWorkspaces) + return originalContent + + // validate workspaces content being appended + const hasInvalidWorkspaces = () => + newWorkspaces.some(w => !(typeof w === 'string')) + if (!newWorkspaces.length || hasInvalidWorkspaces()) { + throw Object.assign( + new TypeError('workspaces should be an array of strings.'), + { code: 'EWORKSPACESINVALID' } + ) + } + + return { + ...originalContent, + workspaces: [ + ...newWorkspaces, + ], + } +} + +module.exports = updateWorkspaces diff --git a/node_modules/@npmcli/package-json/package.json b/node_modules/@npmcli/package-json/package.json new file mode 100644 index 0000000000000..8708ec5eb6fb1 --- /dev/null +++ b/node_modules/@npmcli/package-json/package.json @@ -0,0 +1,34 @@ +{ + "name": "@npmcli/package-json", + "version": "1.0.1", + "description": "Programmatic API to update package.json", + "main": "lib/index.js", + "files": [ + "lib" + ], + "scripts": { + "preversion": "npm test", + "postversion": "npm publish", + "prepublishOnly": "git push origin --follow-tags", + "snap": "tap", + "test": "tap", + "npmclilint": "npmcli-lint", + "lint": "npm run npmclilint -- \"lib/*.*js\" \"test/*.*js\"", + "lintfix": "npm run lint -- --fix", + "posttest": "npm run lint --", + "postsnap": "npm run lintfix --" + }, + "keywords": [ + "npm", + "oss" + ], + "author": "GitHub Inc.", + "license": "ISC", + "devDependencies": { + "@npmcli/lint": "^1.0.1", + "tap": "^15.0.9" + }, + "dependencies": { + "json-parse-even-better-errors": "^2.3.1" + } +} diff --git a/package-lock.json b/package-lock.json index f54f60b5c60a2..43d402a99a24f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -85,6 +85,7 @@ "@npmcli/arborist": "^2.6.3", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", + "@npmcli/package-json": "^1.0.1", "@npmcli/run-script": "^1.8.5", "abbrev": "~1.1.1", "ansicolors": "~0.3.2", @@ -890,6 +891,14 @@ "integrity": "sha512-yrJUe6reVMpktcvagumoqD9r08fH1iRo01gn1u0zoCApa9lnZGEigVKUd2hzsCId4gdtkZZIVscLhNxMECKgRg==", "inBundle": true }, + "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", @@ -10980,6 +10989,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 58ac5d7f8c6c8..9ae533a4ded12 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "@npmcli/arborist": "^2.6.3", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", + "@npmcli/package-json": "^1.0.1", "@npmcli/run-script": "^1.8.5", "abbrev": "~1.1.1", "ansicolors": "~0.3.2", diff --git a/test/lib/init.js b/test/lib/init.js index 44a2af5bcc02b..48bd33d2e133c 100644 --- a/test/lib/init.js +++ b/test/lib/init.js @@ -431,8 +431,10 @@ t.test('workspaces', t => { const Init = t.mock('../../lib/init.js', { ...mocks, - 'json-parse-even-better-errors': () => { - throw new Error('ERR') + '@npmcli/package-json': { + async load () { + throw new Error('ERR') + }, }, }) const init = new Init(npm) @@ -440,7 +442,7 @@ t.test('workspaces', t => { init.execWorkspaces([], ['a'], err => { t.match( err, - /Invalid package.json: Error: ERR/, + /ERR/, 'should exit with error' ) t.end() @@ -452,30 +454,16 @@ t.test('workspaces', t => { // this avoids poluting test output with those logs console.log = noop - npm.localPrefix = t.testdir({ - 'package.json': JSON.stringify({ - name: 'top-level', - }), - }) + npm.localPrefix = t.testdir({}) - const Init = t.mock('../../lib/init.js', { - ...mocks, - fs: { - statSync () { - return true - }, - readFileSync () { - throw new Error('ERR') - }, - }, - }) + const Init = require('../../lib/init.js') const init = new Init(npm) init.execWorkspaces([], ['a'], err => { t.match( err, - /package.json not found/, - 'should exit with error' + { code: 'ENOENT' }, + 'should exit with missing package.json file error' ) t.end() }) diff --git a/test/lib/set-script.js b/test/lib/set-script.js index 96b455785c509..6df2b57e44cd5 100644 --- a/test/lib/set-script.js +++ b/test/lib/set-script.js @@ -121,25 +121,6 @@ t.test('warns when overwriting', (t) => { }) }) -t.test('provided indentation and eol is used', (t) => { - npm.localPrefix = t.testdir({ - 'package.json': JSON.stringify({ - name: 'foo', - }, null, ' '.repeat(6)).replace(/\n/g, '\r\n'), - }) - - t.plan(3) - setScript.exec(['arg1', 'arg2'], (error) => { - t.equal(error, undefined) - // rather than checking every line's content - // we parse the result and verify the symbols match - const contents = fs.readFileSync(resolve(npm.localPrefix, 'package.json')) - const data = parseJSON(contents) - t.equal(data[Symbol.for('indent')], ' '.repeat(6), 'keeps indenting') - t.equal(data[Symbol.for('newline')], '\r\n', 'keeps newlines') - }) -}) - t.test('workspaces', (t) => { ERROR_OUTPUT.length = 0 WARN_OUTPUT.length = 0 @@ -153,7 +134,7 @@ t.test('workspaces', (t) => { 'package.json': '{}', }, 'workspace-b': { - 'package.json': '"notjson"', + 'package.json': '"notajsonobject"', }, 'workspace-c': { 'package.json': JSON.stringify({ @@ -176,8 +157,8 @@ t.test('workspaces', (t) => { t.hasStrict(dataA, { scripts: { arg1: 'arg2' } }, 'defined the script') // workspace-b logged an error - t.match(ERROR_OUTPUT, [ - ['set-script', `Cannot create property 'scripts' on string 'notjson'`], + t.strictSame(ERROR_OUTPUT, [ + ['set-script', `Can't update invalid package.json data`], [' in workspace: workspace-b'], [` at location: ${resolve(npm.localPrefix, 'workspace-b')}`], ], 'logged workspace-b error')