Feat: repo owner changed option #621

KingDarBoja · 2020-01-16T06:10:29Z

Fixes #465. Thanks @stoically for helping me with the packument stuff.

Unfortunately, I don't have any screenshot at the moment to showcase the new owner changed column, as I have no idea which npm package got author changed.

To test it out, I had to tweak this library and use verdaccio for local publish in order to check the behaviour of the packument author data between two versions.

stoically · 2020-01-16T10:49:26Z

lib/npm-check-updates.js

+        const to = versionUtil.colorizeDiff(mappedArgs.from[dep], mappedArgs.to[dep] || '');
+        let owner = '';
+        if (options.ownerChanged) {
+            owner = mappedArgs.owner && mappedArgs.owner[dep] ? '*owner changed*' : '';


Maybe it might make sense to list the actual changed owners? Otherwise looking that up manually could be hard.

How could that be displayed on the table? 🤔

I somewhat prefer *owner changed*. Listing the actual changed owners would make sense in --loglevel verbose if we were to expand results beyond a single line.

Agreed, that might make the whole thing noisy. Maybe additionally a "some owners changed while checking for updates, execute with --loglevel verbose to see the full list" at the end of the listing in case something changed?

On the other hand, since currently getting the owner changed information is already opt-in, wouldn't that be enough intent to actually get the owner information as well?

Another option that I think would be reasonable would be to add something like --full which would be expected to break the single-line formatting and would allow room for additional information to be added in the future, as suggested.

That could get interesting if multiple owners get added and removed (long line). Also there might be a problem with accessibility (color blind) if owners are only added or removed (no arrow, or arrow pointing to / originating from nothing).

But

htmlparser2 ^3.10.1 → ^4.0.0 Owner added: foo, bar, baz removed: baz, bar, foo License changed from: MIT to: GPL moment ^2.22.2 → ^2.24.0 react-click-n-hold ^1.0.6 → ^1.0.7

isn't exactly pretty 😬

Another option that I think would be reasonable would be to add something like --full

So, leaving *owner changed* as is and showing more information with the --full flag?

isn't exactly pretty 😬

That's not so bad for the full view.

So, leaving owner changed as is and showing more information with the --full flag?

That's what I'm leaning towards.

@stoically I'm going to get this PR merged as-is, but I'm open to enhanced functionality in a separate PR!

lib/npm-check-updates.js

lib/package-managers/npm.js

lib/package-managers/pacotetest.js

lib/versionmanager.js

package.json

raineorshine · 2020-01-17T06:12:45Z

lib/npm-check-updates.js

+        const to = versionUtil.colorizeDiff(mappedArgs.from[dep], mappedArgs.to[dep] || '');
+        let owner = '';
+        if (options.ownerChanged) {
+            owner = mappedArgs.owner && mappedArgs.owner[dep] ? '*owner changed*' : '';


I somewhat prefer *owner changed*. Listing the actual changed owners would make sense in --loglevel verbose if we were to expand results beyond a single line.

lib/npm-check-updates.js

lib/package-managers/npm.js

lib/npm-check-updates.js

raineorshine

See comments

lib/package-managers/npm.js

raineorshine · 2020-05-14T21:39:09Z

lib/package-managers/npm.js

+    const result = await pacote.packument(packageName, npmConfig);
+    if (result.versions) {
+        let pkgVersions = Object.keys(result.versions);
+        const current = semver.maxSatisfying(pkgVersions, currentVersion);


I think you want to use currentVersion as-is rather than getting maxSatisfying. If the owner changed on a patch version in between currentVersion and current, it would not detect the owner change.

Thinking about the bug after removing the maxSatisfying, I tested the counterpart function (minSatisfying) and I think it does what I wanted for both current and latest version:

Return the lowest version in the list that satisfies the range, or null if none of them do.

So if Mocha current is ^7.0.1, the minimum version will be 7.0.1 whereas if latest is 8.0.0 or ^8.0.0, it will return 8.0.0. For cases where the version doesn't exist, i.e. current being 8.2.1/ ^8.2.1 and latest being 9.0.0, it will fail as minSatisfying will be unable to find a version to match these.

One image is better than 1000 words.

I can confirm that currentVersion is a range, not an exact version as I suggested in my initial comment.

I changed it to use minSatisfying on currentVersion and maxSatisfying on upgradedVersion. This ensures that the broadest change is covered in case the owner changed anywhere between the two.

bin/ncu

lib/npm-check-updates.js

raineorshine

I am currently getting the following error with some packages. You can run ncu -o on npm-check-updates itself or try mocha as an example.

Question is, why is _npmUser not defined on some packages? If it is not guaranteed, how reliable will this feature be? We might have to print something like *unknown* to avoid a false positive.

$ ncu -o
Checking /Users/raine/projects/npm-check-updates/package.json
[====================] 34/34 100%
/Users/raine/projects/npm-check-updates/lib/npm-check-updates.js:437
  throw err
  ^

TypeError: Cannot read property '_npmUser' of undefined
    at Object.packageAuthorChanged (/Users/raine/projects/npm-check-updates/lib/package-managers/npm.js:68:54)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /Users/raine/projects/npm-check-updates/lib/versionmanager.js:274:28
    at async Promise.all (index 0)
    at async Object.getOwnerPerDependency (/Users/raine/projects/npm-check-updates/lib/versionmanager.js:270:3)
    at async /Users/raine/projects/npm-check-updates/lib/npm-check-updates.js:239:16
    at async getAnalysis (/Users/raine/projects/npm-check-updates/lib/npm-check-updates.js:486:24)

lib/package-managers/npm.js

KingDarBoja · 2020-07-18T14:38:20Z

Tested against mocha with current version being ^7.2.0 and latest being 8.0.1 and checked all version list _npmUser to see what is going on. Noticed that Mocha has _npmUser on any version until now, so the owner in theory never changed but the issue is caused by current having a wildcard.

raineorshine · 2020-07-20T14:21:35Z

I'm sort of following. Do you need assistance or do you have a solution? I can try to understand better if you are in need of support.

KingDarBoja · 2020-07-20T15:38:35Z

I'm sort of following. Do you need assistance or do you have a solution? I can try to understand better if you are in need of support.

Kinda both, I explained it on one of the above comments what I think should be done:

async function packageAuthorChanged(packageName, currentVersion, latestVersion) {
  npmConfig.fullMetadata = true

  const result = await pacote.packument(packageName, npmConfig)
  if (result.versions) {
    const pkgVersions = Object.keys(result.versions)
    const current = semver.minSatisfying(pkgVersions, currentVersion)
    const latest = semver.minSatisfying(pkgVersions, latestVersion)
    if (current && latest) {
      if (result.versions[current]._npmUser && result.versions[latest]._npmUser) {
        const currentAuthor = result.versions[current]._npmUser.name
        const latestAuthor = result.versions[latest]._npmUser.name
        return !_.isEqual(currentAuthor, latestAuthor)
      }
    }
  }
  return null
}

Replace at the previous replit code and see what happens with Mocha.

raineorshine · 2020-07-20T16:27:16Z

That should work. Do you think we should print something like *unknown* in those cases where no comparison can be made? Otherwise it might be possible to have a false positive.

KingDarBoja · 2020-07-20T16:29:15Z

That should work. Do you think we should print something like *unknown* in those cases where no comparison can be made? Otherwise it might be possible to have a false positive.

Of course, that's why I am returning null, to check if it is null before displaying at the table. If it is null, then show unknown :) that's atleast what I have in mind.

Also, I would add some testing to cover this feature so we don't end up with some edge cases.

raineorshine · 2020-07-20T16:30:05Z

Great, that sounds good.

Note that you will also need to check result.versions[current] && result.versions[latest] otherwise we get the Cannot read property '_npmUser' of undefined error.

KingDarBoja · 2020-07-20T17:02:03Z

Great, that sounds good.

Note that you will also need to check result.versions[current] && result.versions[latest] otherwise we get the Cannot read property '_npmUser' of undefined error.

Too bad we can't use the optional chaining operator here 😢

KingDarBoja · 2020-08-22T16:23:53Z

@raineorshine Sorry for the delay, been busy with some real life stuff but managed to update the branch and provide three basic test for this feature, I will be pushing in a few minutes these changes.

Cheers!

raineorshine · 2020-08-24T13:44:35Z

Thanks! I will review this week.

raineorshine · 2020-08-29T01:51:48Z

Okay, I believe it's ready to merge!

I just made some small changes in 7f08648:

Only print ownerChanged status when --ownerChanged is specified
Fix issue with getOwnerPerDependency that was not returning the correct object
Minor refactor, renames, convert to functional style

KingDarBoja · 2020-08-29T02:05:05Z

Finally! After few months and some lazyness from me, this will be merged 🎉

KingDarBoja requested a review from raineorshine January 16, 2020 06:10

stoically requested changes Jan 16, 2020

View reviewed changes

stoically added the feature label Jan 16, 2020

Add owner changed opt-in flag

a5d7163

KingDarBoja force-pushed the feat/repo-owner-column branch from 9517636 to a5d7163 Compare January 17, 2020 04:16

KingDarBoja requested a review from stoically January 17, 2020 04:18

raineorshine requested changes Jan 17, 2020

View reviewed changes

Refactoring of owner data per dependency

4e46c18

KingDarBoja requested a review from raineorshine January 19, 2020 17:17

raineorshine requested changes Jan 26, 2020

View reviewed changes

raineorshine removed the feature label May 8, 2020

KingDarBoja added 2 commits May 10, 2020 17:11

Merge branch 'master' into feat/repo-owner-column

0bae343

Check max version match for the passed versions

17ff16b

raineorshine requested changes May 14, 2020

View reviewed changes

KingDarBoja added 5 commits June 12, 2020 20:27

Add ownerChanged cli option to readme

6995040

Avoid passing options to dependency table function

dcfc1b0

use npmUser rather than author on semver check

1c37368

Merge branch 'master' into feat/repo-owner-column

3d782b0

Fix linter errors due to merge

25e3950

KingDarBoja requested a review from raineorshine June 13, 2020 02:18

raineorshine requested changes Jun 15, 2020

View reviewed changes

lib/package-managers/npm.js Outdated Show resolved Hide resolved

KingDarBoja added 2 commits July 9, 2020 08:25

Merge branch 'master' into feat/repo-owner-column

d12015f

Merge branch 'master' into feat/repo-owner-column

083f9e9

raineorshine force-pushed the master branch from ad1aa6c to 267116a Compare August 21, 2020 22:53

Merge branch 'master' into feat/repo-owner-column

31b684a

tests: add packageAuthorChanged tests

947430c

KingDarBoja requested a review from raineorshine August 22, 2020 16:37

raineorshine added 3 commits August 28, 2020 16:28

Merge branch 'master' into feat/repo-owner-column

7f3c000

Add --ownerChanged to CLI options.

3c4db9f

ownerChanged: Only print with -o, fix getOwnerPerDependency, refactor.

7f08648

raineorshine approved these changes Aug 29, 2020

View reviewed changes

raineorshine removed the request for review from stoically August 29, 2020 01:52

raineorshine merged commit 8851b6b into raineorshine:master Aug 29, 2020

KingDarBoja deleted the feat/repo-owner-column branch August 29, 2020 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: repo owner changed option #621

Feat: repo owner changed option #621

KingDarBoja commented Jan 16, 2020

stoically Jan 16, 2020

KingDarBoja Jan 17, 2020

raineorshine Jan 17, 2020 •

edited

Loading

stoically Jan 17, 2020

stoically Jan 17, 2020

raineorshine Jan 17, 2020

stoically Jan 17, 2020

stoically Jan 17, 2020

raineorshine Jan 17, 2020

raineorshine Aug 29, 2020

raineorshine Jan 17, 2020 •

edited

Loading

raineorshine left a comment

raineorshine May 14, 2020

KingDarBoja Jul 18, 2020

KingDarBoja Jul 18, 2020

raineorshine Aug 29, 2020

raineorshine left a comment

KingDarBoja commented Jul 18, 2020 •

edited

Loading

raineorshine commented Jul 20, 2020

KingDarBoja commented Jul 20, 2020

raineorshine commented Jul 20, 2020

KingDarBoja commented Jul 20, 2020 •

edited

Loading

raineorshine commented Jul 20, 2020 •

edited

Loading

KingDarBoja commented Jul 20, 2020

KingDarBoja commented Aug 22, 2020

raineorshine commented Aug 24, 2020

raineorshine commented Aug 29, 2020

KingDarBoja commented Aug 29, 2020

Feat: repo owner changed option #621

Feat: repo owner changed option #621

Conversation

KingDarBoja commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine left a comment

Choose a reason for hiding this comment

KingDarBoja commented Jul 18, 2020 • edited Loading

raineorshine commented Jul 20, 2020

KingDarBoja commented Jul 20, 2020

raineorshine commented Jul 20, 2020

KingDarBoja commented Jul 20, 2020 • edited Loading

raineorshine commented Jul 20, 2020 • edited Loading

KingDarBoja commented Jul 20, 2020

KingDarBoja commented Aug 22, 2020

raineorshine commented Aug 24, 2020

raineorshine commented Aug 29, 2020

KingDarBoja commented Aug 29, 2020

raineorshine Jan 17, 2020 •

edited

Loading

raineorshine Jan 17, 2020 •

edited

Loading

KingDarBoja commented Jul 18, 2020 •

edited

Loading

KingDarBoja commented Jul 20, 2020 •

edited

Loading

raineorshine commented Jul 20, 2020 •

edited

Loading