Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unpublish bugfixes #7039

Merged
merged 1 commit into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/lib/content/commands/npm-unpublish.md
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.
Comment on lines +31 to +33
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
If you specify a package name but do not specify a version then the
registry will remove the root package entry entirely. The same applies
if you unpublish the last remaining version of a package.


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
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, {
hashtagchris marked this conversation as resolved.
Show resolved Hide resolved
...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 !== '*') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec.rawSpec !== '*'

💭 Looks like * is a specific case of a range spec. Would a isEntireRange method on the npm-package-arg response be useful, particularly if there's ever another rawSpec value that also selects the entire range?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* is a special snowflake (no pun intended) within npm spec parsing and it always means "one version, the best version you can give me" npm 9 even had a breaking change where the few places we were NOT doing that (i.e. x and x@) now parse exactly as if you typed x@*.

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
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

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}` : ''
hashtagchris marked this conversation as resolved.
Show resolved Hide resolved
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)
}

Copy link
Contributor

@hashtagchris hashtagchris Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Check pkgVersion and throw the Refusing to delete entire project usageError here, and get rid of the separate spec/non-spec checks above.

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 })
hashtagchris marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
103 changes: 64 additions & 39 deletions test/lib/commands/unpublish.js
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(), '- test-package@1.0.0')
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
Loading