install: count contributors and mention npx thanks #19817

kemitchell · 2018-02-13T22:55:02Z

This small pull request affects console reporting on npm install and friends.

In addition to reporting the number of packages added, the script now also reports the number of unique contributors to those added packages, based on author and contributors data. When the script reports a contributors count, it also prints a message encouraging users to run @feross' npx thanks for more information on how to support contributors to the code they use.

I've also added a TAP test, based on the test for npm install --report.

kemitchell · 2018-02-13T23:35:51Z

Two lingering thoughts:

~~Do I need to worry more about memory usage in compiling the list of unique contributors?~~

Resolved install: count contributors and mention npx thanks #19817 (comment)
~~I couldn't find any existing tests of the CLI install summary output. Did I miss something ... and possibly break it?~~

Resolved install: count contributors and mention npx thanks #19817 (comment)

iarna · 2018-02-13T23:37:58Z

lib/install.js

@@ -774,6 +774,9 @@ Installer.prototype.printInstalledForHuman = function (diffs, cb) {
  var added = 0
  var updated = 0
  var moved = 0
+  // Count the number of contributors to packages added, tracking
+  // contributors we've seen, so we can produce a running unique count.
+  var contributorsSet = []


You can use a Set here and get uniqueness for free

No compat. issue? Then will do.

Yup, we're Node 4+, which includes Set and Map and Symbol!

kemitchell · 2018-02-13T23:43:34Z

@iarna, 476e38fc66b2fa550188c7922e3f4133aedfe005 refactors to use ES2015 Set.

iarna · 2018-02-13T23:43:48Z

Given that we already have all of the package info in memory (far more than just the contrib list) I'm not overly concerned about it.

There aren't tests of ONLY output, but there are a bunch of tests that include output in their assertions. It looks like your change doesn't impact them, however.

The test failure you're seeing is fixed in 30dc1b1 in release-next, so don't worry about it.

iarna · 2018-02-13T23:46:38Z

lib/install.js

+      // Make sure a normalized string for every person behind this
+      // package is in `contributors`.
+      people.forEach(function (person) {
+        // Ignore errors from malformed `author` and `contributors`.


Under what circumstances could normalizePerson throw? To my reading it would only throw if person were undefined or null? If that's the case, then I think it would be better to just skip those cases…

I didn't have a case in mind. Just try'd it out of prudence.

Perhaps a TypeError on String coercion, if person.name or similar is an Object?

kemitchell · 2018-02-13T23:52:18Z

Amended to fix a typo in a comment.

billyjanitsch · 2018-02-14T00:36:23Z

lib/install.js


  output(report)
  return cb()

  function packages (num) {
    return num + ' package' + (num > 1 ? 's' : '')
  }
+
+  function from (num) {
+    return ' from ' + num + ' contributor' + (num > 1 ? 's' : '')


A tiny suggestion:

In the message "installed N packages from M contributors", it's ambiguous whether "from" applies to "packages" or "installed". Read the latter way, it sounds like packages are being installed from multiple places (like multiple contributing registries or something?) which might be confusing for less npm-savvy folks, esp those unfamiliar with the recent Twitter etc. conversations motivating this change.

Is there a way to rephrase? s/from/created by/?

@kemitchell

PR-URL: #19817 Credit: @kemitchell Reviewed-By: @iarna

billyjanitsch · 2018-02-14T01:16:44Z

Oo also: maybe this could wait until the issue of generality is sorted out? I'm extremely excited about thanks, but if it's gonna be recommended by the npm CLI on every additive install, it's a bummer for the donation information to be centralized -- especially when the tool's wording ("You depend on X authors and Y teams who are seeking donations!") suggests that only authors who have explicitly opted in to this userland project require support.

iarna · 2018-02-14T01:26:03Z

I don't see any value in waiting, especially seeing as we're not bundling any particular version of thanks and instead encouraging folks to use the evergreen npx thanks, which means the moment that lands it will become available to users.

iarna · 2018-02-14T01:26:23Z

This PR has been merged to release-next as 737dc85, thanks again! =)

billyjanitsch · 2018-02-14T01:44:06Z

@iarna would you take a PR with the wording tweak?

iarna · 2018-02-14T02:09:08Z

@billyjanitsch and I chatted a bunch more over on Discord:

That wording tweak sounds good (and I'll just make it directly, don't worry about a PR).
There are reasonable concerns about endorsing tools this way. No conclusions as yet.
@iarna would like npx thanks to have some pointer to how you participate in the "please contribute" economy. Also maybe some credit to folks who aren't looking for donations?

feross · 2018-02-15T02:33:01Z

Wow! Just saw this. You all move quickly!

Open to working on the mentioned issues. Is there a log of the Discord discussion somewhere so I can get up-to-speed?

Edit: Found it

kemitchell · 2018-02-15T20:31:53Z

For the record, I work on a different set of tools for contributor support, have criticized Patreon, GratiPay, Liberapay, and their ilk as approaches to the problem, and am still in favor of having npm i mention npx thanks.

@kemitchell

PR-URL: #19817 Credit: @kemitchell Reviewed-By: @iarna

kemitchell requested a review from a team as a code owner February 13, 2018 22:55

iarna reviewed Feb 13, 2018

View reviewed changes

kemitchell force-pushed the thanks branch from 470b805 to a10b751 Compare February 13, 2018 23:41

iarna reviewed Feb 13, 2018

View reviewed changes

kemitchell added 2 commits February 13, 2018 15:46

install: count contributors and mention npx thanks

f283550

install: Use ES2015 Set to count contributors

e569c4e

kemitchell force-pushed the thanks branch from 476e38f to e569c4e Compare February 13, 2018 23:47

iarna approved these changes Feb 13, 2018

View reviewed changes

billyjanitsch reviewed Feb 14, 2018

View reviewed changes

iarna pushed a commit that referenced this pull request Feb 14, 2018

install: count contributors and mention npx thanks

737dc85

PR-URL: #19817 Credit: @kemitchell Reviewed-By: @iarna

iarna closed this Feb 14, 2018

iarna pushed a commit that referenced this pull request Feb 20, 2018

install: count contributors

4d418c2

PR-URL: #19817 Credit: @kemitchell Reviewed-By: @iarna

feross mentioned this pull request Feb 21, 2018

Migrate the functionality to NPM feross/thanks#76

Closed

dependabot-preview bot mentioned this pull request Mar 19, 2018

build: bump npm from 5.5.1 to 5.7.1 lbryio/lbry-desktop#1131

Closed

install: count contributors and mention npx thanks #19817

install: count contributors and mention npx thanks #19817

Uh oh!

Conversation

kemitchell commented Feb 13, 2018

Uh oh!

kemitchell commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iarna Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

kemitchell Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

iarna Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

kemitchell commented Feb 13, 2018

Uh oh!

iarna commented Feb 13, 2018

Uh oh!

iarna Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

kemitchell Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

kemitchell commented Feb 13, 2018

Uh oh!

billyjanitsch Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kemitchell Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

billyjanitsch commented Feb 14, 2018

Uh oh!

iarna commented Feb 14, 2018

Uh oh!

iarna commented Feb 14, 2018

Uh oh!

billyjanitsch commented Feb 14, 2018

Uh oh!

iarna commented Feb 14, 2018

Uh oh!

feross commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kemitchell commented Feb 15, 2018

Uh oh!

Uh oh!

kemitchell commented Feb 13, 2018 •

edited

Loading

billyjanitsch Feb 14, 2018 •

edited

Loading

feross commented Feb 15, 2018 •

edited

Loading