Skip to content
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

Fix socket #235

Merged
merged 15 commits into from
Jan 30, 2025
Merged

Fix socket #235

merged 15 commits into from
Jan 30, 2025

Conversation

thatstoasty
Copy link
Collaborator

No description provided.

@thatstoasty thatstoasty requested a review from saviorand January 24, 2025 00:41
lightbug_http/client.mojo Outdated Show resolved Hide resolved
lightbug_http/pool_manager.mojo Outdated Show resolved Hide resolved
lightbug_http/socket.mojo Show resolved Hide resolved
@thatstoasty
Copy link
Collaborator Author

thatstoasty commented Jan 25, 2025

I'm moving stuff to URI and also reworking the uri parsing to be a bit more thorough. Do you know what uri.request_uri is supposed to represent? Not sure exactly what request_uri is. It seems to encompass the path, the query string, and potential fragments?

I just changed it to be the full request uri, as in the original uri, since that made more sense given the name. But some guidance would help!

@thatstoasty thatstoasty marked this pull request as draft January 25, 2025 18:40
@saviorand
Copy link
Collaborator

@thatstoasty request_uri was originally supposed to be the uri from the first line of the http request, not 100% sure we stick to that though. We can change it no problem if you have a better solution, it's not important that it stays that way

@thatstoasty
Copy link
Collaborator Author

Gotcha, I'm working on the URI parsing right now so I'll fix the request_uri so it's back to what it was soon 👍🏾

@thatstoasty thatstoasty marked this pull request as ready for review January 28, 2025 15:31
@thatstoasty thatstoasty marked this pull request as draft January 28, 2025 15:35
@thatstoasty thatstoasty marked this pull request as ready for review January 29, 2025 16:42
@thatstoasty
Copy link
Collaborator Author

@izo0x90 @saviorand Please check out the changes! I was also working on updating the URI logic by switching to a byte reader instead of allocating new strings by splitting the initial uri provided. I tried to combine our changes, the tests should be passing now. There's a bit of duplication in the constants, so let me know if you want me to switch to using the Query param constants instead of the Constant struct I added!

lightbug_http/io/bytes.mojo Outdated Show resolved Hide resolved
@thatstoasty
Copy link
Collaborator Author

@saviorand Also thoughts on the client assuming port 80 if not specified? Or expect a port to be specified?

@saviorand
Copy link
Collaborator

@thatstoasty port 80 definitely makes sense, later when we add SSL support might also do 443

@izo0x90
Copy link
Contributor

izo0x90 commented Jan 29, 2025

@thatstoasty awesome changes making things more performant 🔥

@thatstoasty
Copy link
Collaborator Author

Thanks for the feedback! I removed the constants struct to use existing ones, and set the port to default to 80 if http and 443 if https.

Copy link
Collaborator

@saviorand saviorand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's mergeee

@saviorand saviorand merged commit 0d320e7 into Lightbug-HQ:main Jan 30, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants