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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use yarn publish for Yarn #220

Merged
merged 3 commits into from Feb 4, 2018
Merged

Use yarn publish for Yarn #220

merged 3 commits into from Feb 4, 2018

Conversation

ErisDS
Copy link
Contributor

@ErisDS ErisDS commented Dec 23, 2017

refs #216, #203, etc

I've genuinely been pulling my hair out with np for the last 3 days. I've done my absolute best to try to contribute a fix because I love this tool, but I'm utterly lost 馃槶

I've tried to fix the npm publish, but I just don't understand how exec/execa/observable are all related.

Instead I've tried to add yarn publish support, but although the publish happens, the flow of control never returns back to listr it seems, so it sits and hangs forever, and never gets as far as pushing the tags. I am at a loss for how to debug.

This PR includes that attempt.

Can someone who understands listr, execa, etc etc take a look and see if they can find an easy fix?

I'd also love to know as well:

  • what does the streamToObservable wrapper in index.js & observableFromPublish code in publish.js achieve?
  • how come in publish.js execa is used directly, although everywhere else it is execa.stdout

If someone can point me at some related reading that would make this make sense I'd be grateful!

@sindresorhus
Copy link
Owner

what does the streamToObservable wrapper in index.js & observableFromPublish code in publish.js achieve?

This chunk https://github.com/ErisDS/np/blob/dbef1d544c7a8f3ccd0e97c43a4c47df0fa58aec/index.js#L16-L24 splits stdout and stderr on newlines and then merges them in one observable that buffers them until it's finished.

how come in publish.js execa is used directly, although everywhere else it is execa.stdout

execa.stdout is just a shorthand for running execa and only getting the stdout output.

index.js Outdated
return 'Private package: not publishing to Yarn.';
}
},
task: () => exec('yarn', ['publish', '--new-version', input]).catch(err => {
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, it doesn't really make sense to run --new-version here, as we already call yarn version --new-version earlier: https://github.com/ErisDS/np/blob/dbef1d544c7a8f3ccd0e97c43a4c47df0fa58aec/index.js#L96

Copy link

Choose a reason for hiding this comment

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

yarn publish wants you to provide an argument for a new version to publish or you'll be prompted for it, so I think this is the only way to avoid the prompt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @gpoole explained, this isn't unnecessary duplication, it's required by yarn. I don't like it either.

I have added the --tag argument back as requested.

@sindresorhus
Copy link
Owner

I don't use Yarn, so I can't be of much help, but if you can find a way to reliably reproduce the issue, we can figure out exactly what caused it.

Maybe someone could help debug this. // @NeekSandhu @jescalan @gpoole @jesstelford

@zikaari
Copy link
Contributor

zikaari commented Dec 27, 2017

I'm a little baby so I might be missing something, but what issue(s) are we having with using stock npm publish that yarn attempts to solve?

I don't use yarn myself, but unless yarn fixes something that npm really can't or has performance edge, adding more moving parts might just make things sophisticated (IMO).

If you can add npm debug log or CLI output that'd be nice. Thanks.

@ErisDS
Copy link
Contributor Author

ErisDS commented Dec 27, 2017

馃憢 I really don't mind whether or not yarn support gets added - it might not be a good idea whilst the yarn publish tool is so young - I was just trying to fix np any way at all and nothing I tried worked :( Even this PR hangs.

For me 2.16.1 works for private packages only - we're using this on all of our internal tools.

2.17+ all have the problem listed in #216 etc where it says "[Current version number] published 馃帀"

Therefore we're pinned to 2.16.1.

Meanwhile, I don't seem to be able to get np to work at all for public packages - i.e. where an npm publish is involved - what-so-ever. I always get an ENEEDAUTH error, even though I am logged in & running npm publish or yarn publish in the same directory works.

Our team uses yarn for dep management and script running now - so np was installed with yarn, we're on Node v6 latest as the default using nvm.

Here's an example:

This is the package I'm working on: https://github.com/TryGhost/eslint-plugin-ghost/tree/ce1a8c44538d401758429eec1f28d6d03b014d20

I have yarn installed using the bash installer: https://yarnpkg.com/en/docs/install#alternatives-tab

If there's any other information I can provide let me know. There seems to be 2 or 3 different problems here, and which ever one try to solve I bash my head against one of the others 馃槥

@zikaari
Copy link
Contributor

zikaari commented Dec 27, 2017

I'd recommend you to try the latest release of np, you'll see that ENEEDAUTH gets caught in prerequisite checks, implemented here.

I cloned your repo named it eslint-plugin-ghost2, played around and turns out running np straight from cli or npm run pub on cli, works no problem, i.e package gets published for real.
But, I see the same ENEEDAUTH error as you when I do yarn pub or yarn run pub. Looks like (not confirmed) the problem might be nvm which does symlink proxying in a weird way, but again I'm not sure.

I guess the verdict is, that this (^) issue is out of np's bounds, more so related to nvm or yarn itself. Further investigation might include testing this on virtual machine without nvm and pure stock node installation instead. In the meantime, please try latest np release and use npm run pub instead.

@gpoole
Copy link

gpoole commented Dec 28, 2017

@NeekSandhu I've been using np 2.16.1 with nvm and haven't had any issues so I think it should definitely be possible.

@ErisDS very strange issue with not being able to publish since it works outside of np :/ When you say private packages do you mean scoped packages or on a private repository? I'm wondering if the contents of npm-debug.log is any use or does it just say the same thing as that screenshot? You could try running with more verbose logs from npm (NPM_CONFIG_LOGLEVEL=silly np) and see if there's any more info? I've had auth issues before but it was a rogue .npmrc file floating around, although that doesn't seem likely if you're able to publish normally.

@zikaari
Copy link
Contributor

zikaari commented Dec 28, 2017

@gpoole Can you check if publish happens if you do yarn pub on CLI and pub is a script defined in package.json with value set to np?

Maybe try both ways, first np installed globally and then also as a local dev dependency.

@gpoole
Copy link

gpoole commented Dec 28, 2017

Oh yep, good point @NeekSandhu, I'm now getting the same issue running it as a script. Looks like yarn sets the script's environment registry config to https://registry.yarnpkg.com/, which messes up npm publish. Looks like adding yarn publish support is going to be the best way to go to get around it.

@ErisDS I tested with your branch and it seems like it should work but for some reason I get error Couldn't publish package.. I get that with a plain yarn publish too though and it does actually seem to publish something, which is weird. Is that what you're seeing?

@ErisDS
Copy link
Contributor Author

ErisDS commented Dec 31, 2017

@gpoole For me, this PR just hangs on the yarn publish step indefinitely. There is no visible error.

@ErisDS
Copy link
Contributor Author

ErisDS commented Dec 31, 2017

When you say private packages do you mean scoped packages or on a private repository?

I meant packages with private:true set in the package.json, which in 2.16.1, results in a prerequisite login check, but the actual publish step isn't needed or run. I think you figured that out though.

If there's something I can do to help track down the weirdnesses let me know - think all the questions so far have answered themselves or I have answered them.

- Fixed linting errors (indent)
- Added --tag support back for yarn publish
@ErisDS
Copy link
Contributor Author

ErisDS commented Feb 1, 2018

I've updated this PR to pass the linter (whoops) and add back the tag option.

I'm running some more tests to ensure it works. Does anyone have any recommendations for good ways to test & debug the code without actually publishing stuff to npm?

@ErisDS
Copy link
Contributor Author

ErisDS commented Feb 2, 2018

OK I found my dumb mistake that prevented this working.

I've also verified that:

  • np works with npm for projects that don't use yarn
  • np works with yarn for projects that do use yarn
  • np --no-yarn works with npm for projects that use yarn but want to publish with npm
  • np --no-yarn --tag=x on yarn project works as expected
  • np --tag=x on yarn project works as expected

I also tested a few other scenarios e.g. setting private:true in package.json and using np from a script in package.json - all looks good 馃榿

P.S. All my manual tests were from this repo to here.

@sindresorhus sindresorhus changed the title Attempt to add yarn publish Use yarn publish for Yarn Feb 4, 2018
@sindresorhus
Copy link
Owner

@ErisDS Happy to see you got it sorted out in the end 馃憣馃槂 Sorry I couldn't be of more help.

@sindresorhus
Copy link
Owner

Does anyone have any recommendations for good ways to test & debug the code without actually publishing stuff to npm?

I just have a test repo and test npm package I use: https://github.com/sindresorhus/sindre-playground

@sindresorhus sindresorhus merged commit c433f7d into sindresorhus:master Feb 4, 2018
@sindresorhus
Copy link
Owner

New np version is out with this.


return exec('yarn', args).catch(err => {
throw new Error(err);
});
Copy link
Owner

Choose a reason for hiding this comment

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

Just noticed this. Why do you catch the error, rewrap it in an error, and throw it again?

@Rowno
Copy link
Contributor

Rowno commented Feb 6, 2018

Just FYI, this completely breaks publishing for people that have two-factor authentication enabled.

@gpoole
Copy link

gpoole commented Feb 8, 2018

Ah I just noticed this but it looks like yarn version is running before yarn publish which can cause problems if you have a version script that makes changes, since yarn publish will run the script a second time. #235 seems to be working for me, if you've got time can you please check that's still ok with your modules @ErisDS?

@oligot
Copy link
Contributor

oligot commented Feb 21, 2018

I also have problems with np using yarn publish

Given that yarn publish is still buggy compared to npm publish (and doesn't support 2FA as pointed by @Rowno), could we reconsider switching back to npm publish ?
Or maybe have a CLI option that allows to use npm publish even for projects that use yarn ?

@ErisDS
Copy link
Contributor Author

ErisDS commented Feb 21, 2018

@oligot if you still want to use npm, then you can use np --no-yarn.

@oligot
Copy link
Contributor

oligot commented Feb 21, 2018

But then, it will use npm instead of yarn to install the dependencies, which is not what I want (yarn to install deps, npm to publish the package).

@ErisDS
Copy link
Contributor Author

ErisDS commented Feb 21, 2018

I know that the project's philosophy is not to add spurious options.

I found that npm publish wasn't working properly with my yarn projects, hence the reason for this PR to get any access to publishing - it's working fine for me with 2.19, but not with the current version as per #248.

I've never used a custom registry, so I'm not seeing what you're seeing. I'd suggest a bug report with full reproduction case would be the best place to start trying to come to a resolution?

@oligot
Copy link
Contributor

oligot commented Feb 21, 2018

I understand that adding an extra option is not the best solution.

Ok, I'll try to do a bug report with full reproduction case.
Thanks for the suggestion 馃槈

@ErisDS ErisDS deleted the yarn-publish branch February 21, 2018 13:27
@oligot
Copy link
Contributor

oligot commented Feb 26, 2018

There is now an open PR on yarn to only check the status code when publishing a package: yarnpkg/yarn#5398

@arcanis
Copy link

arcanis commented Feb 28, 2018

We just merged it, will be part of the next release 馃憤

@oligot
Copy link
Contributor

oligot commented Apr 19, 2018

Just tested with the new release of yarn (1.6.0) and it works now 馃帀

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.

None yet

7 participants