RosHTTP icon indicating copy to clipboard operation
RosHTTP copied to clipboard

Better default host/port/protocol when running in browser

Open lj-ditrapani opened this issue 6 years ago • 3 comments

Hello,

I like this library, thanks for creating it!

When running in the browser, would it be reasonable to provide default HttpRequest parameter values based off of document.location if the user does not specify host, port and/or protocol? For example, I find myself writing this code whenever I use roshttp on the client side to make requests back the originating server.

  import roshttp.{HttpRequest, Protocol}

  val baseRequest: HttpRequest = {
    val hostPort: Array[String] = document.location.host.split(":")
    val host: String = hostPort(0)
    val request1 = HttpRequest().withHost(host)
    val request2 =
      if (hostPort.length > 1) {
        request1.withPort(hostPort(1).toInt)
      } else {
        request1
      }
    if (document.location.protocol == "https:") {
      request2.withProtocol(Protocol.HTTPS)
    } else {
      request2
    }
  }

lj-ditrapani avatar Jun 23 '18 15:06 lj-ditrapani

Hi, thanks for the feedback.

How about using the constructor function with a relative url?

HttpRequest("/this-relative-endpoint").send()

hmil avatar Jun 24 '18 21:06 hmil

Thanks for the quick response.

Yeah, I originally tried that and just tried again to make sure in both firefox and chrome. The host ends up being null and the port is not set. So HttpRequest("/status").send() causes the browser to request http://null/status.

The behavior seems consistent with the source. https://github.com/hmil/RosHTTP/blob/master/shared/src/main/scala/fr/hmil/roshttp/HttpRequest.scala#L363-L366 The path & host are set to null and the protocol to HTTP.

https://github.com/hmil/RosHTTP/blob/master/shared/src/main/scala/fr/hmil/roshttp/HttpRequest.scala#L219-L224 Then withURL creates a URI from the relative path. The URI will return null for the scheme/host/port, so those remain set to null.

I'm using "2.1.0" with "sbt-scalajs" % "0.6.23".

lj-ditrapani avatar Jun 25 '18 02:06 lj-ditrapani

Hmm.. That's not right.

We should not allow requests to be constructed with a null host, because then the request's url is garbage.

The "withURL" method needs to change. It should accept urls of the form (protocol:)?//host(:port)?(/path)?(?query)? (absolute, note that protocol can be omitted) and /path(?query)? (relative), but reject others with an exception. (Yes, this is not a comprehensive handling of urls, but we'll let more exotic forms aside).

Additionally, the drivers API should be extended with a method used to provide default values when these can not be found in the URL. Something like:

getDefaultProtocol(): String
getDefaultHost(): String
getDefaultPort(): Integer

In the browser, we can use window.location to figure this out. In server-side environments, defaultProtocol should be HTTPS, while defaultHost and defaultPort should both throw an exception.

PRs welcome, I am not contributing code here anymore.

hmil avatar Jun 25 '18 21:06 hmil