From cbb92b120e16a1b6ba44431a906399434f42cfdb Mon Sep 17 00:00:00 2001 From: Gar Date: Mon, 8 May 2023 18:27:13 -0700 Subject: [PATCH 1/2] deps: add new dependency npm-normalize-package-bin@3.0.1 --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index d38757d..3d0711e 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,8 @@ "tap": "^16.0.1" }, "dependencies": { - "json-parse-even-better-errors": "^3.0.0" + "json-parse-even-better-errors": "^3.0.0", + "npm-normalize-package-bin": "^3.0.1" }, "repository": { "type": "git", From 4bba34373b0abe9eee423b70a13115a09534d7e1 Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 9 May 2023 14:05:57 -0700 Subject: [PATCH 2/2] feat: add normalize function This brings in parity with `read-package-json-fast`, which is a normalization method intended for parsing package.json files inside of an existing node_modules. Tests were copied straight from that package to ensure functionality was compatible. The only tests that weren't copied were the ones testing down into the bin normalization. That is delegated to a subdependency so these tests only ensure that *some* normalization is happening. Eventually as we consolidate our package.json reading libs, bin normalization can live in this package and be tested here. Finally, the errors that this package was throwing now include the metadata from the original errors (such as code) instead of making new errors with no context. --- README.md | 14 +++ lib/index.js | 62 +++++---- lib/normalize.js | 68 ++++++++++ test/index.js | 6 +- test/normalize.js | 313 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 436 insertions(+), 27 deletions(-) create mode 100644 lib/normalize.js create mode 100644 test/normalize.js diff --git a/README.md b/README.md index 02dc4f5..c4e9f2d 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,20 @@ const pkgJson = await PackageJson.load('./') --- +### `async PackageJson.normalize()` + +Like `load` but intended for reading package.json files in a +node_modules tree. Some light normalization is done to ensure that it +is ready for use in `@npmcli/arborist` + +--- + +### **static** `async PackageJson.normalize(path)` + +Convenience static method like `load` but for calling `normalize` + +--- + ### `PackageJson.update(content)` Updates the contents of the `package.json` with the `content` provided. diff --git a/lib/index.js b/lib/index.js index e98308f..d48244b 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,18 +1,12 @@ -const fs = require('fs') -const promisify = require('util').promisify -const readFile = promisify(fs.readFile) -const writeFile = promisify(fs.writeFile) +const { readFile, writeFile } = require('fs/promises') const { resolve } = require('path') const updateDeps = require('./update-dependencies.js') const updateScripts = require('./update-scripts.js') const updateWorkspaces = require('./update-workspaces.js') +const normalize = require('./normalize.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([ @@ -29,42 +23,54 @@ const knownKeys = new Set([ ]) class PackageJson { + // default behavior, used when intentionally manipulating a package.json file static async load (path) { return await new PackageJson(path).load() } + // read-package-json-fast compatible behavior + static async normalize (path) { + return await new PackageJson(path).normalize() + } + + #filename + #path + #manifest = {} + #readFileContent = '' + constructor (path) { - this[_filename] = resolve(path, 'package.json') - this[_manifest] = {} - this[_readFileContent] = '' + this.#path = path + this.#filename = resolve(path, 'package.json') } async load () { try { - this[_readFileContent] = - await readFile(this[_filename], 'utf8') + this.#readFileContent = + await readFile(this.#filename, 'utf8') } catch (err) { - throw new Error('package.json not found') + err.message = `Could not read package.json: ${err}` + throw err } try { - this[_manifest] = - parseJSON(this[_readFileContent]) + this.#manifest = + parseJSON(this.#readFileContent) } catch (err) { - throw new Error(`Invalid package.json: ${err}`) + err.message = `Invalid package.json: ${err}` + throw err } return this } get content () { - return this[_manifest] + return this.#manifest } update (content) { // validates both current manifest and content param const invalidContent = - typeof this[_manifest] !== 'object' + typeof this.#manifest !== 'object' || typeof content !== 'object' if (invalidContent) { throw Object.assign( @@ -74,13 +80,13 @@ class PackageJson { } for (const step of knownSteps) { - this[_manifest] = step({ content, originalContent: this[_manifest] }) + 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 + this.#manifest[key] = value } } @@ -91,19 +97,25 @@ class PackageJson { const { [Symbol.for('indent')]: indent, [Symbol.for('newline')]: newline, - } = this[_manifest] + } = this.#manifest const format = indent === undefined ? ' ' : indent const eol = newline === undefined ? '\n' : newline const fileContent = `${ - JSON.stringify(this[_manifest], null, format) + JSON.stringify(this.#manifest, null, format) }\n` .replace(/\n/g, eol) - if (fileContent.trim() !== this[_readFileContent].trim()) { - return await writeFile(this[_filename], fileContent) + if (fileContent.trim() !== this.#readFileContent.trim()) { + return await writeFile(this.#filename, fileContent) } } + + async normalize () { + await this.load() + await normalize(this) + return this + } } module.exports = PackageJson diff --git a/lib/normalize.js b/lib/normalize.js new file mode 100644 index 0000000..85d5c8b --- /dev/null +++ b/lib/normalize.js @@ -0,0 +1,68 @@ +const normalizePackageBin = require('npm-normalize-package-bin') + +const normalize = async (pkg) => { + const data = pkg.content + + // remove _attributes + for (const key in data) { + if (key.startsWith('_')) { + delete pkg.content[key] + } + } + + // _id + if (data.name && data.version) { + data._id = `${data.name}@${data.version}` + } + + // bundleDependencies + if (data.bundleDependencies === undefined && data.bundledDependencies !== undefined) { + data.bundleDependencies = data.bundledDependencies + } + delete data.bundledDependencies + const bd = data.bundleDependencies + if (bd === true) { + data.bundleDependencies = Object.keys(data.dependencies || {}) + } else if (bd && typeof bd === 'object') { + if (!Array.isArray(bd)) { + data.bundleDependencies = Object.keys(bd) + } + } else { + data.bundleDependencies = [] + } + + // it was once common practice to list deps both in optionalDependencies and + // in dependencies, to support npm versions that did not know about + // optionalDependencies. This is no longer a relevant need, so duplicating + // the deps in two places is unnecessary and excessive. + if (data.dependencies && + data.optionalDependencies && typeof data.optionalDependencies === 'object') { + for (const name in data.optionalDependencies) { + delete data.dependencies[name] + } + if (!Object.keys(data.dependencies).length) { + delete data.dependencies + } + } + + // scripts + if (typeof data.scripts === 'object') { + for (const name in data.scripts) { + if (typeof data.scripts[name] !== 'string') { + delete data.scripts[name] + } + } + } else { + delete data.scripts + } + + // funding + if (data.funding && typeof data.funding === 'string') { + data.funding = { url: data.funding } + } + + // bin + normalizePackageBin(data) +} + +module.exports = normalize diff --git a/test/index.js b/test/index.js index 911f8d5..b761728 100644 --- a/test/index.js +++ b/test/index.js @@ -89,8 +89,10 @@ t.test('read missing package.json', async t => { const path = t.testdirName return t.rejects( PackageJson.load(path), - /package.json not found/, - 'should throw package.json not found error' + { + message: /package.json/, + code: 'ENOENT', + } ) }) diff --git a/test/normalize.js b/test/normalize.js new file mode 100644 index 0000000..dca74d6 --- /dev/null +++ b/test/normalize.js @@ -0,0 +1,313 @@ +const t = require('tap') +const pkg = require('../') + +t.test('errors for bad/missing data', async t => { + t.test('raises an error for missing file', t => + t.rejects(pkg.normalize(t.testdir()), { code: 'ENOENT' })) + + t.test('rejects if file is not json', t => + t.rejects(pkg.normalize(t.testdir({ + 'package.json': 'this is not json', + })), { code: 'EJSONPARSE' })) +}) + +t.test('clean up bundleDependencies', async t => { + t.test('change name if bundleDependencies is not present', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ bundledDependencies: [] }), + })) + t.strictSame(content.bundleDependencies, []) + }) + + t.test('dont array-ify if its an array already', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ bundleDependencies: ['a'] }), + })) + t.strictSame(content.bundleDependencies, ['a']) + }) + + t.test('handle bundledDependencies: true', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bundledDependencies: true, + dependencies: { a: '1.2.3' }, + }), + })) + t.strictSame(content.bundleDependencies, ['a']) + }) + + t.test('handle bundleDependencies: true', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bundleDependencies: true, + dependencies: { a: '1.2.3' }, + }), + })) + t.strictSame(content.bundleDependencies, ['a']) + }) + + t.test('handle bundleDependencies: true with no deps', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bundleDependencies: true, + }), + })) + t.strictSame(content.bundleDependencies, []) + }) + + t.test('handle bundleDependencies: false', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bundleDependencies: false, + dependencies: { a: '1.2.3' }, + }), + })) + t.strictSame(content.bundleDependencies, []) + }) + + t.test('handle bundleDependencies object', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bundleDependencies: { a: '1.2.3' }, + dependencies: { a: '1.2.3' }, + }), + })) + t.strictSame(content.bundleDependencies, ['a']) + }) +}) + +t.test('clean up scripts', async t => { + t.test('delete non-object scripts', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + scripts: 1234, + }), + })) + t.notHasStrict(content, 'scripts') + }) + + t.test('delete non-string script targets', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + scripts: { + foo: 'bar', + bar: ['baz'], + baz: { bar: { foo: 'barbaz' } }, + }, + }), + })) + t.strictSame(content.scripts, { foo: 'bar' }) + }) +}) + +t.test('convert funding string to object', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ funding: 'hello' }), + })) + t.strictSame(content.funding, { url: 'hello' }) +}) + +t.test('cleanup bins', async t => { + t.test('handle string when a name is set', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ name: 'x', bin: 'y' }), + })) + t.strictSame(content.bin, { x: 'y' }) + }) + + t.test('delete string bin when no name', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ bin: 'y' }), + })) + t.notHasStrict(content, 'bin') + }) + + t.test('remove non-object bin', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ bin: 1234 }), + })) + t.notHasStrict(content, 'bin') + }) + + t.test('remove non-string bin values', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ bin: { + x: 'y', + y: 1234, + z: { a: 'b' }, + } }), + })) + t.strictSame(content.bin, { x: 'y' }) + }) +}) + +t.test('dedupe optional deps out of regular deps', async t => { + t.test('choose optional deps in conflict, removing empty dependencies', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + optionalDependencies: { + whowins: '1.2.3-optional', + }, + dependencies: { + whowins: '1.2.3-prod', + }, + }), + })) + t.notHasStrict(content, 'dependencies') + t.strictSame(content.optionalDependencies, { whowins: '1.2.3-optional' }) + }) + + t.test('choose optional deps in conflict, leaving populated dependencies', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + optionalDependencies: { + whowins: '1.2.3-optional', + }, + dependencies: { + otherdep: '1.0.0', + whowins: '1.2.3-prod', + }, + }), + })) + t.strictSame(content.dependencies, { otherdep: '1.0.0' }) + t.strictSame(content.optionalDependencies, { whowins: '1.2.3-optional' }) + }) + + t.test('do not create regular deps if only optional specified', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + optionalDependencies: { + whowins: '1.2.3-optional', + }, + }), + })) + t.notHasStrict(content, 'dependencies') + t.strictSame(content.optionalDependencies, { whowins: '1.2.3-optional' }) + }) +}) + +t.test('set _id if name and version set', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ name: 'a', version: '1.2.3' }), + })) + t.equal(content._id, 'a@1.2.3') +}) + +t.test('preserve indentation', async t => { + const obj = { + name: 'object', + version: '1.2.3', + } + const path = t.testdir({ + none: { + 'package.json': JSON.stringify(obj), + }, + twospace: { + 'package.json': JSON.stringify(obj, null, 2), + }, + tab: { + 'package.json': JSON.stringify(obj, null, '\t'), + }, + weird: { + 'package.json': JSON.stringify(obj, null, ' \t \t '), + }, + winEol: { + none: { + 'package.json': JSON.stringify(obj).replace(/\n/g, '\r\n'), + }, + twospace: { + 'package.json': JSON.stringify(obj, null, 2).replace(/\n/g, '\r\n'), + }, + tab: { + 'package.json': JSON.stringify(obj, null, '\t').replace(/\n/g, '\r\n'), + }, + weird: { + 'package.json': JSON.stringify(obj, null, ' \t \t ').replace(/\n/g, '\r\n'), + }, + }, + doubleSpaced: { + none: { + 'package.json': JSON.stringify(obj).replace(/\n/g, '\n\n'), + }, + twospace: { + 'package.json': JSON.stringify(obj, null, 2).replace(/\n/g, '\n\n'), + }, + tab: { + 'package.json': JSON.stringify(obj, null, '\t').replace(/\n/g, '\n\n'), + }, + weird: { + 'package.json': JSON.stringify(obj, null, ' \t \t ').replace(/\n/g, '\n\n'), + }, + }, + doubleWin: { + none: { + 'package.json': JSON.stringify(obj).replace(/\n/g, '\r\n\r\n'), + }, + twospace: { + 'package.json': JSON.stringify(obj, null, 2).replace(/\n/g, '\r\n\r\n'), + }, + tab: { + 'package.json': JSON.stringify(obj, null, '\t').replace(/\n/g, '\r\n\r\n'), + }, + weird: { + 'package.json': JSON.stringify(obj, null, ' \t \t ').replace(/\n/g, '\r\n\r\n'), + }, + }, + }) + const i = Symbol.for('indent') + const n = Symbol.for('newline') + t.equal((await pkg.normalize(`${path}/none`)).content[i], '') + t.equal((await pkg.normalize(`${path}/none`)).content[n], '') + t.equal((await pkg.normalize(`${path}/twospace`)).content[i], ' ') + t.equal((await pkg.normalize(`${path}/twospace`)).content[n], '\n') + t.equal((await pkg.normalize(`${path}/tab`)).content[i], '\t') + t.equal((await pkg.normalize(`${path}/tab`)).content[n], '\n') + t.equal((await pkg.normalize(`${path}/weird`)).content[i], ' \t \t ') + t.equal((await pkg.normalize(`${path}/weird`)).content[n], '\n') + t.equal((await pkg.normalize(`${path}/winEol/none`)).content[i], '') + t.equal((await pkg.normalize(`${path}/winEol/none`)).content[n], '') + t.equal((await pkg.normalize(`${path}/winEol/twospace`)).content[i], ' ') + t.equal((await pkg.normalize(`${path}/winEol/twospace`)).content[n], '\r\n') + t.equal((await pkg.normalize(`${path}/winEol/tab`)).content[i], '\t') + t.equal((await pkg.normalize(`${path}/winEol/tab`)).content[n], '\r\n') + t.equal((await pkg.normalize(`${path}/winEol/weird`)).content[i], ' \t \t ') + t.equal((await pkg.normalize(`${path}/winEol/weird`)).content[n], '\r\n') + t.equal((await pkg.normalize(`${path}/doubleSpaced/none`)).content[i], '') + t.equal((await pkg.normalize(`${path}/doubleSpaced/none`)).content[n], '') + t.equal((await pkg.normalize(`${path}/doubleSpaced/twospace`)).content[i], ' ') + t.equal((await pkg.normalize(`${path}/doubleSpaced/twospace`)).content[n], '\n\n') + t.equal((await pkg.normalize(`${path}/doubleSpaced/tab`)).content[i], '\t') + t.equal((await pkg.normalize(`${path}/doubleSpaced/tab`)).content[n], '\n\n') + t.equal((await pkg.normalize(`${path}/doubleSpaced/weird`)).content[i], ' \t \t ') + t.equal((await pkg.normalize(`${path}/doubleSpaced/weird`)).content[n], '\n\n') + t.equal((await pkg.normalize(`${path}/doubleWin/none`)).content[i], '') + t.equal((await pkg.normalize(`${path}/doubleWin/none`)).content[n], '') + t.equal((await pkg.normalize(`${path}/doubleWin/twospace`)).content[i], ' ') + t.equal((await pkg.normalize(`${path}/doubleWin/twospace`)).content[n], '\r\n\r\n') + t.equal((await pkg.normalize(`${path}/doubleWin/tab`)).content[i], '\t') + t.equal((await pkg.normalize(`${path}/doubleWin/tab`)).content[n], '\r\n\r\n') + t.equal((await pkg.normalize(`${path}/doubleWin/weird`)).content[i], ' \t \t ') + t.equal((await pkg.normalize(`${path}/doubleWin/weird`)).content[n], '\r\n\r\n') +}) + +t.test('strip _fields', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + name: 'underscore', + version: '1.2.3', + _lodash: true, + }), + })) + t.notHasStrict(content, '_lodash') +}) + +// For now this is just checking one of the many side effects of +// npm-normalize-package-bin so we're sure it got called +t.test('normalize bin', async t => { + const { content } = await pkg.normalize(t.testdir({ + 'package.json': JSON.stringify({ + bin: false, + }), + })) + t.notHasStrict(content, 'bin') +})