fix: use URL instead of url.parse #141
Conversation
wraithgar
force-pushed
the
gar/git_ssh_url
branch
from
4470605 to
ea724ae
Compare
| res.fetchSpec = parsedUrl.toString() | ||
| } | ||
| if (res.fetchSpec.startsWith('git+')) { | ||
| res.fetchSpec = res.fetchSpec.slice(4) |
There was a problem hiding this comment.
protocol is not able to be changed in a URL instance like it was in a url.parse result.
There was a problem hiding this comment.
I think that might be related to nodejs/node#49319 - it's supposed to be changeable.
There was a problem hiding this comment.
file: is a special spec so changing from git+file: (non special) to file: (special) appears to be forbidden.
There was a problem hiding this comment.
Indeed, it seems that the spec forbids it, but all browsers allow it, which means the spec will likely change to allow it.
There was a problem hiding this comment.
Browsers do not obey the spec and also break in fun ways when you do change the spec
Firefox:
x = new URL("git+file://a")
URL { href: "git+file://a", origin: "null", protocol: "git+file:", username: "", password: "", host: "", hostname: "", port: "", pathname: "//a", search: "" }
x.toString()
"git+file://a"
x.protocol = "file:"
"file:"
x.toString()
"file:///" Node silently ignores the setter with no warnings or errors (which I suppose is to spec since it just says return).
node.js
> x = new URL('git+file://a')
URL {
href: 'git+file://a',
origin: 'null',
protocol: 'git+file:',
username: '',
password: '',
host: 'a',
hostname: 'a',
port: '',
pathname: '',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
> x.toString()
'git+file://a'
> x.protocol = 'file:'
'file:'
> x.protocol
'git+file:'
> x.toString()
'git+file://a'There was a problem hiding this comment.
lol, that is fun. the .protocol accessor correctly updates tho, even if the double slashes and default port behavior ends up being inconsistent.
(obv none of this helps this particular migration)
| } | ||
| delete urlparse.hash | ||
| res.fetchSpec = url.format(urlparse) | ||
| parsedUrl.hash = '' |
There was a problem hiding this comment.
delete does not work on the URL instance's hash attribute like it did with url.parse
| } else { | ||
| setGitCommittish(res, urlparse.hash != null ? urlparse.hash.slice(1) : '') |
There was a problem hiding this comment.
the hash attribute of a URL instance is always set to a string object.
lib/npa.js
Outdated
| } | ||
| } else if (rawSpec.startsWith('git+file://')) { | ||
| // URL can't handle windows paths | ||
| const noProtocol = rawSpec.slice(11).replace(/\\/g, '/') |
There was a problem hiding this comment.
This is leftover from when I was also trying to get the drive letter stuff fixed here. That ended up not being needed so stripping the protocol here is unneeded as it does not ever contain backslashes.
It does more than set one attribute, and shouldn't need to return res if it' mutating in place
wraithgar
force-pushed
the
gar/git_ssh_url
branch
from
ea724ae to
ef82ddf
Compare
There was a problem hiding this comment.
It would be interesting to see what sort of overlap there could potentially be with the url parsing in hosted-git-info (particularly https://github.com/npm/hosted-git-info/blob/1cfe4e8bb2c9373eeb84f2d94ddad6abc000f6f8/lib/parse-url.js)
No description provided.