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

fix: unpublish bugfixes #7039

merged 1 commit into from
Dec 1, 2023

Conversation

wraithgar
Copy link
Member

  • 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

Closes: #7015

@wraithgar wraithgar requested a review from a team as a code owner November 30, 2023 20:35
 - 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
Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Great tests and comments made this easy to follow along. :shipit:

@wraithgar wraithgar merged commit be4741f into latest Dec 1, 2023
21 of 25 checks passed
@wraithgar wraithgar deleted the gar/workspace-unpublish branch December 1, 2023 17:30
Comment on lines +31 to +33
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.
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.

lib/commands/unpublish.js Show resolved Hide resolved
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@*.

lib/commands/unpublish.js Show resolved Hide resolved
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.

lib/commands/unpublish.js Show resolved Hide resolved
lib/commands/unpublish.js Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Dec 1, 2023
wraithgar added a commit that referenced this pull request Dec 1, 2023
- 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
wraithgar added a commit that referenced this pull request Dec 1, 2023
- 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
@github-actions github-actions bot mentioned this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm unpublish --force <package-name>@<version> deletes all versions
3 participants