Regression?

[gohmc]

Hi,

When the following code runs on client side on a remote machine; absoluteUrl returns https despite that the app is just normal http.

const {origin} = absoluteUrl(req) const p1 = ${origin}/api/vehicles console.log('absoluteUrl returned: ' + p1) const p2 = window.location.protocol + '//' + window.location.hostname + (window.location.port ? ':' + window.location.port: '') + '/api/vehicles' console.log('window returned: ' + p2)

Not sure because of index.js line 14 : var protocol = /^localhost(:\d+)?$/.test(host) ? 'http:' : 'https:'

Strange enough, if the next.js app runs on localhost then everything seems fine.

[jakeburden]

@gohmc thank you for opening this issue!

Yes, I can totally see the bug here. Basically, we're checking if the string localhost is in window.location.host, and if so, using http as the protocol. If the host isn't localhost, we're setting the host to https. This behavior is obviously incorrect as you've pointed out!

I think the fix is to replace that regex line with something that checks if the user is in the browser, and then sets the protocol to window.location.protocol

I'm not sure if I'll have time to update this today or tomorrow, but would gleefully accept a pull request to fix this. Otherwise, I'll have some time on Friday to make the fix.

[jakeburden]

@gohmc looking back at the first iteration of this module, we actually did used to use window.location.protocol. Looks like this was a regression!

function absoluteUrl (req, setLocalhost) {
  var protocol = 'https:'
  var host = req ? req.headers.host : window.location.hostname
  if (host.indexOf('localhost') > -1) {
    if (setLocalhost) host = setLocalhost
    protocol = 'http:'
  }  return {
    protocol: protocol,
    host: host
  }
}

module.exports = absoluteUrl

[gohmc]

I think replacing regex with window.location is good enough, this way also remove the assumption that localhost must be http. This is a very handy module, can easily extend with optional callback fn to perform custom logic.

Thanks for your prompt replied!

[jakeburden]

Reopening to set a reminder for myself to revisit this. Won't have time today to implement the fix.