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

feat(config): add in-range #3397

Closed
wants to merge 4 commits into from
Closed

Conversation

wraithgar
Copy link
Member

This adds a flag for npm outdated to use to limit output either to
packages that have updates in-range of the current package's
requirements, or those that have no updates in-range of the current
package's requirements.

It also adds to the normal (i.e. non JSON or parseable) output directing
folks to use npm explain for more info. This is because the output of
npm outdated is misleading, sometimes saying an available version is
present when there is another package in the tree responsible for
keeping the version where it is.

@wraithgar wraithgar requested a review from a team as a code owner June 10, 2021 18:59
This adds a flag for `npm outdated` to use to limit output either to
packages that have updates in-range of the current package's
requirements, or those that have no updates in-range of the current
package's requirements.

It also adds to the normal (i.e. non JSON or parseable) output directing
folks to use `npm explain` for more info.  This is because the output of
`npm outdated` is misleading, sometimes saying an available version is
present when there is another package in the tree responsible for
keeping the version where it is.
lib/outdated.js Outdated
const include =
this.npm.config.get('in-range') === null ||
(this.npm.config.get('in-range') === true && wanted === latest) ||
(this.npm.config.get('in-range') === false && wanted !== latest)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, iiuc, "in-range" means "only show where wanted is latest". My expectation was that it would be "only show where the wanted version is within my dep range". Maybe I'm misreading this?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, like, if the current is 1.0.0, wanted is 1.0.1, but latest is 2.0.0, I'd expect npm outdated --in-range to show that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, in that case, I'd also expect it to show up for npm outdated --no-in-range, because there's an out-of-range version I could install.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted a way to filter between the red and the yellow outputs in pretty print.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ultimately "what could I fix with npm update and what would need a package.json change?"

Copy link
Member Author

@wraithgar wraithgar Jun 10, 2021

Choose a reason for hiding this comment

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

My expectation was that it would be "only show where the wanted version is within my dep range

YES this is the intent. "What could I fix with npm update"
Also related: this is what I assume is being communicated by the red and yellow output colors here.

lib/outdated.js Outdated Show resolved Hide resolved
@darcyclarke darcyclarke added semver:minor new backwards-compatible feature Release 7.x work is associated with a specific npm 7 release labels Jun 10, 2021
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wraithgar
Copy link
Member Author

@ruyadorno I think I'm actually gonna close this, or move it back to draft cause the discussion seems to be warranted here about what problem(s) we're actually solving.

@wraithgar wraithgar marked this pull request as draft June 15, 2021 22:00
@wraithgar wraithgar added the Needs Discussion is pending a discussion label Jun 15, 2021
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Jun 16, 2021
Comment on lines +104 to +108
When running `npm outdated` setting `--in-range` will limit output to only
those packages with existing versions in range of the current project's
dependencies. `--no-in-range` will limit output to only those packages with
updates that are out of range of the current project's dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

so with these options, how do i override NPM_CONFIG_IN_RANGE=false npm outdated (or true)? do i have to do --in-range=null?

Generally a boolean isn't a great choice when there's three options, so perhaps an enum would be clearer here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb see the discussion on the if statements above, it is as you say checking for the default null vs an explicit true or false. I agree it's not ideal and that discussion is why this PR went back to draft status to be discussed and solved as part of a better solution that lets npm update --dry-run solve the "What can be updated problem" first.

@ljharb
Copy link
Contributor

ljharb commented Jun 23, 2021

Per today's RFC call, here's my thoughts:

there's really two mental models - two use cases - that i think people are using npm outdated for.

  1. "show me in-range updates" - this is the one it seems to be primarily designed for, which is basically "what npm update hopefully does". (npm update --dry-run and npm update foo --dry-run would be super useful on its own, precisely because i'm not actually sure what npm update does). This shows me things that are safe to update to.

  2. "show me out-of-range updates" - this is where i'd be likely crossing semver-major lines, and things might break. This is the list of things I likely have to painstakingly and manually work through, looking at changelogs, and updating one at a time, to make sure nothing breaks.

It might be useful for the default view of npm outdated to remain the first case, but add a flag to optimize for the second case?

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label Jun 30, 2021
@wraithgar
Copy link
Member Author

The tracking issue for 'make reify dry run things good' is here #1999

@darcyclarke
Copy link
Contributor

Closing: we are going to queue up npm update --dry-run (ref. #1999)

@darcyclarke darcyclarke closed this Aug 9, 2021
@glen-84
Copy link

glen-84 commented Aug 9, 2021

@darcyclarke Does that solve #3409, which was closed in favour of this PR?

@glen-84
Copy link

glen-84 commented Aug 12, 2021

@wraithgar ?

@wraithgar
Copy link
Member Author

Yes this will fall under that same umbrella. npm outdated will end up being a design layer over the underlying dry run of an update

@wraithgar wraithgar deleted the gar/outdated-in-range branch August 12, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion is pending a discussion Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants