Skip to content

Commit

Permalink
fix: unpublish bugfixes (#7039)
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@wraithgar
wraithgar committed Dec 1, 2023
1 parent ce701e9 commit e0e75e5
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 88 deletions.
8 changes: 6 additions & 2 deletions docs/lib/content/commands/npm-unpublish.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
103 changes: 56 additions & 47 deletions lib/commands/unpublish.js
View file
Original file line number Diff line number Diff line change
@@ -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')
Expand All @@ -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) {
Expand Down Expand Up @@ -59,28 +59,43 @@ 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 {
return versions.map(v => `${pkgs[0]}@${v}`)
}
}

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.'
Expand All @@ -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 })
}
}
}
Expand Down
103 changes: 64 additions & 39 deletions test/lib/commands/unpublish.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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/
)
})
Expand All @@ -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 <pkg>@version not the last version', async t => {
const { joinedOutput, npm } = await loadMockNpm(t, {
config: {
Expand Down Expand Up @@ -129,7 +149,24 @@ t.test('unpublish <pkg>@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,
Expand All @@ -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', [])
Expand Down Expand Up @@ -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',
}),
},
Expand All @@ -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,
Expand All @@ -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 })
Expand All @@ -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 => {
Expand Down Expand Up @@ -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(), '- [email protected]')
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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])
})
Expand Down

0 comments on commit e0e75e5

Please sign in to comment.