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

npm outdated with arborist #1208

Merged
merged 6 commits into from
Jun 5, 2020
Merged

Conversation

claudiahdz
Copy link
Contributor

@claudiahdz claudiahdz commented Apr 25, 2020

Changes

This PR is a refactor of npm outdated with Arborist. It removes the --depth option flag and introduces a new --all one for displaying all outdated dependencies in a tree. The homepage information of a package is now taken directly from the packument instead of the package.json to avoid displaying outdated information. The location info is now the node.location (it's position on the physical tree) instead of it's position on the logical dependency tree.

A new column Depended by was also added on the default display that shows which package depends on the displayed dependency. This makes the information displayed when --all is on easier to understand.

References

http://github.com/npm/rfcs/pull/133

Examples

➜  arborist git:(master) npm outdated
Package                   Current   Wanted   Latest  Location                               Depended by
@npmcli/run-script          1.2.1    1.3.1    1.3.1  node_modules/@npmcli/run-script        arborist
minify-registry-metadata    2.1.0    2.2.0    2.2.0  node_modules/minify-registry-metadata  arborist
npm-package-arg             8.0.0    8.0.1    8.0.1  node_modules/npm-package-arg           arborist
pacote                     11.1.0   11.1.7   11.1.7  node_modules/pacote                    arborist
semver                      7.1.2    7.3.2    7.3.2  node_modules/semver                    arborist
tap                       14.10.6  14.10.7  14.10.7  node_modules/tap                       arborist
tcompare                    3.0.4    3.0.4    5.0.2  node_modules/tcompare                  arborist
treeverse                   1.0.2    1.0.3    1.0.3  node_modules/treeverse                 arborist
➜  arborist git:(master) npm outdated tar
Package  Current  Wanted  Latest  Location                                Depended by
tar        6.0.1   6.0.2   6.0.2  node_modules/tar                        cacache
tar        6.0.1   6.0.2   6.0.2  node_modules/tar                        pacote
tar       4.4.13   6.0.2   6.0.2  node_modules/node-gyp/node_modules/tar  node-gyp

@claudiahdz claudiahdz requested a review from isaacs May 1, 2020 22:28
@claudiahdz claudiahdz marked this pull request as ready for review May 1, 2020 22:30
@claudiahdz claudiahdz requested a review from a team as a code owner May 1, 2020 22:30
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.

There are some ways to lean on the Edge class to do a bit more work here, and we can optimize the async fetches for packuments to go a little faster.

Areas for further improvement (perhaps outside the scope of this PR):

  • What happens with git deps? It'd be nice if we could detect things other than versions being outdated, but also git deps that are not up to date. We'd have to treat the "wanted" as the resolved value, of course, since a lot of the time, those things won't be on the registry, and the "packument" is fake, so we'd need to look at the manifest._resolved rather than manifest.version.
  • Needs to be updated to handle cases where there are multiple edges that limit what a package can be. If you have a deduped top-level dep that can be updated, with another dep depending on its current version, maybe "wanted" would be a lower version in --all mode? I'm not sure the best way to present that. Showing a separate line for each dependent is pretty noisy.

lib/update.js Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Outdated Show resolved Hide resolved
lib/outdated.js Show resolved Hide resolved
@darcyclarke darcyclarke added this to the OSS - Sprint 8 milestone Jun 1, 2020
@darcyclarke darcyclarke added Enhancement new feature or improvement Release 7.x work is associated with a specific npm 7 release labels Jun 1, 2020
@claudiahdz claudiahdz force-pushed the feat/npm-outdated branch 3 times, most recently from 8d8d997 to f17be9a Compare June 4, 2020 21:50
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.

Super minor nit that could be polished to remove the extra name field in the set of objects, and one open question. But the nit really is super minor, and the open question should probably be an RFC anyway (or maybe just a separate issue/PR) since it'd be a deviation from v6 behavior.

LGTM!

columns[5] = type
columns[6] = homepage
}
const tree = await arb.loadActual()
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we might want this to be arb.buildIdealTree() so it finds outdated deps in the virtual tree, falling back to the actual if there's no lockfile. But I wonder if that even makes sense? The current behavior only looks at the actual tree, but it also predates the existence of lockfiles, so I'm not sure how much sense it makes to go based on that.

lib/outdated.js Outdated Show resolved Hide resolved
if (deps.length !== 0) {
// specific deps
for (let i = 0; i < deps.length; i++) {
const nodes = tree.inventory.query('name', deps[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

I was initially a little curious as to why you had that in/out switch in getEdges, but it makes sense seeing this. This is a lot more efficient than looping over every node in the tree!

@claudiahdz claudiahdz merged commit 05e76ef into release/v7.0.0-beta Jun 5, 2020
@claudiahdz claudiahdz deleted the feat/npm-outdated branch June 5, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants