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

Don't attempt to npm publish if package flagged as private in package.json #64

Merged
merged 7 commits into from
Jul 22, 2016

Conversation

unkillbob
Copy link
Contributor

@unkillbob unkillbob commented Jul 19, 2016

Fixes #65

private

We have a bunch of packages that are installed out of private git repos and attempting to use np fails at the second last step as npm publish exits with an error when the package.json specifies private: true.

This pull request makes the change to simply skip the publish step if the package is flagged as private. I wasn't sure whether you'd want this mentioned in the readme or not - let me know if I should update it.

np is really awesome and exactly what we needed, I hope we can get this small change in to adapt it for our use case!

@SamVerschueren
Copy link
Collaborator

np is all about publishing a module. Shouldn't this just be a prerequisite check and exit early? Just wondering.

@@ -116,7 +121,12 @@ module.exports = (input, opts) => {
},
{
title: 'Publishing package',
task: () => exec('npm', ['publish'].concat(opts.tag ? ['--tag', opts.tag] : []))
task: () => getPackageJson()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the load-json-file package here

@unkillbob
Copy link
Contributor Author

np is all about publishing a module. Shouldn't this just be a prerequisite check and exit early?

We still "publish" these modules (we install them from the tag produced by npm version) so need all the other aspects of np (clean, npm install, version bump, push tags, etc).

@jamestalmage
Copy link
Contributor

I think this makes sense.

However, you should use read-pkg-up instead.

Also, it should very clearly state that the publish step was skipped, and why.

private package: publishing skipped

Is it possible to publish private packages on npm if they are namespaced and you have a paid account? If so we may need to add extra logic for that.

@unkillbob
Copy link
Contributor Author

Is it possible to publish private packages on npm if they are namespaced and you have a paid account?

Reading the docs (https://docs.npmjs.com/files/package.json#private) it doesn't look like it. The purpose of private:true is to prevent accidental publishing. If you simply want to change where your package is published to you configure the publishConfig instead.

@jamestalmage
Copy link
Contributor

Yeah, this makes sense then. If private:true, then npm publish will always fail. Relying on git tagging just makes sense then.

As stated - use read-pkg-up, and provide a good message when private mode is encountered. Otherwise 👍

@unkillbob
Copy link
Contributor Author

provide a good message when private mode is encountered

Not really sure how to accomplish this with Listr - are you able to point me in the right direction?

I tried resolving the 'publish' task's promise to the message, I tried exec'ing an echo with the message and I tried a console.log - all happen so fast you don't even see it.

@jamestalmage
Copy link
Contributor

@unkillbob
Copy link
Contributor Author

@jamestalmage yes that's exactly what I need, thanks for raising those issues! In the meantime I've addressed all your other feedback - hopefully this can go ahead without the official skip warning?

@@ -116,7 +114,14 @@ module.exports = (input, opts) => {
},
{
title: 'Publishing package',
task: () => exec('npm', ['publish'].concat(opts.tag ? ['--tag', opts.tag] : []))
task: () => readPkgUp()
Copy link
Owner

Choose a reason for hiding this comment

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

I would use readPkgUp.sync() here. No real benefit of using async fs here as these tasks are done serially.

@sindresorhus
Copy link
Owner

hopefully this can go ahead without the official skip warning?

Sure, just open an issue with the remaining work so we don't forget.

@sindresorhus
Copy link
Owner

Looks great. Could you add a note to the readme about the behavior? This might be useful for other people to, but I don't think they would realize it's possible without some kind of indication.

@unkillbob
Copy link
Contributor Author

Skipping tasks is hopefully not too far away (SamVerschueren/listr#17), happy to hold out for that - this isn't super urgent for us.

@jamestalmage jamestalmage merged commit 4a5a090 into sindresorhus:master Jul 22, 2016
@jamestalmage
Copy link
Contributor

Skipping tasks is hopefully not too far away (SamVerschueren/listr#17), happy to hold out for that

We can follow up once that lands.

Great work. Thanks!

@sindresorhus
Copy link
Owner

Excellent work on this @unkillbob :) Cheers.

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.

4 participants