Add a success message to adduser/login #10903

ekmartin · 2015-12-20T19:10:56Z

Basically just adds a green Authentication successful. message to login/adduser (fixes #10775):

Also refactors some of the adduser tests to have less duplicate code.

othiym23 · 2016-01-15T22:14:22Z

Thanks for putting this together, and thanks for cleaning up the tests!

Two bits of feedback:

Anything using color must observe the --color / --no-color configuration option used elsewhere in the code base. Also, I'm not sure that color is super useful in this case, as it's a simple single line of output. Consider ditching that.
Take a look at the resolution to @zkat's resolution of UX: Add login success message #10775 (comment) – it would be useful to have something like
```
Logged in as othiym23 on https://registry.npmjs.org/.
```
or
```
Logged in as othiym23 to scope @nothingness on https://registry.npmjs.org/.
```
which is a nice visual confirmation of what the user actually did.

If you can work those bits in, we'll get this merged in the next following release. Thanks again for your time!

ekmartin · 2016-01-15T22:34:34Z

That seems better indeed.
Without a scope it now prints:
Logged in as ekmartin on https://registry.npmjs.org/.

With a scope it prints:
Logged in as ekmartin to scope @myco on https://registry.npmjs.org/.

Also removed the color.

othiym23 · 2016-01-15T22:37:56Z

Fantastic! Pending Travis being green, this can be merged to both npm@master and npm@2.x.

ekmartin · 2016-01-15T22:50:22Z

test/tap/adduser-always-auth.js


-      if (remaining === 0) runner.stdin.end()
+test('npm login --scope', function (t) {


@othiym23, maybe the --scope test should check a few other things as well?
Examples:

That @scope:registry is set to the registry URI in the config upon completion.

That the correct registry URI is set when a scope is given.

I.e. this block of code: https://github.com/npm/npm/blob/master/lib/adduser.js#L133-L140 (by the way, what does the multiple argument npm.config.get do here (npm.config.get('registry', 'cli'))?

Could test the @ prefixing of scopes too.

what does the multiple argument npm.config.get do here (npm.config.get('registry', 'cli'))?

npm uses a package called config-chain to set up the precedence hierarchy between the various configuration files, environment variables, and configuration options set as command-line arguments. It's not really widely-known, but you can query one of those sources in particular by passing its name to config.get() as the second parameter, as you see here. The reason for this is that for the specific case of npm login, we don't want a scope set in a config file to trigger the scope-specific behavior for login.

Also, those tests sound like great ideas. You have a few days until we're going to be in a position to merge this change, so if you want to try other approaches and refine the tests, that would be greatly appreciated. Just let us know, so we can re-review before merging. Thanks for your help!

ekmartin · 2016-01-18T00:03:10Z

Added a few tests for --scope in a7f9576.

@zkat

PR-URL: #10903 Credit: @zkat

@iarna

PR-URL: #10903 Credit: @iarna

@iarna

PR-URL: #10903 Credit: @iarna

@iarna

PR-URL: #10903 Credit: @iarna

@iarna

…eamAtomic PR-URL: #10903 Credit: @iarna

@iarna

0.8 http streams have a bug, where if they're paused with data in their buffers when the socket closes, they call `end` before emptying those buffers, which results in the entire pipeline ending and thus the point that applied backpressure never being able to trigger a `resume`. We work around this by piping into a pass through stream that has unlimited buffering. The pass through stream is from readable-stream and is thus a current streams3 implementation that is free of these bugs even on 0.8. PR-URL: #10903 Credit: @iarna

@iarna

PR-URL: #10903 Credit: @iarna

@zkat

Specially on travis -- changing ports prevents that conflict from happening, but we'll need to keep doing it for every test that uses git. PR-URL: #10903 Credit: @zkat

@zkat

PR-URL: #10903 Credit: @zkat

@zkat

PR-URL: #10903 Credit: @zkat

@ekmartin

PR-URL: #10903 Credit: @ekmartin

@ekmartin

PR-URL: #10903 Credit: @ekmartin

@ekmartin

PR-URL: #10903 Fixes: #10775 Credit: @ekmartin Reviewed-By: @othiym23

@ekmartin

PR-URL: #10903 Fixes: #10775 Credit: @ekmartin Reviewed-By: @othiym23

iarna · 2016-01-25T22:31:05Z

This was merged to 3.6.0 & 2.14.16! =)

ekmartin · 2016-01-25T22:33:20Z

Great!

othiym23 added the needs-discussion label Jan 15, 2016

adduser: add a success message

0609eaa

ekmartin force-pushed the adduser_message branch from 6dde2b6 to 0609eaa Compare January 15, 2016 22:34

othiym23 added merge-to-lts merge-to-latest and removed needs-discussion labels Jan 15, 2016

ekmartin reviewed Jan 15, 2016
View reviewed changes

Add tests for npm adduser --scope

a7f9576

zkat added a commit that referenced this pull request Jan 19, 2016

test: Rewrite tests using nock to use other alternatives

bf812a5

PR-URL: #10903 Credit: @zkat

zkat pushed a commit that referenced this pull request Jan 19, 2016

test: remove nock dependency

8a587b0

PR-URL: #10903 Credit: @iarna

zkat pushed a commit that referenced this pull request Jan 19, 2016

test: install-link-scripts, no chmod on write in 0.8

9e84fa4

PR-URL: #10903 Credit: @iarna

zkat pushed a commit that referenced this pull request Jan 19, 2016

test: Rewrite noargs-install-config-save to use common.npm

255be6f

PR-URL: #10903 Credit: @iarna

zkat pushed a commit that referenced this pull request Jan 19, 2016

src: Make fs-write-stream-atomic consistently referred to as writeStr…

6541690

…eamAtomic PR-URL: #10903 Credit: @iarna

zkat pushed a commit that referenced this pull request Jan 19, 2016

test: sorted-package-json: don't stomp on error output

649c193

PR-URL: #10903 Credit: @iarna

zkat added a commit that referenced this pull request Jan 19, 2016

test: git servers take a while to shut down

a9f6fe9

Specially on travis -- changing ports prevents that conflict from happening, but we'll need to keep doing it for every test that uses git. PR-URL: #10903 Credit: @zkat

zkat added a commit that referenced this pull request Jan 19, 2016

test: give a little more info with registry test failure

75ab216

PR-URL: #10903 Credit: @zkat

zkat added a commit that referenced this pull request Jan 19, 2016

test: use --reuseaddr to fix git port conflicts

64995be

PR-URL: #10903 Credit: @zkat

zkat pushed a commit that referenced this pull request Jan 19, 2016

adduser: add a success message

4cb25d0

PR-URL: #10903 Credit: @ekmartin

zkat pushed a commit that referenced this pull request Jan 19, 2016

test: Add tests for npm adduser --scope

2c44d8d

PR-URL: #10903 Credit: @ekmartin

iarna added this to the next milestone Jan 20, 2016

iarna pushed a commit that referenced this pull request Jan 20, 2016

adduser: add a success message

0cafa7d

PR-URL: #10903 Fixes: #10775 Credit: @ekmartin Reviewed-By: @othiym23

iarna pushed a commit that referenced this pull request Jan 21, 2016

adduser: add a success message

f44d8c9

PR-URL: #10903 Fixes: #10775 Credit: @ekmartin Reviewed-By: @othiym23

iarna closed this Jan 25, 2016

ekmartin deleted the adduser_message branch January 25, 2016 22:31

zkat mentioned this pull request Jan 29, 2016

deps: upgrade to npm 2.14.16 nodejs/node#4960

Closed

This was referenced Feb 7, 2016

Update "npm" to version 3.7.2 marekventur/dependency-updater#21

Closed

Update "npm" to version 3.7.2 marekventur/dependency-updater#22

Closed

Update "npm" to version 3.7.2 marekventur/dependency-updater#24

Closed

kevinSuttle mentioned this pull request Apr 21, 2016

UX: Add login success message #10775

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a success message to adduser/login #10903

Add a success message to adduser/login #10903

ekmartin commented Dec 20, 2015

othiym23 commented Jan 15, 2016

ekmartin commented Jan 15, 2016

othiym23 commented Jan 15, 2016

ekmartin Jan 15, 2016

othiym23 Jan 16, 2016

othiym23 Jan 16, 2016

ekmartin commented Jan 18, 2016

iarna commented Jan 25, 2016

ekmartin commented Jan 25, 2016


		if (remaining === 0) runner.stdin.end()
		test('npm login --scope', function (t) {

Add a success message to adduser/login #10903

Add a success message to adduser/login #10903

Conversation

ekmartin commented Dec 20, 2015

othiym23 commented Jan 15, 2016

ekmartin commented Jan 15, 2016

othiym23 commented Jan 15, 2016

ekmartin Jan 15, 2016

Choose a reason for hiding this comment

othiym23 Jan 16, 2016

Choose a reason for hiding this comment

othiym23 Jan 16, 2016

Choose a reason for hiding this comment

ekmartin commented Jan 18, 2016

iarna commented Jan 25, 2016

ekmartin commented Jan 25, 2016