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

[Feat] Postinstall message about repo owner #619

Closed

Conversation

KingDarBoja
Copy link
Contributor

Fixes #465 and also some missing lint errors that were causing the travis build to fail.

The only thing that I don't understand is why the chalk color isn't being applied on the postinstall after using the one from npm while locally it works.

Chalk doesnt work

@raineorshine
Copy link
Owner

Thanks! Could you explain the connection between postinstall and there being a new owner? I'm not sure I understand how they are connected. There is nothing in this PR which checks npm owner ls PACKAGE.

@KingDarBoja
Copy link
Contributor Author

Thanks! Could you explain the connection between postinstall and there being a new owner? I'm not sure I understand how they are connected. There is nothing in this PR which checks npm owner ls PACKAGE.

You are right, the reason with this PR was addresses on the linked issue as someone asked to provide some message if the owner of this repo changed.

I would have to add the extra check if npm owner ls PACKAGE is different that the original one in order to trigger the postinstall message. It will let some room for any other custom message if package owner/maintainers want to display important things like breaking changes and so on, just saying.

@stoically
Copy link
Collaborator

I don't think it makes much sense to work with stdout from (post)install scripts anymore, since it's planned to get removed soonish: npm/rfcs#77 (comment)

@KingDarBoja
Copy link
Contributor Author

I managed to retrieve the list of this package owner as example by using a process child, not sure if it is the ideal way to do it.

Of course we can apply the same logic for every package that is being updated. However, it have a big question about how the message should be displayed as I was thinking on displaying the entire list of packages whose owner changed on the postinstall script but noticed it was a DUMB idea as the author of the original issue was talking about every package being update and not only this one 🤦‍♂

I can provide such functionality and delete the postinstall script if someone helps me to clarify the author needs.

Cheers!

@raineorshine
Copy link
Owner

I think the original issue was referring to dependency owners. If a dependency owner changes, there is a security risk. npm-check-updates is not a runtime dependency so does not have the same risk.

I would suggest the following output if an owner changed:

 htmlparser2              ^3.10.1  →   ^4.0.0
 moment                   ^2.22.2  →  ^2.24.0   *owner changed*
 react-click-n-hold        ^1.0.6  →   ^1.0.7

I have some concern about the performance implications of a separate network request for each package. For this reason I think it should be opt-in.

@stoically
Copy link
Collaborator

Doesn't the owner ls endpoint just give the current owners? Is there a historical endpoint? As otherwise that wouldn't reliably work, I think

@raineorshine
Copy link
Owner

@stoically Indeed, we would need a reliable historical owner endpoint. Or it could be cached locally but that seems fragile.

@KingDarBoja
Copy link
Contributor Author

KingDarBoja commented Jan 4, 2020

I don't think npm has such historical owner endpoint...

EDIT: A possible solution would be comparing the owners array of the current version against the one that ncu will update and if the number isn't equal, we can put the *owner changed* column as suggested.

@stoically
Copy link
Collaborator

Querying npm for a specific package version gives the maintainer information from that very version. So querying all old versions first and comparing that to the new versions might actually work.

@KingDarBoja
Copy link
Contributor Author

KingDarBoja commented Jan 12, 2020

Sounds good to me, I will make the changes, test and share a screenshot if the result is the expected one. After that, I will close this PR and open another in order to keep clean the commit history.

UPDATE Pacote doesn't fetch author, maintainers or colaborators data.

const pacote = require('pacote');

const npmConfig = require('libnpmconfig').read();
// npmConfig.cache = false;

function sample(packageName) {
    return pacote.packument(packageName, npmConfig).then(result => {
        console.log(`Package ${packageName} metadata`);
        for (let key in result.versions) {
            const versionVal = result.versions[key];
            console.log(`Version ${key}`);
            console.log(versionVal);
            if (versionVal.author) {
                console.log(`Author ${versionVal.author.name}`);
            }
            if (versionVal.contributors && versionVal.contributors.length) {
                console.log(`Contributors count: ${versionVal.contributors.length}`);
            }
            if (versionVal.maintainers && versionVal.maintainers.length) {
                console.log(`Maintainers count: ${versionVal.maintainers.length}`);
            }
        }
    });
}

sample('express').then(() => console.log('All good')).catch(reason => console.error(reason.message));

Returns some properties for each version but none of them includes the important ones for this feature. I tried with pacote.manifest without success.

@stoically
Copy link
Collaborator

Setting npmConfig.fullMetadata = true might help.

@KingDarBoja
Copy link
Contributor Author

Thanks mate, that was really helpfull. 🚀

@KingDarBoja KingDarBoja deleted the feat/repo-owner-changed branch January 16, 2020 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue a warning when repository owner is changed
3 participants