- Notifications
You must be signed in to change notification settings - Fork90
fix: do not use url.URL to support early node 6 and scp-style URLs#64
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
billneff79 commentedFeb 26, 2020
| legacy.auth=whatwg.username||'' | ||
| if(whatwg.password)legacy.auth+=':'+whatwg.password | ||
| // Replace the url decoded username:password with the url encoded username:password from the original url | ||
| legacy.auth=giturl.match(newRegExp('^[^/]+//([^@]+)@'))[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I originally had more checks here for URLs that did not have a protocol or slashes in it, and then I realized I couldn't test that because a URL without a protocol or slashes will never satisfylegacy.auth being truthy, so I removed them.
isaacs commentedFeb 26, 2020
Ah, I'd already landed#62 when I saw this, sorry. I agree, this is a bit of a cleaner fix for legacy v6 node users, but since that node version is only barely supported anyway, I think it's fine either way. Closing this for now, if we get complaints about urlencoding in auth in urls in node v6 less than 6.12, we can revisit it :) |
This gets rid of using url.URL, whichfixes#60 and#61
This in theory could also be applied to the latest branch. While node v6 isn't supported in the latest branch, it may be a more elegant fix to#60 than leaving url.URL in place.