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

RFC: Remove --depth from npm outdated #133

Merged
merged 1 commit into from
May 27, 2020
Merged

Conversation

claudiahdz
Copy link
Contributor

cc. @isaacs

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Release 7.x semver:major backwards-incompatible breaking changes labels Apr 29, 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.

This is good, and we can discuss a bit more in today's RFC call. I think one thing to clarify is what exactly would show in each column.

Presenting some kind of proposal to chew on would be useful. It's especially confusing that "location" here is "the path in the dependency graph to the thing that depends on the node" and not "the location of the node".

I'd suggest each line could be:

  • Package Name of the node
  • Current Node current version
  • Wanted Most up to date version that satisfies each edgesIn Edge
  • Latest Latest version in the packument
  • Location The node.location of the node (not the dependent)

And then group by each combination of wanted+location. So, you might see something like this, which is a little weird, but shows that the deduped package may have to get un-deduped:

Package      Current  Wanted  Latest  Location
bar          1.0.0    1.4.0    2.0.0  node_modules/bar
bar          1.0.0    2.0.0    4.0.0  node_modules/bar

## Rationale and Alternatives
Since `npm update` will go ahead and update everything at any level, maybe there's no need to know in detail which nested dependencies are outdated and we could just show a message that certain top-level dependency has outdated deps.

We could also remove all-together the functionality if people are not paying much attention to outdated nested deps, Yarn only displays top-level outdated packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be worth adding a not here that "depth" doesn't have a clear meaning in the context of a dependency graph over a deduplicated tree, since the same node will often be reachable via multiple different dependency paths. (Ie, the same issue brought up in https://github.com/npm/rfcs/blob/latest/accepted/0019-remove-update-depth-option.md)


In this example, we only care about two versions of `tar`, `4.4.13` and `6.0.2`. Instead we are shown 4 lines of output.

Moreover, npm v6 currently has inconsistent behavior. Doing `npm outdated --depth 9999` displays all outdated packages that are direct children of the root package, missing out dependencies that are nested at other levels. This gives a wrong impression since we are not really displaying _all_ outdated dependencies of the tree. However, doing `npm outdated foo --depth 9999`, will indeed display all appearances of `foo` on the tree no matter if they are direct children of the root package or not. This is precisely the functionality that displaying all deps should have too (`npm outdated` with no flags).
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing npm outdated --depth 9999 displays all outdated packages that are direct children of the root package, missing out dependencies that are nested at other levels. This gives a wrong impression since we are not really displaying all outdated dependencies of the tree.

I don't think that's the case, unless I'm misreading this maybe? npm outdated --depth=9999 includes this output, for example:

log-update                  3.4.0    3.4.0    4.0.0  npm-audit-report > tap > ink
log-update                  3.4.0    3.4.0    4.0.0  npm-audit-report > tap > treport > ink

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 think in that case that particular dependency is being deduped and is indeed a direct child of your root package, dependencies that are not deduped and are nested within their own dependant, do not seem to show up when doing only npm oudated --depth 9999 but DO show up when you do npm oudated --depth 9999 <packageName>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, it's not though. log-update is a dependency of ink, and my package depends on tap, which depends on ink.

$ find node_modules -name log-update
node_modules/tap/node_modules/log-update

It is a good example of the fact that --depth is pretty weird when talking about a dependency graph, though. It looks like I have two outdated versions of log-update, when really they're the same dep node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, correct. The thing is that log-update is a direct child of tap. So I misphrased that by saying the command shows only direct children of the root package when I meant to say that the v6 algorithm is iterating recursively over the node's direct children (on the file system) instead of over node.edgesOut.

Try doing npm outdated --depth 9999 ansi-escapes (basically any specific dep inside node_modules/tap/log-update/node_modules) and you'll see they are not displayed when calling outdated without a package-name.

Using tree.inventory and node.edgesIn on v7 will definitely solve this problem and properly give us a complete list of all outdated packages.

@wesleytodd
Copy link

The addition of --all, as discussed on the RFC call, means a new config option all, which presents a hazard because these are global and would apply to more than just this command. This also came up with the workspace filter flag. We just need to be clear what the goals are for config/flags.

@isaacs isaacs closed this May 1, 2020
@isaacs isaacs reopened this May 1, 2020
@isaacs
Copy link
Contributor

isaacs commented May 1, 2020

Update from RFC call: --long is not a suitable replacement for --all, because it's already used by npm outdated to provide additional information.

Worth considering if we could make npm update continue to only update root level deps, and then have the (current, as of this moment) npm update behavior be triggered by npm update --all. Would definitely justify having the flag on outdated, and provide some nice symmetry, though I think you're more likely to want to update everything that see outdated versions of everything. The fact that you don't have to care about nested deps means "just update it" is fine, and "show me what needs updating" is unnecessary noise most of the time.

@ljharb
Copy link
Contributor

ljharb commented May 1, 2020

Definitely the ideal default for update is "everything", and for outdated is "my direct deps only".

@claudiahdz claudiahdz force-pushed the claudiahdz/outdated-depth branch from d544e60 to b35e667 Compare May 1, 2020 22:07
@ruyadorno
Copy link
Contributor

are there any breaking changes expected for the --json output?

@claudiahdz
Copy link
Contributor Author

@ruyadorno No, the only thing to note is that the location info has changed and is now node.location (its position on the physical tree) instead of its position on the logical dependency tree

@wesleytodd
Copy link

wesleytodd commented May 13, 2020

Just to add here what I bought up on the call, could we display both the logical and physical path?

After continued discussion, removing this concern for now in lieu of better support for this via direct libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants