Update to use WHATWG URL API instead of url.parse() #793
Conversation
| @@ -46,7 +43,7 @@ class HttpDependencyParser extends RequestParser { | |||
| * Gets a dependency data contract object for a completed ClientRequest. | |||
| */ | |||
| public getDependencyTelemetry(baseTelemetry?: Contracts.Telemetry, dependencyId?: string): Contracts.DependencyTelemetry { | |||
| let urlObject = url.parse(this.url); | |||
| let urlObject = new url.URL(this.url); | |||
There was a problem hiding this comment.
When this.url is used, it is the full url (including protocol) so ignore base.
| const parsed = url.parse(options); | ||
| if (parsed.host === "443") { | ||
| options = url.parse("https://" + options); | ||
| const parsed = new url.URL("http://" + options); |
There was a problem hiding this comment.
The protocol "http://" on this line is in order to make the new URL work - it requires protocol to parse url:
new URL(input[, base])
- input The absolute or relative input URL to parse. If input is relative, then base is required. If input is absolute, the base is ignored.
| @@ -167,13 +165,15 @@ class HttpRequestParser extends RequestParser { | |||
| } | |||
|
|
|||
| var encrypted = (<any>request).connection ? ((<any>request).connection as any).encrypted : null; | |||
| var requestUrl = url.parse(request.url); | |||
|
|
|||
| var protocol = (encrypted || request.headers["x-forwarded-proto"] == "https") ? "https" : "http"; | |||
There was a problem hiding this comment.
Update the sequence to get the protocol first, then URL API can be used - it requires protocol in url.
| telemetry.target = url.parse(telemetry.data).host; | ||
| // url.parse() is deprecated, update to use WHATWG URL API instead | ||
| try { | ||
| telemetry.target = new url.URL(telemetry.data).host; |
There was a problem hiding this comment.
A TypeError will be thrown if the input or base are not valid URLs.
To be compliant with previous behavior, when url is invalid, set telemetry.target as null.
| assert.equal(dependencyTelemetry.data, "https://a.bing.com/search"); | ||
| assert.equal(dependencyTelemetry.target, "a.bing.com"); |
There was a problem hiding this comment.
Remove port because :
When using URL API to create a new object, when port matches with the protocol, the port is dropped.
For example:
- new URL("https://a.bing.com:443/search").port returns "" - port dropped since https matches with 443
- new URL("http://a.bing.com:443/search").port returns "443" - port not dropped
- new URL("http://a.bing.com:80/search").port returns "" - port dropped since http matches with 80
- new URL("https://a.bing.com:80/search").port returns "80" - port not dropped
- new URL("https://a.bing.com:1234/search").port returns "1234" - port not dropped
- new URL("http://a.bing.com:1234/search").port returns "1234" - port not dropped
Confirmed with Nev that : the default port for a protocol is often never sent @hectorhdzg can you confirm this? Thanks.
This PR includes:
url.parse()method.url.parse()instead ofurl.URL()#432Details:
Note:
new URL(input[, base]): detailsinputis relative, thenbaseis required. Ifinputis absolute, thebaseis ignored.TypeErrorwill be thrown if theinputorbaseare not valid URLs.