saviorand / lightbug_http

Simple and fast HTTP framework for Mojo! 🔥
MIT License
394 stars 27 forks source link

feat: add http client #28

Closed ZacHooper closed 2 months ago

ZacHooper commented 2 months ago

Noticed that someone else recently pushed a commit with an HTTP client. It looks like I've taken a slightly different approach, so making a PR so it's hopefully not wasted effort 😄. Might be able to merge them a bit or just close mine.

Mojo HTTP Client

Haven't really tested beyond get requests, but is working making requests to httpbin.org and google.com currently.

thatstoasty commented 2 months ago

I'm glad some of my work on mojo-http-client was able to help with this!

saviorand commented 2 months ago

Woah, great teamwork! Will review today, I think we can combine this with the other code that was already merged and get an even better implementation 💪

saviorand commented 2 months ago

@ZacHooper made a couple changes to bring this in line with latest state on main and merge with the other client implementation. What do you think? Love the create_connection function. I didn't compare the performance of two implementations though. But the current simple test is passing on both.

ZacHooper commented 2 months ago

Awesome looks good to me! 😄 Ran a couple requests against httpbin and working well.

Did notice one thing though. Not sure if I'm using the HTTPRequest/URI structs wrong but found I needed to add the port to the URL or it caused compiler error.

Good

var req = HTTPRequest(
        URI("http://www.httpbin.org:80/status/300"),
        req_str,
        RequestHeader(req_str),
    )

Bad

var req = HTTPRequest(
        URI("http://www.httpbin.org/status/300"),
        req_str,
        RequestHeader(req_str),
    )
SchmittB commented 2 months ago

The unified code from the two implementation looks good, it is more robust and better looking than my original PR for sure. Congrats !

saviorand commented 2 months ago

@ZacHooper just pushed a fix to properly handle URLs without a port and encode requests. Also added a simple test for external calls. Can you try again? The URI struct needs quite some improvement, haha. Also I realized a decode method for incoming responses would be helpful, not in scope of this PR though. If everything works I'll merge. Thanks a ton!

ZacHooper commented 2 months ago

Yep working great. Much easier to write the request now 😄.

saviorand commented 2 months ago

Perfect! Done merged.