-
Notifications
You must be signed in to change notification settings - Fork 675
feat: adds a simple Http
library to Std
#10478
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
base: master
Are you sure you want to change the base?
Conversation
Mathlib CI status (docs):
|
Reference manual CI status:
|
29ae7af
to
be3f20d
Compare
8b58da0
to
3009358
Compare
dde6cfa
to
380dae6
Compare
206ffa3
to
ac37427
Compare
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.
Some preliminary review for the "easy" part of the PR.
/-- | ||
Builds and returns the final HTTP Request with the specified body | ||
-/ | ||
def body (builder : Builder) (body : t) : Request t := |
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.
Should this function force the user to set the Content-Type
header?
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 it's something the HTTP client can infer and add when someone tries to send the request, to prevent it from being malformed.
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 guess for Request
it's kind of moot right now, but it does matter for Response.ok
. Right now, Std.Http.Response.new |>.status .ok |>.text "hi"
returns something different than Std.Http.Response.ok "hi"
and I think that's just confusing.
/-- | ||
Creates a new HTTP POST Request with the specified URI and body | ||
-/ | ||
def post (uri : RequestTarget) (body : t) : Request t := |
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.
This function seems a bit strange to me, because it bypasses the nice helper functions with set the content type for you.
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 it's kinda nice to have a helper when you don't want to use the builder. Instead of writing Request.new |>.uri (parse! "/") |>.text "something"
, you can just use Request.post (parse! "/") "something"
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 agree that this is nice to have, but my point is that the latter will not set the Content-Type
header correctly, whereas the former does.
8c47efc
to
45e2368
Compare
ac37427
to
577dfde
Compare
I don't have a full test case for this, but when I create a response like
then I think the |
It seems to me that there are some problems with timeouts around I have this super simple server: import Std.Internal.Http
def main : IO Unit := do
(Std.Http.Server.serve (Std.Net.SocketAddress.v6 ⟨.ofParts 0 0 0 0 0 0 0 0, 8007⟩) (fun _ => pure <| .ok "hi")).block Then I have this script: #!/bin/bash
while true; do
printf "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"
sleep 2
done When I run the server and do This also reproduces in a browser. If I open This seems to highlight several problems:
|
It's probably a problem with the Timer API, the interaction with Selectable and the reference counting. I'm not sure how to fix this right now but I have a solution that changes a lot of stuff in the timer api :S |
60aeda4
to
4ac2b87
Compare
93182a1
to
2bf41e9
Compare
This PR adds a simple
Http
library toStd
.