feat(client): refactor createSocketUrl to make it better unit tested #2336
Conversation
nougad
requested review from
alexander-akait,
hiroppy and
knagaitsev
as code owners
| // strip leading `?` from query string to get a valid URL | ||
| .substr(1) | ||
| // replace first `&` with `?` to have a valid query string | ||
| .replace('&', '?'), |
There was a problem hiding this comment.
The full URL is something like http://example.com/?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost while this method only gets the path: ?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost
This will convert the path into a full url by 1) removing the leading ? and 2) replace the first & with a ?. The result is a valid URL which can be parsed: url.parse('http://0.0.0.0:8096?sockPort=8097&sockHost=localhost')
The removal of the leading ? was already done before. The difference is the replace of & to get a valid URL. That allows to use urlParts.query instead of before querystring.parse(urlParts.path).
I agree the parsing and replacement is a bit funky but I also don't get why the parameter is a invalid url instead of just passing the host as a normal query parameter.
There was a problem hiding this comment.
@nougad what about(rewrite) fix it in other PRs?
There was a problem hiding this comment.
Not sure how to fix this because the actual problem comes from a problematic protocol to signal the sock endpoint. I don't think I have enough knowledge about the details to change the protocol.
| } else { | ||
| // Else, get the url from the <script> this file was called with. | ||
| const scriptHost = getCurrentScriptSource(); | ||
| urlParts = url.parse(scriptHost || '/', false, true); | ||
| urlParts = url.parse(scriptHost || '/', true, true); |
There was a problem hiding this comment.
Why we need parse query string here?
There was a problem hiding this comment.
This is related to the comment above. Having a full URL including parsed query string saves from another querystring.parse(urlParts.path) later on.
Codecov Report
@@ Coverage Diff @@
## master #2336 +/- ##
==========================================
+ Coverage 93.88% 93.94% +0.06%
==========================================
Files 34 34
Lines 1291 1289 -2
Branches 368 367 -1
==========================================
- Hits 1212 1211 -1
+ Misses 78 77 -1
Partials 1 1
Continue to review full report at Codecov.
|
|
Thanks! |
Motivation / Use-Case
Follow up from #2303 to add more tests