From edcac41ebb174989818a07eaa48592194cc686e5 Mon Sep 17 00:00:00 2001 From: Gar Date: Fri, 1 Dec 2023 09:30:26 -0800 Subject: [PATCH 1/2] fix: unpublish bugfixes (#7039) - Properly work in workspace mode. Previously setting the workspace meant the entire package was unpublished. Now it follows the convention of acting as if you were running from the workspace directory. - Better checking of when you are unpublishing the last version of a package. npm checks the actual manifest and compares it to the version you are asking to unpublish. - Error on ranges and tags. npm doesn't unpublish ranges or tags, and giving those as inputs would give unexepected results. - Proper output of what was unpublished. Previously the package (and sometimes version) displayed would not match what was actually unpublished. - Updated docs specifying that unpublishing with no parameters will only unpublish the version represented by the local package.json --- docs/lib/content/commands/npm-unpublish.md | 8 +- lib/commands/unpublish.js | 103 +++++++++++---------- test/lib/commands/unpublish.js | 103 +++++++++++++-------- 3 files changed, 126 insertions(+), 88 deletions(-) diff --git a/docs/lib/content/commands/npm-unpublish.md b/docs/lib/content/commands/npm-unpublish.md index 82c4424c90d38..741fc83cee9aa 100644 --- a/docs/lib/content/commands/npm-unpublish.md +++ b/docs/lib/content/commands/npm-unpublish.md @@ -25,8 +25,12 @@ removing the tarball. The npm registry will return an error if you are not [logged in](/commands/npm-adduser). -If you do not specify a version or if you remove all of a package's -versions then the registry will remove the root package entry entirely. +If you do not specify a package name at all, the name and version to be +unpublished will be pulled from the project in the current directory. + +If you specify a package name but do not specify a version or if you +remove all of a package's versions then the registry will remove the +root package entry entirely. Even if you unpublish a package version, that specific name and version combination can never be reused. In order to publish the package again, diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index 402f8f30efff8..f5568a3540e12 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -1,7 +1,7 @@ const libaccess = require('libnpmaccess') const libunpub = require('libnpmpublish').unpublish const npa = require('npm-package-arg') -const npmFetch = require('npm-registry-fetch') +const pacote = require('pacote') const pkgJson = require('@npmcli/package-json') const { flatten } = require('@npmcli/config/lib/definitions') @@ -23,12 +23,12 @@ class Unpublish extends BaseCommand { static ignoreImplicitWorkspace = false static async getKeysOfVersions (name, opts) { - const pkgUri = npa(name).escapedName - const json = await npmFetch.json(`${pkgUri}?write=true`, { + const packument = await pacote.packument(name, { ...opts, spec: name, + query: { write: true }, }) - return Object.keys(json.versions) + return Object.keys(packument.versions) } static async completion (args, npm) { @@ -59,7 +59,7 @@ class Unpublish extends BaseCommand { return pkgs } - const versions = await this.getKeysOfVersions(pkgs[0], opts) + const versions = await Unpublish.getKeysOfVersions(pkgs[0], opts) if (!versions.length) { return pkgs } else { @@ -67,20 +67,35 @@ class Unpublish extends BaseCommand { } } - async exec (args) { + async exec (args, { localPrefix } = {}) { if (args.length > 1) { throw this.usageError() } - let spec = args.length && npa(args[0]) + // workspace mode + if (!localPrefix) { + localPrefix = this.npm.localPrefix + } + const force = this.npm.config.get('force') const { silent } = this.npm const dryRun = this.npm.config.get('dry-run') + let spec + if (args.length) { + spec = npa(args[0]) + if (spec.type !== 'version' && spec.rawSpec !== '*') { + throw this.usageError( + 'Can only unpublish a single version, or the entire project.\n' + + 'Tags and ranges are not supported.' + ) + } + } + log.silly('unpublish', 'args[0]', args[0]) log.silly('unpublish', 'spec', spec) - if ((!spec || !spec.rawSpec) && !force) { + if (spec?.rawSpec === '*' && !force) { throw this.usageError( 'Refusing to delete entire project.\n' + 'Run with --force to do this.' @@ -89,69 +104,63 @@ class Unpublish extends BaseCommand { const opts = { ...this.npm.flatOptions } - let pkgName - let pkgVersion let manifest - let manifestErr try { - const { content } = await pkgJson.prepare(this.npm.localPrefix) + const { content } = await pkgJson.prepare(localPrefix) manifest = content } catch (err) { - manifestErr = err - } - if (spec) { - // If cwd has a package.json with a name that matches the package being - // unpublished, load up the publishConfig - if (manifest && manifest.name === spec.name && manifest.publishConfig) { - flatten(manifest.publishConfig, opts) - } - const versions = await Unpublish.getKeysOfVersions(spec.name, opts) - if (versions.length === 1 && !force) { - throw this.usageError(LAST_REMAINING_VERSION_ERROR) - } - pkgName = spec.name - pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : '' - } else { - if (manifestErr) { - if (manifestErr.code === 'ENOENT' || manifestErr.code === 'ENOTDIR') { + // we needed the manifest to figure out the package to unpublish + if (!spec) { + if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { throw this.usageError() } else { - throw manifestErr + throw err } } + } - log.verbose('unpublish', manifest) - + let pkgVersion // for cli output + if (spec) { + pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : '' + } else { spec = npa.resolve(manifest.name, manifest.version) - if (manifest.publishConfig) { - flatten(manifest.publishConfig, opts) + log.verbose('unpublish', manifest) + pkgVersion = manifest.version ? `@${manifest.version}` : '' + if (!manifest.version && !force) { + throw this.usageError( + 'Refusing to delete entire project.\n' + + 'Run with --force to do this.' + ) } + } - pkgName = manifest.name - pkgVersion = manifest.version ? `@${manifest.version}` : '' + // If localPrefix has a package.json with a name that matches the package + // being unpublished, load up the publishConfig + if (manifest?.name === spec.name && manifest.publishConfig) { + flatten(manifest.publishConfig, opts) + } + + const versions = await Unpublish.getKeysOfVersions(spec.name, opts) + if (versions.length === 1 && spec.rawSpec === versions[0] && !force) { + throw this.usageError(LAST_REMAINING_VERSION_ERROR) + } + if (versions.length === 1) { + pkgVersion = '' } if (!dryRun) { await otplease(this.npm, opts, o => libunpub(spec, o)) } if (!silent) { - this.npm.output(`- ${pkgName}${pkgVersion}`) + this.npm.output(`- ${spec.name}${pkgVersion}`) } } async execWorkspaces (args) { await this.setWorkspaces() - const force = this.npm.config.get('force') - if (!force) { - throw this.usageError( - 'Refusing to delete entire project(s).\n' + - 'Run with --force to do this.' - ) - } - - for (const name of this.workspaceNames) { - await this.exec([name]) + for (const path of this.workspacePaths) { + await this.exec(args, { localPrefix: path }) } } } diff --git a/test/lib/commands/unpublish.js b/test/lib/commands/unpublish.js index 6e898bd3d07e4..044b2cf8c546a 100644 --- a/test/lib/commands/unpublish.js +++ b/test/lib/commands/unpublish.js @@ -26,10 +26,10 @@ t.test('no args --force success', async t => { authorization: 'test-auth-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- test-package@1.0.0') + t.equal(joinedOutput(), '- test-package') }) t.test('no args --force missing package.json', async t => { @@ -63,11 +63,11 @@ t.test('no args --force error reading package.json', async t => { ) }) -t.test('no args entire project', async t => { +t.test('no force entire project', async t => { const { npm } = await loadMockNpm(t) await t.rejects( - npm.exec('unpublish', []), + npm.exec('unpublish', ['@npmcli/unpublish-test']), /Refusing to delete entire project/ ) }) @@ -82,6 +82,26 @@ t.test('too many args', async t => { ) }) +t.test('range', async t => { + const { npm } = await loadMockNpm(t) + + await t.rejects( + npm.exec('unpublish', ['a@>1.0.0']), + { code: 'EUSAGE' }, + /single version/ + ) +}) + +t.test('tag', async t => { + const { npm } = await loadMockNpm(t) + + await t.rejects( + npm.exec('unpublish', ['a@>1.0.0']), + { code: 'EUSAGE' }, + /single version/ + ) +}) + t.test('unpublish @version not the last version', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { @@ -129,7 +149,24 @@ t.test('unpublish @version last version', async t => { ) }) -t.test('no version found in package.json', async t => { +t.test('no version found in package.json no force', async t => { + const { npm } = await loadMockNpm(t, { + config: { + ...auth, + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: pkg, + }, null, 2), + }, + }) + await t.rejects( + npm.exec('unpublish', []), + /Refusing to delete entire project/ + ) +}) + +t.test('no version found in package.json with force', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { force: true, @@ -147,7 +184,7 @@ t.test('no version found in package.json', async t => { authorization: 'test-auth-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) @@ -219,7 +256,7 @@ t.test('workspaces', async t => { 'workspace-b': { 'package.json': JSON.stringify({ name: 'workspace-b', - version: '1.2.3-n', + version: '1.2.3-b', repository: 'https://github.com/npm/workspace-b', }), }, @@ -231,20 +268,20 @@ t.test('workspaces', async t => { }, } - t.test('no force', async t => { + t.test('with package name no force', async t => { const { npm } = await loadMockNpm(t, { config: { - workspaces: true, + workspace: ['workspace-a'], }, prefixDir, }) await t.rejects( - npm.exec('unpublish', []), + npm.exec('unpublish', ['workspace-a']), /Refusing to delete entire project/ ) }) - t.test('all workspaces --force', async t => { + t.test('all workspaces last version --force', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { workspaces: true, @@ -258,9 +295,9 @@ t.test('workspaces', async t => { registry: npm.config.get('registry'), authorization: 'test-auth-token', }) - const manifestA = registry.manifest({ name: 'workspace-a' }) - const manifestB = registry.manifest({ name: 'workspace-b' }) - const manifestN = registry.manifest({ name: 'workspace-n' }) + const manifestA = registry.manifest({ name: 'workspace-a', versions: ['1.2.3-a'] }) + const manifestB = registry.manifest({ name: 'workspace-b', versions: ['1.2.3-b'] }) + const manifestN = registry.manifest({ name: 'workspace-n', versions: ['1.2.3-n'] }) await registry.package({ manifest: manifestA, query: { write: true }, times: 2 }) await registry.package({ manifest: manifestB, query: { write: true }, times: 2 }) await registry.package({ manifest: manifestN, query: { write: true }, times: 2 }) @@ -271,28 +308,6 @@ t.test('workspaces', async t => { await npm.exec('unpublish', []) t.equal(joinedOutput(), '- workspace-a\n- workspace-b\n- workspace-n') }) - - t.test('one workspace --force', async t => { - const { joinedOutput, npm } = await loadMockNpm(t, { - config: { - workspace: ['workspace-a'], - force: true, - ...auth, - }, - prefixDir, - }) - const registry = new MockRegistry({ - tap: t, - registry: npm.config.get('registry'), - authorization: 'test-auth-token', - }) - const manifestA = registry.manifest({ name: 'workspace-a' }) - await registry.package({ manifest: manifestA, query: { write: true }, times: 2 }) - registry.nock.delete(`/workspace-a/-rev/${manifestA._rev}`).reply(201) - - await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- workspace-a') - }) }) t.test('dryRun with spec', async t => { @@ -331,6 +346,16 @@ t.test('dryRun with no args', async t => { }, null, 2), }, }) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: 'test-auth-token', + }) + const manifest = registry.manifest({ + name: pkg, + packuments: ['1.0.0', '1.0.1'], + }) + await registry.package({ manifest, query: { write: true } }) await npm.exec('unpublish', []) t.equal(joinedOutput(), '- test-package@1.0.0') @@ -360,10 +385,10 @@ t.test('publishConfig no spec', async t => { authorization: 'test-other-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- test-package@1.0.0') + t.equal(joinedOutput(), '- test-package') }) t.test('publishConfig with spec', async t => { @@ -421,7 +446,7 @@ t.test('scoped registry config', async t => { authorization: 'test-other-token', }) const manifest = registry.manifest({ name: scopedPkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', [scopedPkg]) }) From bae2ee50ae6c06994727b20bb3ad2018b7d2a8bd Mon Sep 17 00:00:00 2001 From: Gar Date: Fri, 1 Dec 2023 11:44:28 -0800 Subject: [PATCH 2/2] fix(unpublish): bubble up all errors parsing local package.json (#7049) If the file is there and there is any error that's always a show stopper --- lib/commands/unpublish.js | 14 +++++++++----- test/lib/commands/unpublish.js | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index f5568a3540e12..a9c20900534c3 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -109,13 +109,17 @@ class Unpublish extends BaseCommand { const { content } = await pkgJson.prepare(localPrefix) manifest = content } catch (err) { - // we needed the manifest to figure out the package to unpublish - if (!spec) { - if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { + if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { + if (!spec) { + // We needed a local package.json to figure out what package to + // unpublish throw this.usageError() - } else { - throw err } + } else { + // folks should know if ANY local package.json had a parsing error. + // They may be relying on `publishConfig` to be loading and we don't + // want to ignore errors in that case. + throw err } } diff --git a/test/lib/commands/unpublish.js b/test/lib/commands/unpublish.js index 044b2cf8c546a..097309393a258 100644 --- a/test/lib/commands/unpublish.js +++ b/test/lib/commands/unpublish.js @@ -63,6 +63,23 @@ t.test('no args --force error reading package.json', async t => { ) }) +t.test('with args --force error reading package.json', async t => { + const { npm } = await loadMockNpm(t, { + config: { + force: true, + }, + prefixDir: { + 'package.json': '{ not valid json ]', + }, + }) + + await t.rejects( + npm.exec('unpublish', [pkg]), + /Invalid package.json/, + 'should throw error from reading package.json' + ) +}) + t.test('no force entire project', async t => { const { npm } = await loadMockNpm(t)