-
Notifications
You must be signed in to change notification settings - Fork 332
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
[FR] --target minor or patch should exclude 0.x.x release #958
Comments
Well... of course, I think |
npm-check-updates/lib/version-util.js Lines 210 to 215 in 26ceb1e
I think we can solve this problem by adding a condition to check for 'minor' and 'patch' when |
But... this fix seems to me to be a breaking change. |
@ashidaharo I've just discovered it too) It should be something that was left intentionally or a new bug (but I think it was always like this). I should not think nobody hasn't noticed it during all this time, for it's a very important behavior. But what was the intension? |
I think it should depend on how clear it was communicated from the description of the interface of the library, though it's a very interesting question by itself;) |
When I found the source of the problem, I confirmed that this was a behavior that had always existed, whether it was intentional or not. I think it's probably just that the person who originally wrote it didn't pay attention to exceptions like '0.x.x'. However, the current behavior has been confirmed as a specification by the following tests. npm-check-updates/test/version-util.test.js Lines 393 to 396 in 26ceb1e
For that reason, I considered the current behavior of this library to be as per the specification of this package, although it does not follow the SemVer specification. Therefore, I thought that fixing this problem should be treated as a breaking change, not a bug. |
Well, it's just that... there's no consistency between the color coding when |
Thanks for reporting!
That makes sense. We should change it. We may have to add a
I would consider it a breaking change, since developers could be counting on the existing behavior (even if it is wrong from a semver perspective). |
Uh... then I'm sorry, but I'm afraid I can't help you... |
so is it 2 issues here? It might sound crazy but is it possible that people got used to this inconsistency of "existing behavior" and either of the changes should be a major update?)) I think I might get to use to see red colors in --target minor too, maybe it makes sense after all?) They are special - https://semver.org/#spec-item-4 |
Docs: |
This could be done for completeness, but it's low priority.
Correct.
Yes, though it is called
Only the change in functionality would be a breaking change. The color change is not breaking since it is not part of the official API. |
So, we want --target minor not to return them not because of the
for they don't have a notion of minor change for (0.y.z) like but because of the https://github.com/npm/node-semver#caret-ranges-123-025-004
But wait a minute..
so they call them patch updates. So it should not be in --target minor then.. but not in major either. Is that what's needed? and major !== breaking change btw |
I'd say we should stick with the definitions of major , minor, patch from https://semver.org and https://github.com/npm/node-semver which look consistent with each other, just 1st 2nd 3rd digit, that's it. And a red color for everything that might contain a breaking change. If it's ok, then I think I understand what should be done) |
And..it seems to me now that nothing should be done, because it already works like this )) |
Yes, it's only |
sorry, I don't really see why they are broken. 0.x.x are paches (according to npm docs). I thought your "minor" should include them, like minor and all less than minor. May we have a quick conference (I prefer google meet but zoom should be fine as well) so we can sort it out eventually, cause chatting tends to be excessively long because of the long feedback time ? 🙂 |
A patch version is defined in the spec as follows: "Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced." The where-clause clearly excludes cases where the major version is I can hold off on the change until more people upvote this issue. Nobody has complained about this behavior for the few years that it has existed, so maybe I should listen to that.
No, but thank you for offering. I'm of the (possibly unpopular) opinion that technical discussions benefit from being in written form since it forces those involved to be precise in their language, and automatically provides a "change log" of the discussion. |
"Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced." it's fine and I don't see any inconsistency with the current behavior
Yeah, I think so. And that's consistent with the definition of patch. Minor would be minor or patch. Patch is just the third digit. that's it. No more and no less. It might be a breaking change even in case of 0.X.Y because of
and MUST not be in case of Z (x.y.Z | x > 0)
usually I do some meetings and then write down the decisions and why they were made.....anyway) Kinda funny I'm convincing your back from what I was standing for, must be very confusing for everybody who might read this) All this major, bracking, ^ are very confusing but I feel like I've got it eventually)) Probably you want your "minor" to be ^ ? ) |
I have some thoughts after reading this discussion, so let me give my new opinion with some modifications, although it may confuse the discussion further. I would first argue that, as before: The difference between the color coding when using https://github.com/npm/node-semver#tilde-ranges-123-12-1 In contrast, the current color-coding rules are defined as follows. npm-check-updates/lib/version-util.js Lines 179 to 184 in 26ceb1e
This is clearly different from the behavior of node-semver when either tildes or carets are given to a version. And... this is more of a personal bias than following the spec, but I think most people use Caret to declare SemVer anyway... Therefore, in my biased opinion, if we are going to change the behavior, we should add part of the behavior definition of caret-ranges in node-semver to the SemVer definition as a basis for defining the new behavior.
That's all. I know this will confuse the discussion even more, but it seemed to me that if we were going to make a change, it would be better to base it on this approach. |
Well, of course, node-semver also says this.
This behavior is only based on "commonly observed practices" and is not quite specifics. |
Oh, I forgot. |
There are 3 things: minor, caret (^) and breaking change and they are not the same and not the opposite of each other. It's like Venn diagram. Minor it's just the second digit and I haven't seen nowhere in the standards that it's something different, except maybe this small hint
but I'd put it all in the ""
The current red color, as I can see from the source code, reflects that it might be a breaking change. |
In npm, in You can check this out yourself by using https://npmjs.com/semver. |
sorry, what exactly I should call to see that in 0.x.y x is semver-major let's say? Are you saying it's semver-major? @ljharb |
@kokushkin |
but caret ^ and major/minor are different things.Can you show us in the documentation that minor = ^ ? |
No, because a) there's no documentation needed, the implementation is what matters, and b) minor doesn't equal Go to http://semver.npmjs.com. Type in |
In my opinion, semver definition says
So, even if the 0.0.x update is treated as a non-breaking update by commonly practices, i think it should be treated as a breaking change by definition, since it is only practices.
|
@ashidaharo That's v2 of the semver spec. npm follows v1 of the spec, and "what npm does" is the arbiter of truth for the npm ecosystem. npm's rules are #958 (comment) - there's no room for debate. |
|
none the less, semver.npmjs.com and the semver package treats it the way i've described, so all that means is the docs are wrong. |
Yes, that is mentioned.
However, this is not a definition, but an implementation of npm's Semver. |
The implementation is the only thing that matters, since that's how |
It looks like we will have to wait for the response of the maintainer and others who are interested in this issue, as we cannot seem to reach a mutual agreement on this. |
I mean sure, but what would the point be of "npm check updates" if it wasn't checking "what npm will actually update" |
https://github.com/raineorshine/npm-check-updates#npm-check-updates |
If you want to update safely according to npm's behavior, use |
And in that way, you can use not only Caret but also Tilde, X-Ranges, and various other version control methods provided by npm. |
From the beginning, this tool was a tool that worked independently of the npm implementation, or rather, actively ignored it, even before the target option was added. |
I take your point. It's not a binary breaking-nonbreaking distinction, and it's not strictly 1st 2nd 3rd, although it's close to that. As described in the README:
I actually think this is still the best representation. Showing
That's right. npm-check-updates has primarily existed to forcefully break versions, since this functionality is not in npm. Over time, developers have come to use npm-check-updates as an alternative to |
so --target semver would show what? |
Well, I suppose I would want |
I'm not sure if npm update does anything about dependencies without ^ or ~ (rather nothing). --target semver also doesn't reflect what is written in semver in case of 0.x.y. Would it be better to have just --target caret (updates everything as if it has ^), and --target tilde (updates everything as if it has ~)? |
Yes, I like that!
Faster releases and better support :) |
I am currently using the
gives me not exactly want I want. So instead I changed that to
Which gives me the "correct" minor and patches for the ones I want and skips pinned versions. Though I wonder if we could introduce a new target version like
What do you think? EDIT: Apparently I missed #581 / #950 - however if you like I can try to take a stab at it 👍🏻 |
@Primajin Please open a new issue and I will respond there. This issue is specifically about whether I recognize that I myself floated |
Sure happy to - just thought two birds one stone 😅 |
As of /** Custom target.
@param dependencyName The name of the dependency.
@param parsedVersion A parsed Semver object from semver-utils.
(See https://git.coolaj86.com/coolaj86/semver-utils.js#semverutils-parse-semverstring)
@returns One of the valid target values (specified in the table above).
*/
target: (dependencyName, [{ semver, version, operator, major, minor, patch, release, build }]) => {
if (major === '0') return 'minor'
return 'latest'
} (edited to reflect the string type pointed out by @betaorbust) You can hack together your own semver-like target logic, but the intention is to eventually add a |
As a note: on my machine the major tag came through as a string, instead of a number. -if (major === 0) return 'minor'
+if (major === '0') return 'minor' @raineorshine Thanks for your time and effort making such a great tool! |
@betaorbust You can also just use two equals |
Chaotic Alignment has entered the chat 😆 |
Closing, as described in #958 (comment). For the proposed |
npm-check-updates
node >= 10.17
Steps to Reproduce
or
Current Behavior
Both results include the new release of 0.x.x.
like
Expected Behavior
0.x.x means that all release MAY contain BREAKING CHANGES.
So, release of 0.x.x should be treated as a MAJOR release.
This should be a feasible feature, since it is correctly displayed as such when --color is set.
The text was updated successfully, but these errors were encountered: