-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix: unpublish bugfixes #7039
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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') | ||
|
|
@@ -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) { | ||
|
|
@@ -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 !== '*') { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💭 Looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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 | ||
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) | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Check |
||
| 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
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 <pkg>@version not the last version', async t => { | ||
| const { joinedOutput, npm } = await loadMockNpm(t, { | ||
| config: { | ||
|
|
@@ -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, | ||
|
|
@@ -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(), '- [email protected]') | ||
|
|
@@ -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]) | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.