fix: use regex to avoid falsely replacing ports starting with 80

[azrikahar]

This PR fixes #120.

Problem Statement

Following what the issue addresses, the following logic is currently used to remove :80 when the baseURL starts with http://.

http/lib/module.js

Lines 117 to 120 in c002aaa

	// Remove port 80 when http
	if (options.baseURL.includes('http://')) {
	options.baseURL = options.baseURL.replace(':80', '')
	}

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:

• Starts with http:// (^http:\/\/ : ^ means "start of line" and we need to escape / into \/)
• contains whatever characters in between (.*: . means any character and * means appearing zero or more times)
• Ends with :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 to http://localhost81 (as reported by the issue) whereas the regex logic will not trigger as intended.

[codecov-commenter]

Codecov Report

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.

[azrikahar]

I was hesitant at first, but I double checked and noticed the test cases includes a trailing slash to the baseURL here:

http/test/defaults.test.js

Line 11 in c002aaa

expect(options.baseURL).toBe('http://localhost:3000/')

So I added another commit that checks for this as well since my original regex will fail for baseURLs with trailing slash.

Original Regex

/^http:\/\/.*:80$/

New Regex

/^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.

[pi0]

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)

[azrikahar]

@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.

Test code

Click to toggle source code

const baseUrls = [
  'http://foo.com:80',
  'http://foo.com:8080',
  'http://foo.com:80/',
  'http://foo.com:8080/',
  'http://foo.com:80/foo',
  'http://foo.com:8080/foo',
]

const rg = /^http:\/\/.*:80(\/|$)/

baseUrls.forEach(baseUrl => {
  console.log('before:', baseUrl)
  console.log('after: ', rg.test(baseUrl) ? baseUrl.replace(':80', '') : baseUrl)
  console.log('---')
})

Test result

[pi0]

Thanks for investigation @azrikahar <3 I merged with /^http:\/\/.*:80(\/|$)/ seems perfect!