From b547e0dfbc51c20ca13cfaba9a27f221dc38fc10 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Tue, 23 Apr 2024 08:25:14 -0700 Subject: [PATCH] fix: use @npmcli/package-json (#356) --- lib/dir.js | 2 +- lib/fetcher.js | 13 +++++++------ lib/file.js | 31 ++++++++++++++++--------------- lib/git.js | 6 +++--- lib/registry.js | 8 ++++---- package.json | 3 +-- test/bin.js | 2 +- test/fetcher.js | 13 ++++++------- test/git.js | 12 ++++++------ 9 files changed, 45 insertions(+), 45 deletions(-) diff --git a/lib/dir.js b/lib/dir.js index 6979462e..135be8e6 100644 --- a/lib/dir.js +++ b/lib/dir.js @@ -87,7 +87,7 @@ class DirFetcher extends Fetcher { return Promise.resolve(this.package) } - return this[_readPackageJson](this.resolved + '/package.json') + return this[_readPackageJson](this.resolved) .then(mani => this.package = { ...mani, _integrity: this.integrity && String(this.integrity), diff --git a/lib/fetcher.js b/lib/fetcher.js index 287ec795..c4a707e7 100644 --- a/lib/fetcher.js +++ b/lib/fetcher.js @@ -5,7 +5,6 @@ const npa = require('npm-package-arg') const ssri = require('ssri') -const { promisify } = require('util') const { basename, dirname } = require('path') const tar = require('tar') const { log } = require('proc-log') @@ -16,12 +15,14 @@ const cacache = require('cacache') const isPackageBin = require('./util/is-package-bin.js') const removeTrailingSlashes = require('./util/trailing-slashes.js') const getContents = require('@npmcli/installed-package-contents') -const readPackageJsonFast = require('read-package-json-fast') -const readPackageJson = promisify(require('read-package-json')) +const PackageJson = require('@npmcli/package-json') const { Minipass } = require('minipass') - const cacheDir = require('./util/cache-dir.js') +// Pacote is only concerned with the package.json contents +const packageJsonPrepare = (p) => PackageJson.prepare(p).then(pkg => pkg.content) +const packageJsonNormalize = (p) => PackageJson.normalize(p).then(pkg => pkg.content) + // Private methods. // Child classes should not have to override these. // Users should never call them. @@ -93,9 +94,9 @@ class FetcherBase { this.fullMetadata = this.before ? true : !!opts.fullMetadata this.fullReadJson = !!opts.fullReadJson if (this.fullReadJson) { - this[_readPackageJson] = readPackageJson + this[_readPackageJson] = packageJsonPrepare } else { - this[_readPackageJson] = readPackageJsonFast + this[_readPackageJson] = packageJsonNormalize } // rrh is a registry hostname or 'never' or 'always' diff --git a/lib/file.js b/lib/file.js index bf99bb86..95769de1 100644 --- a/lib/file.js +++ b/lib/file.js @@ -1,10 +1,11 @@ -const Fetcher = require('./fetcher.js') const fsm = require('fs-minipass') const cacache = require('cacache') -const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved') -const _exeBins = Symbol('_exeBins') const { resolve } = require('path') -const fs = require('fs') +const { stat, chmod } = require('fs/promises') +const Fetcher = require('./fetcher.js') + +const _exeBins = Symbol('_exeBins') +const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved') const _readPackageJson = Symbol.for('package.Fetcher._readPackageJson') class FileFetcher extends Fetcher { @@ -26,7 +27,7 @@ class FileFetcher extends Fetcher { // have to unpack the tarball for this. return cacache.tmp.withTmp(this.cache, this.opts, dir => this.extract(dir) - .then(() => this[_readPackageJson](dir + '/package.json')) + .then(() => this[_readPackageJson](dir)) .then(mani => this.package = { ...mani, _integrity: this.integrity && String(this.integrity), @@ -40,23 +41,23 @@ class FileFetcher extends Fetcher { return Promise.resolve() } - return Promise.all(Object.keys(pkg.bin).map(k => new Promise(res => { + return Promise.all(Object.keys(pkg.bin).map(async k => { const script = resolve(dest, pkg.bin[k]) // Best effort. Ignore errors here, the only result is that // a bin script is not executable. But if it's missing or // something, we just leave it for a later stage to trip over // when we can provide a more useful contextual error. - fs.stat(script, (er, st) => { - if (er) { - return res() - } + try { + const st = await stat(script) const mode = st.mode | 0o111 if (mode === st.mode) { - return res() + return } - fs.chmod(script, mode, res) - }) - }))) + await chmod(script, mode) + } catch { + // Ignore errors here + } + })) } extract (dest) { @@ -64,7 +65,7 @@ class FileFetcher extends Fetcher { // but if not, read the unpacked manifest and chmod properly. return super.extract(dest) .then(result => this.package ? result - : this[_readPackageJson](dest + '/package.json').then(pkg => + : this[_readPackageJson](dest).then(pkg => this[_exeBins](pkg, dest)).then(() => result)) } diff --git a/lib/git.js b/lib/git.js index 533d83d3..2cac44ae 100644 --- a/lib/git.js +++ b/lib/git.js @@ -156,11 +156,11 @@ class GitFetcher extends Fetcher { [_resolvedFromClone] () { // do a full or shallow clone, then look at the HEAD // kind of wasteful, but no other option, really - return this[_clone](dir => this.resolved) + return this[_clone](() => this.resolved) } [_prepareDir] (dir) { - return this[_readPackageJson](dir + '/package.json').then(mani => { + return this[_readPackageJson](dir).then(mani => { // no need if we aren't going to do any preparation. const scripts = mani.scripts if (!mani.workspaces && (!scripts || !( @@ -312,7 +312,7 @@ class GitFetcher extends Fetcher { return this.spec.hosted && this.resolved ? FileFetcher.prototype.manifest.apply(this) : this[_clone](dir => - this[_readPackageJson](dir + '/package.json') + this[_readPackageJson](dir) .then(mani => this.package = { ...mani, _resolved: this.resolved, diff --git a/lib/registry.js b/lib/registry.js index de25a11a..1423a92b 100644 --- a/lib/registry.js +++ b/lib/registry.js @@ -3,7 +3,7 @@ const RemoteFetcher = require('./remote.js') const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved') const pacoteVersion = require('../package.json').version const removeTrailingSlashes = require('./util/trailing-slashes.js') -const rpj = require('read-package-json-fast') +const PackageJson = require('@npmcli/package-json') const pickManifest = require('npm-pick-manifest') const ssri = require('ssri') const crypto = require('crypto') @@ -127,12 +127,12 @@ class RegistryFetcher extends Fetcher { } const packument = await this.packument() - let mani = await pickManifest(packument, this.spec.fetchSpec, { + const mani = await new PackageJson().fromContent(pickManifest(packument, this.spec.fetchSpec, { ...this.opts, defaultTag: this.defaultTag, before: this.before, - }) - mani = rpj.normalize(mani) + })).normalize().then(p => p.content) + /* XXX add ETARGET and E403 revalidation of cached packuments here */ // add _time from packument if fetched with fullMetadata diff --git a/package.json b/package.json index 9fc3f2cf..d0d9e877 100644 --- a/package.json +++ b/package.json @@ -46,6 +46,7 @@ "dependencies": { "@npmcli/git": "^5.0.0", "@npmcli/installed-package-contents": "^2.0.1", + "@npmcli/package-json": "^5.1.0", "@npmcli/promise-spawn": "^7.0.0", "@npmcli/run-script": "^8.0.0", "cacache": "^18.0.0", @@ -57,8 +58,6 @@ "npm-registry-fetch": "^16.0.0", "proc-log": "^4.0.0", "promise-retry": "^2.0.1", - "read-package-json": "^7.0.0", - "read-package-json-fast": "^3.0.0", "sigstore": "^2.2.0", "ssri": "^10.0.0", "tar": "^6.1.11" diff --git a/test/bin.js b/test/bin.js index 264ee3b9..e91bf289 100644 --- a/test/bin.js +++ b/test/bin.js @@ -23,7 +23,7 @@ pacote.manifest = (spec, conf) => Promise.resolve({ pacote.packument = (spec, conf) => Promise.resolve({ method: 'packument', spec, conf }) pacote.tarball.file = (spec, file, conf) => Promise.resolve({ method: 'tarball', spec, file, conf }) const { Minipass } = require('minipass') -pacote.tarball.stream = (spec, handler, conf) => handler(new Minipass().end('tarball data')) +pacote.tarball.stream = (spec, handler) => handler(new Minipass().end('tarball data')) pacote.extract = (spec, dest, conf) => Promise.resolve({ method: 'extract', spec, dest, conf }) t.test('running bin runs main file', t => { diff --git a/test/fetcher.js b/test/fetcher.js index bd0e88a4..6bff73eb 100644 --- a/test/fetcher.js +++ b/test/fetcher.js @@ -94,15 +94,14 @@ t.test('tarball data', async t => { t.test('tarballFile', t => { const target = resolve(me, 'tarball-file') - t.test('basic copy', t => - new FileFetcher(abbrevspec, { cache }) - .tarballFile(target + '/basic/1.tgz')) - t.test('again, folder already created', t => - new FileFetcher(abbrevspec, { cache }) - .tarballFile(target + '/basic/2.tgz')) + t.test('basic', async t => { + await new FileFetcher(abbrevspec, { cache }) + .tarballFile(target + '/basic/1.tgz') + + await new FileFetcher(abbrevspec, { cache }) + .tarballFile(target + '/basic/2.tgz') - t.test('check it', t => { const one = fs.readFileSync(target + '/basic/1.tgz') const two = fs.readFileSync(target + '/basic/2.tgz') const expect = fs.readFileSync(abbrev) diff --git a/test/git.js b/test/git.js index 54b668fb..94ff5c6b 100644 --- a/test/git.js +++ b/test/git.js @@ -87,7 +87,7 @@ const tar = require('tar') let REPO_HEAD = '' t.test('setup', { bail: true }, t => { - t.test('create repo', t => { + t.test('create repo', () => { const git = (...cmd) => spawnGit(cmd, { cwd: repo }) const write = (f, c) => fs.writeFileSync(`${repo}/${f}`, c) const npm = (...cmd) => spawnNpm('npm', [ @@ -162,7 +162,7 @@ t.test('setup', { bail: true }, t => { .then(({ stdout }) => REPO_HEAD = stdout.trim()) }) - t.test('create cycle of git prepared deps', async t => { + t.test('create cycle of git prepared deps', async () => { for (const repoDir of [cycleA, cycleB]) { const git = (...cmd) => spawnGit(cmd, { cwd: repoDir }) const write = (f, c) => fs.writeFileSync(`${repoDir}/${f}`, c) @@ -227,7 +227,7 @@ t.test('setup', { bail: true }, t => { daemon.on('close', () => rmSync(me, { recursive: true, force: true })) }) - t.test('create a repo with a submodule', t => { + t.test('create a repo with a submodule', () => { const submoduleRepo = resolve(me, 'submodule-repo') const git = (...cmd) => spawnGit(cmd, { cwd: submoduleRepo }) const write = (f, c) => fs.writeFileSync(`${submoduleRepo}/${f}`, c) @@ -285,7 +285,7 @@ t.test('setup', { bail: true }, t => { .then(() => git('commit', '-m', 'package')) }) - t.test('create a repo with workspaces', t => { + t.test('create a repo with workspaces', () => { const workspacesRepo = resolve(me, 'workspaces-repo') const wsfolder = resolve(me, 'workspaces-repo/a') const git = (...cmd) => spawnGit(cmd, { cwd: workspacesRepo }) @@ -315,7 +315,7 @@ t.test('setup', { bail: true }, t => { .then(() => git('commit', '-m', 'a/package.json')) }) - t.test('create a repo with only a prepack script', t => { + t.test('create a repo with only a prepack script', () => { const prepackRepo = resolve(me, 'prepack-repo') const git = (...cmd) => spawnGit(cmd, { cwd: prepackRepo }) const write = (f, c) => fs.writeFileSync(`${prepackRepo}/${f}`, c) @@ -608,7 +608,7 @@ t.test('fetch a weird ref', t => { t.end() }) -t.test('fetch a private repo where the tgz is a 404', t => { +t.test('fetch a private repo where the tgz is a 404', () => { const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, opts) gf.spec.hosted.tarball = () => `${hostedUrl}/not-found.tgz` // should fetch it by falling back to ssh when it gets an http error