Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create separate types for request and response headers #190
Create separate types for request and response headers #190
Changes from 4 commits
5d63fc2
0920cd0
edd268b
3d4c989
9b12e81
0f82b0b
9967c99
ff7b3c4
e48524e
06514d9
6603688
4975d10
79ed5f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're missing a way to access array-like headers like
set-cookie
here. Should we also have aread'
that returns maybe aRequestHeaders' (Object (Array String))
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that we just can't know which headers are array-like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a function that returns
Either String (Array String)
for the headers, I guess, is that what you mean?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have
read :: Request -> Object String
, we could also haveread' :: Request -> Object (Array String)
. Or we could have a more descriptive name likereadArray :: Request -> Object (Array String)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, either change is breaking, it's just breaking in different directions (
).
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it though, my suggestion might not require a breaking change. We just add a new
ArrayHeaders
type toHTTPure.Headers
, and the supporting functions. Or even add a new module forHTTPure.ArrayHeaders
(again, not set on the name, maybeMultipleHeaders
orHeadersCollection
, etc.), and then the current headers continue to work as-is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One major problem with this approach is that we'd also have to create
HTTPure.Request.RequestArray
andHTTPure.Response.ResponseArray
variants, as currentlyHTTPure.Request.Request
andHTTPure.Request.Response
both have aheaders :: HTTPure.Headers.Headers
field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would just extend them to contain both (I'm now thinking the name could be
MultiHeaders
instead ofArrayHeaders
):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 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I hadn't considered that option. Looks like it should work. Thanks!