Closed azrikahar closed 4 years ago
Merging #124 into dev will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## dev #124 +/- ##
=======================================
Coverage 97.18% 97.18%
=======================================
Files 1 1
Lines 71 71
Branches 39 39
=======================================
Hits 69 69
Misses 2 2
Impacted Files | Coverage Δ | |
---|---|---|
lib/module.js | 97.18% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update c002aaa...6d721f4. Read the comment docs.
I was hesitant at first, but I double checked and noticed the test cases includes a trailing slash to the baseURL here:
https://github.com/nuxt/http/blob/c002aaa97c45fa5dfd91325a005fd5cc18a55b57/test/defaults.test.js#L11
So I added another commit that checks for this as well since my original regex will fail for baseURLs with trailing slash.
/^http:\/\/.*:80$/
/^http:\/\/.*:80\/?$/
The \/?
means the forward slash \/
can occur once or zero times, as indicated by the ?
. Sorry for not double checking before making the initial PR.
Hi @azrikahar thanks for PR and nice explanations. I think we only have one issue with current regex which is when baseURL also has a path like (http://foo.com:80/foo
) /^http:\/\/.*:80\/?$/.test('http://foo.com:80/foo')
does not matches for replace. So /^http:\/\/.*:80\/|^http:\/\/.*:80$/
would do the trick (or if you have better idea)
@pi0 thank you for the review :) Can't believe that scenario slipped my mind 🤦♂️
From my understanding, /^http:\/\/.*:80\/|^http:\/\/.*:80$/
means either it is :80/(anything or empty)
or ends with :80
right? I believe we can shorten it to /^http:\/\/.*:80(\/|$)/
(which just uses your |
and shorten it).
Definitely not super experienced in regex so here's additional tests for you to review in case I still miss anything.
Thanks for investigation @azrikahar <3 I merged with /^http:\/\/.*:80(\/|$)/
seems perfect!
This PR fixes #120.
Problem Statement
Following what the issue addresses, the following logic is currently used to remove
:80
when the baseURL starts withhttp://
.https://github.com/nuxt/http/blob/c002aaa97c45fa5dfd91325a005fd5cc18a55b57/lib/module.js#L117-L120
However, this logic will also be true for ANY ports that starts with
:80
, such as:8081
,:8000
and so on since they do technically contain:80
in them.PR Fix
We can use the regex
/^http:\/\/.*:80$/
to make the check stricter, which requires the baseURL to fulfill all the following conditions:http://
(^http:\/\/
:^
means "start of line" and we need to escape/
into\/
).*
:.
means any character and*
means appearing zero or more times):80
only (:80$
:$
means "end of line")Test Results
Test Case 1 - baseURL ends with :80
Both logic works as intended.
Test Case 2 - baseURL ends with :8081
Here we can see the original logic falsely turns
http://localhost:8081
tohttp://localhost81
(as reported by the issue) whereas the regex logic will not trigger as intended.