Use port from endpoint url to create HttpRequestOptions: fix for 594 #595
Conversation
limhjgrace
requested review from
qhanam,
limhjgrace and
williazz
and removed request for
qhanam
src/dispatch/DataPlaneClient.ts
Outdated
| @@ -114,6 +114,11 @@ export class DataPlaneClient { | |||
| const options = { | |||
| method: METHOD, | |||
| protocol: this.config.endpoint.protocol, | |||
| port: | |||
| !this.config.endpoint.port || | |||
There was a problem hiding this comment.
Seems like we can simplify lines 117-118. Otherwise, looks great! Thank you
src/dispatch/DataPlaneClient.ts
Outdated
| @@ -114,6 +114,11 @@ export class DataPlaneClient { | |||
| const options = { | |||
| method: METHOD, | |||
| protocol: this.config.endpoint.protocol, | |||
| port: | |||
| !this.config.endpoint.port || | |||
There was a problem hiding this comment.
Is there coverage for this line?
Not sure what the point of !this.config.endpoint.port is.
src/dispatch/DataPlaneClient.ts
Outdated
| port: | ||
| !this.config.endpoint.port || | ||
| isNaN(parseInt(this.config.endpoint.port, 10)) | ||
| ? undefined |
There was a problem hiding this comment.
Is there test coverage for when the port is not a number, and becomes undefined?
There was a problem hiding this comment.
Almost all the other tests are for the use-case where port is not set and I have added one where port is set. I cannot add a case for invalid port because URL class does not allow that.
There was a problem hiding this comment.
I added couple of more tests to specifically test when port is not set and validated it against the code without my change.
This is fix for #594 .
These options are used to create HttpRequest (
aws-rum-web/src/dispatch/DataPlaneClient.ts
Line 76 in 032a9f5
aws-rum-web/src/dispatch/FetchHttpHandler.ts
Line 70 in 032a9f5