Adds support for native URL parsing if node API is unavailable #149
Conversation
| @@ -1,7 +1,19 @@ | |||
|
|
|||
| var URL = require('url'); | |||
There was a problem hiding this comment.
Maybe the intent would be clearer to rather test if the global URL is set and require it otherwise?
Or we could also rely directly on it since it is available since node@10.
What do you think?
There was a problem hiding this comment.
That makes sense - you'd probably know better than me if there are folks depending on this using ancient node versions, but as long as this came in a major bump IMO it'd be simplest to just use the global URL since it is available in all maintenance+ releases of node
lib/clean-host.js
Outdated
| } | ||
|
|
||
| var parts = URL.parse(url, null, true); | ||
| var parts = parseURL(url); |
Also discovers & resolves a few URL related issues: - WHATWG parses any potentially valid integer string as an IP - add explicit IP detection to avoid this case and preserve existing parsing functionality - WHATWG does not fuzzy parse ipv6 addresses, so add simple regex-based ipv6 extraction to handle this case and preserve existing functionality - WHATWG does not parse scheme-less URLs so change prefixing behavior to include a scheme when falling back to URL-based parsing
jesseditson
force-pushed
the
native-url-parsing
branch
from
d956699 to
2fb0a6c
Compare
|
|
||
| var needsTrimming = checkTrimmingNeeded(url); | ||
| if (needsTrimming) { | ||
| url = url.trim(); | ||
| } | ||
|
|
||
| var needsLowerCase = checkLowerCaseNeeded(url); | ||
| if (needsLowerCase) { | ||
| url = url.toLowerCase(); | ||
| } |
There was a problem hiding this comment.
This diff is a little funky, but I just moved the lowercase check to before the ipv6 check since the later IP checks expect a lowercased string.
| url = '//' + url; | ||
| url = 'https://' + url; |
There was a problem hiding this comment.
WHATWG URL does not support scheme-less URLs so I had to add a scheme to get it to parse.
| if (ipLikeRE.test(parts.hostname) && !ipLikeRE.test(value)) { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
WHATWG URL will parse the string http://42 as the URL http://0.0.0.42, which is not how the legacy URL parser worked (it would fail to parse). This just aligns them. A slight difference in this is that when value is not a string, we will no longer coerce it to one when returning it. I believe this is closer to the documented API but is technically a breaking change. LMK if you'd like me to restore the test and the string-coercion.
|
|
||
| it('should return the initial value if it is not a valid hostname', function(){ | ||
| expect(tld.extractHostname(42)).to.equal('42'); | ||
| expect(tld.extractHostname(42)).to.equal(42); |
There was a problem hiding this comment.
As mentioned above - "return the initial value" feels like it should return an integer, not a string. But this is a change in behavior.
2 participants
Fixes #148