Fix discrepancy between node url.parse and muon.url.parse #13916
Conversation
Test Plan: 1. npm run test -- --grep='muon tests'
| @@ -365,7 +365,7 @@ module.exports = { | |||
| test.strictEqual(urlUtil().getOrigin('https://abc.bing.com'), 'https://abc.bing.com') | |||
| }, | |||
| 'gets URL origin for url with port': (test) => { | |||
| test.strictEqual(urlUtil().getOrigin('https://bing.com:443/?test=1#abc'), 'https://bing.com:443') | |||
| test.strictEqual(urlUtil().getOrigin('https://bing.com:8000/?test=1#abc'), 'https://bing.com:8000') | |||
There was a problem hiding this comment.
this test was failing in muon because muon gets the origin from GURL.GetOrigin (https://cs.chromium.org/chromium/src/url/gurl.h?q=gurl&sq=package:chromium&l=201), which doesn't return the port if it's 443 for HTTPS or 80 for HTTP
Codecov Report
@@ Coverage Diff @@
## master #13916 +/- ##
==========================================
- Coverage 56.48% 56.48% -0.01%
==========================================
Files 283 283
Lines 28993 28992 -1
Branches 4812 4812
==========================================
- Hits 16378 16377 -1
Misses 12615 12615
|
|
nice 👍 |
| @@ -481,11 +481,12 @@ const UrlUtil = { | |||
| // parsed.origin is specific to muon.url.parse | |||
| if (parsed.origin !== undefined) { | |||
| if (parsed.protocol === 'about:') { | |||
| return [parsed.protocol, parsed.path].join('') | |||
| return [parsed.protocol, parsed.path.replace(/\/.*/, '')].join('') | |||
There was a problem hiding this comment.
Is parsed.path always a string, and never null / undefined? I'm guessing so if the test cases without path are working...
There was a problem hiding this comment.
@petemill this code only runs for muon.url.parse, where parsed.origin is not undefined. in that case, path is always a string.
|
master / 0.24.x: 4ae26c3 On second thought, I'm going to move this back out of 0.23.x since it depends on other changes to the urlutil tests that are only in 0.24.x |
fix #13906
Test Plan:
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests