Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Fix error when trying to publish repo #3

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

sreeramjayan
Copy link
Contributor

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

Wow, what an oversight. Thank you so much for tracking this down and fixing it!

@zkat zkat merged commit 38422d0 into npm:latest Jan 22, 2019
@pvdlg
Copy link

pvdlg commented Jan 24, 2019

@@ -88,7 +88,7 @@ function patchedManifest (spec, auth, base, opts) {
// currently gets filled in by the npm registry itself, based on auth
// information.
manifest._npmUser = {
username: auth.username,
name: auth.username,

Choose a reason for hiding this comment

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

I think the underlying issue is that the conditional that wraps the _npmUser assignment isn't also checking for auth._auth, and if found, splitting the <username>:<password> field appropriately. I can't speak to the correctness of the "username" vs "name" property itself (although I suppose it should be what normalize-package-data does, so yes, name is the correct field AFAICT).

if (auth.username || auth.email || auth._auth) {
  manifest._npmUser = {
    name: auth.username || (auth._auth && auth._auth.split(':').shift()),
    email: auth.email,
  }
}

Copy link

Choose a reason for hiding this comment

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

I don't know if username if recognized by the registry, but I can say for sure that name is because it's the way previous version of npm worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for looking into this y'all. I'm working right this second on adding proper _auth support to npm-registry-fetch, where I think this should live.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants