This sounds reasonable, but definitely needs a test to be accepted.

My concern is ensuring that this doesn't cause any problems with how pacote et al make the request for the module. Do you have an example where this currently fails in the CLI, so that we can ensure it is fixing a real problem? If it currentlydoesn't fail in the CLI, then that means it probablywill fail with this change (which might still be reasonable, but means that other changes will be required to integrate the version of hosted-git-info/npm-package-arg that includes this change, making it semver-major).

For npm v7, we've been moving towards using the WhatWG-style urls throughout the CLI. I'm assuming that might change the approach here, since escaped auth would be the default? If it turns out to be a semver-major change, we may as well go all the way and use the new parsing style, especially if it's not possible (or just not worth the effort) to pull this in until npm v7 anyway.

Test added, along with fix for edge-cases where only usernameor password is provided, not both.

Yes, I encountered the problem in the CLI and tracked the error via pacote and npa through to this module.

With apackage.json file similar to the following...

{"dependencies": {"my-dependency":"https://my-username:p%40ssword@github.com/my-username/my-dependency.git"  }}

...the CLI fails with an error similar to the following...

npm ERR! Error while executing:npm ERR! /usr/bin/git ls-remote -h -t https://my-username:p@ssword@github.com/my-username/my-dependency.gitnpm ERR! npm ERR! fatal: unable to access 'https://ssword@github.com/my-username/my-dependency.git/': Could not resolve host: ssword@github.comnpm ERR! npm ERR! exited with error code: 128

With my fix, I am able to runnpm install successfully with no impact (that I could find) on unauthenticated URLs or authenticated URLs without special characters. Therefore I believe this is a bugfix,not a semver-major change.

For npm v7, bear in mind that the newer URL API doesn't provide an.auth property on parsed URLs. You'll have to build the auth from.username and.passwordthe same way I did here; but yes, it will be escaped by default (including in.href).

Thanks for looking into that@stevenhilder! It looks like this is reasonable to try to get into the next npm/cli v6 release as a bugfix then.