-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Conversation
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.
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} : {} |
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.
🤔
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
?
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.
If so, these lines can be condensed a little cuter:
const [spec, options] = p.rawSpec ? [p.fetchSpec, {}]
: ['*', { includePrerelease: true }]
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.
I’d expect both. Excluding prereleases is generally the surprising case for non-installs.
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.
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 `[email protected]`, then presumably `[email protected]` has the same expectation.
5db80d7
to
44d4331
Compare
Thanks @isaacs ! |
The NPM docs say:
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 theincludePrerelease: true
option so that all prereleases are included in the deprecated versions.References
Fixes #1615