Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Isaacs/install finish #1245

Closed
wants to merge 10 commits into from
Closed

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented May 5, 2020

Big red diff day!

Sorry for the massive drop. The main things going on here:

  • All of the commands that perform an arborist.reify() have been ported to actually use Arborist (except for prune and dedupe that are blocked by Arborist lacking those features for the moment.) That is: install, ci, rm, update, audit, audit fix.
  • A util/reify-output.js module is added, which all of these call to produce consistent output after operation.
  • npm audit is almost where it needs to be, but Arborist.audit() and npm-audit-report need to be made aware that bundled deps can't be fixed with npm audit fix.
  • Interesting change: if there's still an audit issue after running npm audit fix, it'll print a full audit, so you don't have the annoying behavior of running npm audit fix and having it tell you to run npm audit fix again. (Also: it'll only suggest running npm audit fix if there's actually something that can be fixed, and npm audit fix can fix transitive meta-dependency issues no matter how deep!)
  • npm shrinkwrap is updated to use Arborist, so that it will always update the shrinkwrap to the latest and greatest lockfileVersion.

Unfortunately, we still can't fully remove the lib/install/ folder, because it's being used by ls, fund, and util/error-message.js. But almost!

Next up for pieces keeping lib/install/*.js and lib/fetch-package-metadata.js around:

  • implement prune in Arborist
  • implement dedupe in Arborist
  • get npm outdated with arborist #1208 landed
  • implement rebuild in Arborist (or, if not a top-level arborist method, implement it using Arborist.loadActual() and bin-links instead; but it really feels like it might be more under Arborist's charter than the cli's.)
  • port ls to use Arborist.loadActual()
  • port fund to use Arborist.loadActual()
  • remove lib/build.js
  • remove lib/unbuild.js
  • remove or refactor the diff detection in utils/error-message.js so it doens't rely on install/read-shrinkwrap.js

@isaacs isaacs requested a review from a team as a code owner May 5, 2020 00:32
@isaacs
Copy link
Contributor Author

isaacs commented May 5, 2020

implement rebuild in Arborist (or, if not a top-level arborist method, implement it using Arborist.loadActual() and bin-links instead; but it really feels like it might be more under Arborist's charter than the cli's.)

Ok, just had a "how hard could it be? 🤷‍♂️" moment, and took a crack at doing this in cli.

Yeah, no. It needs to be an arborist thing. The thing that's there is kind of broken/incomplete anyway, and I don't want to reimplement half of reify outside of Arborist, that sense makes not any.

@ruyadorno
Copy link
Contributor

ah great, I see it fixes #1234

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.

Just a small comment, otherwise everything LGTM 👍

Notes:

  • Let's add tests later, make sure we also have 100% coverage across the cli
  • Some of the builds are currently broken with an exit signal on install 🤔 is that expected?


if (npm.flatOptions.depth !== Infinity) {
log.warn('update', 'The --depth option no longer has any effect. See RFC0019.\n' +
'https://github.com/npm/rfcs/blob/latest/accepted/0019-remove-update-depth-option.md')
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm! I considered that, but I figured if we ever update it or something, we'd want to have the user see the latest and greatest version, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer linking to the commit as I can see many possible problematic things happening:

  • what if us (or a future team working on this repo) decides to rename branches? latest to master, etc
  • what if someone at a point in future decides to rename the accepted folder?
  • what if that rfc gets withdrawn and/or moved to a diff folder?

Although it seems unlikely in the near-future, I had been bitten by these sorts of changes in the past and I grew to really appreciate linking to the commit blob links instead, just by seeing these things actually happening 😊

but then again I also see that the chance of this message changing before any of the aforementioned happening is also really high 😄

@ruyadorno
Copy link
Contributor

ooohh actually might be a good idea to rebase release/v7.0.0-beta to master, I have tweaked the GH actions since to get all tests running again, including windows builds

@isaacs isaacs force-pushed the release/v7.0.0-beta branch from cd6840b to e57a991 Compare May 8, 2020 01:14
isaacs added 10 commits May 7, 2020 18:18
This adds support for Arborist.audit()
This adds a 'reify-output.js' util, which can be passed any Arborist
object after it reifies a tree.  Consistent output is printed in all
cases, showing the number of packages added/removed/changed, packages
needing funding, and a minimal (but always actionable and relevant)
audit summary.

The only code using the Installer class now is in lib/outdated.js, which
is has a pending update coming soon.

Prune and dedupe commands are awaiting top-level Arborist methods, so
that they can be similarly tightened up. (For now, this commit just has
them fail with a 'coming soon' message.)

The last piece holding the 'install/*.js' code in this repo is that it
is used in 'ls', 'fund', 'shrinkwrap', and the error-message util.
We buffer the output for scripts, and throw it away if the failure is
not something we have to care about.  But if we DO have to care about
it, it's important to show it.

This is a bare-minimum approach.  The error handling stuff here could
use a careful refactor, and it'd be nice if @npmcli/promise-spawn put
something more definitive on the error it returns, so that we didn't
have to duck-type it like this.
@isaacs isaacs force-pushed the isaacs/install-finish branch from 01300dd to 56a688a Compare May 8, 2020 01:20
isaacs added a commit that referenced this pull request May 8, 2020
PR-URL: #1245
Credit: @isaacs
Close: #1245
Reviewed-by: @ruyadorno
@isaacs
Copy link
Contributor Author

isaacs commented May 8, 2020

Landed on release/v7.0.0-beta. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants