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

Include prerelease versions when deprecating an entire package. #2366

Merged
merged 1 commit into from Dec 18, 2020

Conversation

tiegz
Copy link
Contributor

@tiegz tiegz commented Dec 17, 2020

The NPM docs say:

To deprecate an entire package, run the following command, replacing <package-name> 
with  the name of your package, and "<message>" with your deprecation message:

npm deprecate <package-name> "<message>"

This should mark all versions as deprecated, but it currently excludes any prerelease versions.

The filtering of versions in the deprecate command is done by passing a version range to semver.satisfies(). A wildcard * is passed as the version range when deprecating an entire package, but wildcards don't include prereleases by the semver library's rules, so we need to pass the includePrerelease: true option so that all prereleases are included in the deprecated versions.

References

Fixes #1615

@tiegz tiegz requested a review from a team as a code owner December 17, 2020 01:04
@darcyclarke darcyclarke added Needs Review Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release labels Dec 17, 2020
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Seems good to me, just a comment about whether npm deprecate pkg@* should include prereleases as well.

lib/deprecate.js Outdated
@@ -45,6 +45,7 @@ const deprecate = async ([pkg, msg]) => {
// npa makes the default spec "latest", but for deprecation
// "*" is the appropriate default.
const spec = p.rawSpec === '' ? '*' : p.fetchSpec
const options = spec === '*' ? {includePrerelease: true} : {}
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Should this check be const options = p.rawSpec === '' ? { includePrerelease: true } : {}?

Ie, if you did npm deprecate foo@* should that include prereleases? Or only if you do npm deprecate foo?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, these lines can be condensed a little cuter:

const [spec, options] = p.rawSpec ? [p.fetchSpec, {}]
  : ['*', { includePrerelease: true }]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d expect both. Excluding prereleases is generally the surprising case for non-installs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect both too, but I didn't know about the prerelease rules in the semver library until this week 🤯

What if I added @isaacs 's recommendation and then a note was added to the docs, e.g. note that passing a range '*' will not deprecate prereleases versions.?

PR-URL: npm#2366
Credit: @tiegz
Close: npm#2366
Reviewed-by: @isaacs

EDIT(@isaacs): updated to make _all_ deprecation ranges include
prereleases.  If `foo@*` would be expected to deprecate
`foo@1.0.0-beta`, then presumably `foo@1.x` has the same expectation.
@isaacs isaacs changed the base branch from latest to release/v7.3.0 December 18, 2020 19:48
@isaacs isaacs merged commit 44d4331 into npm:release/v7.3.0 Dec 18, 2020
@tiegz
Copy link
Contributor Author

tiegz commented Dec 18, 2020

Thanks @isaacs !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to deprecate package with - (hyphen) in the version number
4 participants