Create separate types for request and response headers

[sjpgarcia]

Closes #157. This pull request separates the Headers type into RequestHeaders and ResponseHeaders respectively, as discussed in #157 (comment).

The only breakage this introduces is with module imports. The headers and header functions still work on singleton headers, but I've introduced headers' and header' respectively for setting multiple values.

I could also update relevant documentation for this if needed.

[sjpgarcia]

I may have misunderstood the intent of NonEmptyArray String for response headers. Does it mean that something like:

h = headers
  [ Tuple "Set-Cookie" "foo=21"
  , Tuple "Set-Cookie" "bar=21"
  ]

inserts foo=21 and bar=21 into the Set-Cookie key, which would then be rendered as:

Set-Cookie: foo=21
Set-Cookie: bar=21

?

[arthurxavierx]

According to RFC 7230, section 3.2.2:

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

A recipient MAY combine multiple header fields with the same field
name into one "field-name: field-value" pair, without changing the
semantics of the message, by appending each subsequent field value to
the combined field value in order, separated by a comma. The order
in which header fields with the same field name are received is
therefore significant to the interpretation of the combined field
value; a proxy MUST NOT change the order of these field valus when
forwarding a message.

Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
appears multiple times in a response message and does not use the
list syntax, violating the above requirements on multiple header
fields with the same name. Since it cannot be combined into a
single field-value, recipients ought to handle "Set-Cookie" as a
special case while processing header fields. (See Appendix A.2.3
of [Kri2001] for details.)

I'm not entirely sure of how browsers handle this exactly, but it seems like node's implementation doesn't concatenate fields with comma-separated lists and instead renders them as multiple field-value pairs, see nodejs/node#3591 and nodejs/node#14200.

[davezuch]

Thanks for picking this up @arthurxavierx! I have a few questions or comments to discuss below, but overall this looks good!

[davezuch]

Seems like we're missing a way to access array-like headers like set-cookie here. Should we also have a read' that returns maybe a RequestHeaders' (Object (Array String))?

[arthurxavierx]

The problem here is that we just can't know which headers are array-like.

[davezuch]

We can with FFI. That of course requires us to assume a JS backend, but we're already using Node bindings e.g. for setHeaders, so I think we've already made that assumption.

[arthurxavierx]

We could have a function that returns Either String (Array String) for the headers, I guess, is that what you mean?

[davezuch]

Yeah, we have read :: Request -> Object String, we could also have read' :: Request -> Object (Array String). Or we could have a more descriptive name like readArray :: Request -> Object (Array String).

[davezuch]

Well, either change is breaking, it's just breaking in different directions (

Basically, instead of splitting Headers into RequestHeaders and ResponseHeaders, we'd split it into StringHeaders and ArrayHeaders

).

If we go with the direction of this PR, it probably wouldn't be worth it to adopt my suggestion later. It's more a one or the other choice IMO.

But like I said, I don't feel strongly here. It seems a more flexible API, but maybe there's no point in supporting array headers for requests.

[davezuch]

Thinking about it though, my suggestion might not require a breaking change. We just add a new ArrayHeaders type to HTTPure.Headers, and the supporting functions. Or even add a new module for HTTPure.ArrayHeaders (again, not set on the name, maybe MultipleHeaders or HeadersCollection, etc.), and then the current headers continue to work as-is.

[arthurxavierx]

One major problem with this approach is that we'd also have to create HTTPure.Request.RequestArray and HTTPure.Response.ResponseArray variants, as currently HTTPure.Request.Request and HTTPure.Request.Response both have a headers :: HTTPure.Headers.Headers field.

[davezuch]

I think we would just extend them to contain both (I'm now thinking the name could be MultiHeaders instead of ArrayHeaders):

  type Request =
    { method :: Method
    , path :: Path
    , query :: Query
    , headers :: Headers
+   , multiHeaders :: MultiHeaders
    , httpVersion :: Version
    , url :: String
    }

  type Response =
    { status :: Status
    , headers :: Headers
+   , multiHeaders :: MultiHeaders
    , writeBody :: HTTP.Response -> Aff Unit
    }

[arthurxavierx]

Hm I hadn't considered that option. Looks like it should work. Thanks!

[davezuch]

> Run nix-shell --run check-format-purescript
Some files are not formatted:

/home/runner/work/purescript-httpure/purescript-httpure/test/Test/HTTPure/RequestHeadersSpec.purs
Error: Process completed with exit code 1.

Looks like there's a formatting issue in Test.HTTPure.RequestHeadersSpec.

[davezuch]

Closing in favor of #197.