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

Weird behavior: in unstable, ^1.1.0 is not updated to ^1.2.1 #94

Closed
etiktin opened this issue Jul 4, 2015 · 26 comments
Closed

Weird behavior: in unstable, ^1.1.0 is not updated to ^1.2.1 #94

etiktin opened this issue Jul 4, 2015 · 26 comments
Labels

Comments

@etiktin
Copy link
Contributor

etiktin commented Jul 4, 2015

The unstable version (v2.0.0-alpha.11) returns different results from the stable version (v1.5.1).
In my package.json I have the following dependency:

  "dependencies": {
    ...
    "bindings": "^1.1.0",
    ...
  }

The bindings latest version is 1.2.1 (ATTOW). When I run the unstable version of ncu it doesn't list the update, but when I run the stable version it does.
The reason it doesn't list the update is because in the isUpgradeable function we test if the latest version is satisfied by the current version and if it does, we skip it (1.2.1 is satisfied by ^1.1.0). I guess the reason this test was added is because if the latest version is satisfied by the current version, all you have to do to get it is npm install.
Some users, myself included, expect ncu to let us know of any update. If I had run npm i bindings --save when the version was 1.1.0 and then I run ncu today when the version is 1.2.1, I won't get any warning...
Also, the documentation in the readme file (for v2), in the "How dependency updates are determined" section states that ncu will do the following:

^1.2.0 => ^1.3.0

But it's not how v2 currently works.

I suggest removing the !semver.satisfies(latest, version) so we will be notified of any update.
At the very list we should update the documentation to make it clear that v2 doesn't show updates to versions that are satisfied by the current version.

@etiktin etiktin changed the title Weird behavior: in unstable, ^1.0.0 is not updated to ^1.2.1 Weird behavior: in unstable, ^1.1.0 is not updated to ^1.2.1 Jul 4, 2015
@raineorshine
Copy link
Owner

Thank you for a clear explanation of this incongruence. I agree that this would be confusing for anyone switching from v1. I have updated the README to highlight the key differences in v2. Let me know how it looks.

The change was indeed made because npm update already handles upgrades within satisfied version ranges. npm-check-updates is intended to focus on functionality that npm does not provide itself. There are some people that do not understand how to best leverage semver ranges, and are constantly upgrading their package.json when it is not necessary.

For the old behavior, you can specify the -f/--force flag.

I'm open to hearing about use cases that I may not have considered.

@etiktin
Copy link
Contributor Author

etiktin commented Jul 4, 2015

Your welcome. Thanks for the quick reply.

The changes to the documentation looks good, the issue is clarified.

I think that npm update is not a good replacement for the old behavior because it doesn't just list the available updates, it actually updates them. So if I just want to check, not install, I have to turn to ncu -f.

I didn't realize what the -f flag did, before your comment. I guess I just assumed that a --force flag does something else, like upgrading and removing the semantic rules. Maybe changing the flag name and description will make it clearer. For example:

-a, --all(or any?)         get all upgrades including upgrades where the latest version satisfies
                           the declared semver dependency

What about changing ncu so that it will always list all the updates, but updates that are satisfied by npm update will be listed in a different color or with some prefix, so it will be obvious that there is an update (and you can see if what you have installed is older)?
When you choose to upgrade using -u we won't change the versions for those dependencies because there is no real need to.
When you use -f it will force update those version numbers in package.json (it will do -u for all upgrades).
What do you think?

@raineorshine
Copy link
Owner

I like that idea. How about this?

  • Default behavior without -u shows all upgrades but says "satisfies current dependency" for the appropriate packages
  • Default behavior with -u only upgrades those that are not satisfied (v2-alpha behavior)
  • Using -u and -a/--all flags upgrades all (v1 behavior)

@etiktin
Copy link
Contributor Author

etiktin commented Jul 4, 2015

Sounds good 👍

For versions that are satisfied by the current version, the output will look like this?
"bindings" satisfies current dependency (Installed: 1.1.0, Latest: 1.2.1)

Regarding the upgrade all, maybe we can replace -u -a with -ua --upgradeAll, cause the -a won't have any meaning on it's own.

@raineorshine
Copy link
Owner

Yes, and maybe add an additional note to the end:

Run with '-u' to upgrade your package.json
Run with '-ua' to upgrade even those that satisfy the declared version range

@etiktin
Copy link
Contributor Author

etiktin commented Jul 4, 2015

Awesome! :)

Let me know if you need any assistance.

@mathieumg
Copy link

Good stuff! 👍 It all makes sense and is more intuitive!

etiktin pushed a commit to etiktin/npm-check-updates that referenced this issue Jul 10, 2015
raineorshine added a commit that referenced this issue Jul 11, 2015
@raineorshine raineorshine reopened this Jul 15, 2015
@raineorshine
Copy link
Owner

"Merging" discussion from #96. Please continue discussion here.

@raineorshine
Copy link
Owner

(regarding @etiktin's comment)

I like the table display (inspired by salita), and am excited to move forward with that!

I agree that the installed version is nice information to have, and that the exact latest version does not need to be displayed since it is implied by the upgraded version range.

I'm still not a big fan of showing satisfied dependencies, so I'm thinking about what a good compromise might be. I find it unnecessary to modify package.json to change ^1.0.0 to ^1.1.0 when they will ultimately install the same versions. Unnecessary commits, and potentially perpetuates ignorance of the npm update command.

Still, I recognize that people have their reasons for wanting that behavior, and it was confusing for folks when the satisfied dependencies were omitted completely.

I suggest we separate out the satisfied dependencies from the main upgrades:

Dependency Installed Current Upgraded
express 3.10.1 ^3.0.0 ^4.11.0
moment 1.7.2 ^1.7.2 ^2.10.3
The following dependencies are satisfied by the declared version range, but the installed 
version is behind. They can be updated without modifying the dependency by running 
`npm update` or upgraded in the package.json by running npm-check-updates with 
-a/--upgradeAll.
Dependency Installed Current Upgraded
bindings 1.10.0 ^1.10.1 ^1.21.1
async 1.0.0 ^1.0.0 ^1.3.0

We can leave off the verbs since they would be in separate groups. We could perhaps color the entire satisfied table gray to make it clearer that they do not need to be upgraded in the same way.

Thanks for your input!

@etiktin
Copy link
Contributor Author

etiktin commented Jul 15, 2015

It looks good 👍

In my opinion, the main reason to show satisfied dependencies is just so the user will know that there's a newer version than the one currently installed. Displaying it in a separate table with a different color makes sense.
The message is a bit long, but it explains the issue clearly. If the length of the message becomes an issue, perhaps we could show a shorter message and guide the user to nce --help for more detailed information. Something like:

The following dependencies are satisfied by the declared version range. There's
no need to modify them. For more information run `ncu -h`.

What do you think about coloring the range operators in some contrasting color so the version number will pop out?

@raineorshine
Copy link
Owner

I'm in favor of a shorter message as long as it is still clear. Even
shorter:

The following dependencies are satisfied but behind. Run `ncu -h` for

details.

I'm not sure about the colored range operators. Let's get a basic version
working and then tweak the colors as needed.

Looking good! 👍

On Wed, Jul 15, 2015 at 11:45 AM Eran Tiktin notifications@github.com
wrote:

It looks good [image: 👍]

In my opinion, the main reason to show satisfied dependencies is just so
the user will know that there's a newer version than the one currently
installed. Displaying it in a separate table with a different color makes
sense.
The message is a bit long, but it explains the issue clearly. If the
length of the message becomes an issue, perhaps we could show a shorter
message and guide the user to nce --help for more detailed information.
Something like:

The following dependencies are satisfied by the declared version range. There's
no need to modify them. For more information run ncu -h.

What do you think about coloring the range operators in some contrasting
color so the version number will pop out?


Reply to this email directly or view it on GitHub
#94 (comment)
.

@mathieumg
Copy link

This is more than fine! 😄 Always better than just ignoring them.

@raineorshine
Copy link
Owner

I have the colored output table implemented. I opted for a more minimal output that just shows current → upgraded. It's hard to overestimate the beauty of simplicity.

screen shot 2015-07-22 at 2 29 23 pm

The satisfied versions are shown in a separate table as previously discussed:

screen shot 2015-07-22 at 2 30 58 pm

@etiktin and @mathieumg, would love to get your opinion!

Good news is that after this is confirmed, I think I'm finally ready to release v2 on the stable tag. 😅

@mathieumg
Copy link

This looks great! When did you want to release? I've been meaning to finish my work on #83 (which I had working with alpha 10, though not polished), but I've been really busy as of late... I had to fork https://github.com/hughsk/closest-package too, so I'd need to make a PR there as well or roll out my own modified version.

@etiktin
Copy link
Contributor Author

etiktin commented Jul 22, 2015

It looks awesome! 👍
The use of colors really makes the diff pop.

Some things that come to mind:

  • If we don't show the installed field, how do we represent packages that are listed in "package.json" but are not installed?
  • Upgrades (-u or -ua) look the same? Do we show a message at the end of the update that tells the user to run npm update to finish updating the dependencies (with the purple color 😄)?
  • The satisfied message, uses the terms: update and upgrade. I think we should just choose one and stick with it.

Before publishing it on the stable tag, maybe we should use with it for a couple of days and see if anything is missing.

@mathieumg, the issue you mentioned doesn't seem to involve breaking the current V2 API, so I think it would be fine to add it when it's ready.
I was thinking of making a PR for #95 when I have some free time (if it's still open).

Thanks Raine!

@mathieumg
Copy link

@etiktin Good point!

I just realized that for instance only the 3 in 13 gets colored when updating body-parser because the 1 "stays the same", but I wonder if that should be... I feel like it may be better to color the whole 13 regardless.

@etiktin
Copy link
Contributor Author

etiktin commented Jul 22, 2015

I think it's fine to color just what changed as long as we color all the digits that comes after it.
For example:
12 to 13 color 3
1 to 13 color 13
111 to 121 color 21

@mathieumg
Copy link

I think that is the current behavior, I was just poking around saying perhaps it shouldn't be.

@raineorshine
Copy link
Owner

I've been meaning to finish my work on #83...

@mathieumg I'm trying to get all breaking changes into v2, but #83 is an enhancement that can be added as a minor version update any time. Hopefully we can leverage the work you've done already!

I feel like it may be better to color the whole 13 regardless.

@mathieumg Yeah, now that I look at it more, I agree the whole version part should be colored. It looks weird having half of it colored.

If we don't show the installed field, how do we represent packages that are listed in "package.json" but are not installed?

@etiktin I'm wondering why packages would be listed in package.json but not installed, and even if they are, it'll be pretty obvious when you try to start the app. I really value the minimal presentation. I would be open to a "verbose" mode that spells out everything with multiple columns, close to what was originally planned.

Upgrades (-u or -ua) look the same?

What are you referring to here? The message about -ua shows up when there are any packages that satisfy but are behind. Is there somewhere else we need a message?

The satisfied message, uses the terms: update and upgrade. I think we should just choose one and stick with it.

update refers to npm update while upgrade refers to modifying the package.json using npm-check-updates. Maybe a moot distinction though.

Before publishing it on the stable tag, maybe we should use it for a couple of days and see if anything is missing.

Yes, we can wait a few days.

@etiktin
Copy link
Contributor Author

etiktin commented Jul 25, 2015

In #107 I updated the look of the rest of the messages for consistency.

I think the satisfied message is not clear enough. The older version that Raine suggested (the one with the "They can be updated without modifying the dependency...") was clearer even due it was longer. I think we should use that or use the short message that tells the user to ncu -h for more information.

Another thing that I notice is that if you only have satisfied updates, we still show the "Run with -u to upgrade your package.json". I think we shouldn't show it, because it won't do anything in this specific case.

I'm wondering why packages would be listed in package.json but not installed, and even if they are, it'll be pretty obvious when you try to start the app. I really value the minimal presentation. I would be open to a "verbose" mode that spells out everything with multiple columns, close to what was originally planned.

It happens from time to time (e.g. if you accidentally used npm uninstall without --save, so the package is not removed from your package.json). I think the new behavior is fine and there's no reason for a verbose mode (at least not for this specific case). ncu is all about the declaration and not about what's actually installed. If the declaration is not the latest it doesn't matter if the package is installed or not, we will show it in the list of updates.

update refers to npm update while upgrade refers to modifying the package.json using npm-check-updates. Maybe a moot distinction though.

I get that, but I think it might confuse users. Maybe we can just use the term "update" everywhere (including arguments) and make sure that in any relevant message we explain that ncu updates the versions in package.json while npm update updates the installed packages according to the versions in package.json.

By the way, in your screenshots the color used for commands seems more like a type of purple/magenta, but the actual color I see in the source and when I run it, is blue. Did you change it?
I think that magenta looks better, since it has a higher contrast with dark backgrounds.

@raineorshine
Copy link
Owner

Thanks! Pull request look good.

I agree, we can use the word update for everything.

The satisfied wording is almost identical. What do you think needs clarification?

Suggested:

The following dependencies are satisfied by the declared version range, but the installed 
version is behind. They can be updated without modifying the dependency by running 
npm update or upgraded in the package.json by running npm-check-updates with 
-a/--upgradeAll.

Current:

The following dependency is satisfied by its declared version range, but the installed 
version is behind. Update without modifying your package.json by running 
npm update. Upgrade your package.json by running ncu -ua

@raineorshine
Copy link
Owner

Publishing v2 tomorrow. We can always patch if the wording needs to be tweaked.

@etiktin
Copy link
Contributor Author

etiktin commented Aug 1, 2015

The satisfied wording is almost identical. What do you think needs clarification?

The first wording is just more explicit. In the second, I'm not sure it's obvious that updating package.json is unnecessary and that the result (the updates you will get from npm update) are exactly the same.
What do you think about:

The following dependency is satisfied by its declared version range, but the installed version 
is behind. You can update the installed dependency to the latest version without modifying 
your package.json using npm update. If you want to update the dependency in your package.json 
any way, use ncu -ua

We might want to remove "using npm update" to make it a bit shorter (at the end of every upgrade we already mention that you should run npm update to update the installed dependencies).

@raineorshine
Copy link
Owner

Yes! How about:

The following dependency is satisfied by its declared version range, but the installed version
is behind. You can update the installed dependency to install the latest version without modifying
your package.json using npm update. If you want to update the dependency in your package.json
anyway, use ncu -ua/--updateAll.

@etiktin
Copy link
Contributor Author

etiktin commented Aug 3, 2015

👍 I think we have a winner :)

@raineorshine
Copy link
Owner

Published in v2.0.2.

Thanks for all the help getting this nailed down!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants