Enable 2FA when publishing a new package #346
Conversation
SamVerschueren
force-pushed
the
iss288
branch
from
c00c8b6 to
fd8cb61
Compare
| @@ -79,6 +79,7 @@ process.on('SIGINT', () => { | |||
| { | |||
| ...cli.flags, | |||
| confirm: true, | |||
| exists: !isAvailable, | |||
There was a problem hiding this comment.
I need this, and it is returned from the ui() step, so makes sense to add it here as well.
| @@ -15,7 +15,8 @@ const pkgDir = require('pkg-dir'); | |||
| const hostedGitInfo = require('hosted-git-info'); | |||
| const prerequisiteTasks = require('./prerequisite-tasks'); | |||
| const gitTasks = require('./git-tasks'); | |||
| const publishTaskHelper = require('./publish-task-helper'); | |||
| const publish = require('./npm/publish'); | |||
There was a problem hiding this comment.
I could merge these two into
const {publish, enable2fa} = require('./npm');I don't have a strong opinion, this might be a bit cleaner.
| const {throwError} = require('rxjs'); | ||
| const {catchError} = require('rxjs/operators'); | ||
|
|
||
| const handleNpmError = (error, task, message, executor) => { |
There was a problem hiding this comment.
I extracted this out of the publish-task-helper as it's necessary for every npm command which uses OTP.
source/prerequisite-tasks.js
Outdated
| const versions = JSON.parse(await execa.stdout('npm', ['version', '--json'])); | ||
|
|
||
| if (version.satisfies(versions.npm, '<6.5.0')) { | ||
| throw new Error(`npm@${versions.npm} does not support enabling two-factor authentication on new repositories`); |
There was a problem hiding this comment.
Not sure about the error messages. Should we help the user and tell which version he should install? Currently, only npm@6.5.0 works. npm@6.8.0 will probably work as well, but it's not released yet.
There was a problem hiding this comment.
npm@6.8.0will probably work as well, but it's not released yet.
It is: https://github.com/npm/cli/releases/tag/v6.8.0
I think we should just drop this error message and require npm@6.8.0 for everyone.
There was a problem hiding this comment.
Done, good idea. When I pushed this yesterday, v6.8 wasn't released yet so I had to add this because otherwise it was untestable :).
👍 |
sindresorhus
changed the title
|
Because of the npm version check, the tests fail on Travis. Should we do a |
|
| const versions = JSON.parse(await execa.stdout('npm', ['version', '--json'])); | ||
|
|
||
| if (version.satisfies(versions.npm, '<6.8.0')) { | ||
| throw new Error('Please upgrade to npm@6.8.0 or newer'); |
There was a problem hiding this comment.
Not sure about the error message here...
|
|
Tested this when publishing a new package now and it worked perfectly 👌🎉 |
|
@SamVerschueren You need to submit the PR URL to IssueHunt to claim the bounty. |
|
Why are |
|
@itaisteinherz I agree, it's weird to have it like that. Let's rename |
|
Hi all, I'm using |
|
@mehulkar See #398 (and #424, #427). If you need this ASAP, feel free to open a PR that addresses #398 (comment) and we'll gladly review it and try to get it landed. |
This PR fixes #288 by adding a new step only if the package is new and the package is not private.
I also moved npm specific parts to an
npmdirectory. We might want to do the same forgit? I can revert it but to me it looks cleaner this way.I also noticed that almost every time, the OTP of the publish package will be the same as for the 2FA. So maybe we should just try to re-use the first OTP, if that fails, it will still just ask for a new OTP. But in 99% of the use cases, this would only require to enter the OTP once. What do you think?