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

[Do Not Merge] ReqBody support #17

Closed
wants to merge 4 commits into from
Closed

[Do Not Merge] ReqBody support #17

wants to merge 4 commits into from

Conversation

nsaunders
Copy link
Contributor

This adds ReqBody support in probably the most naïve way possible. Related to #1, but offers no "notion of delayed IO".

@maxhallinan
Copy link

Thanks for opening this PR! What do we need to do to get this merged?

@nsaunders
Copy link
Contributor Author

Good question.

If you look closely, this is a pretty crude implementation, definitely not production-ready. The main issue is that the request body is read to a string on every request whether it is used or not. I haven't found a straightforward way around this yet, and I think significant design changes (probably even in Hyper) might be required to fix it.

@owickstrom has explained to me that Hyper and Hypertrout are considered experimental, so he may be OK with integrating something like this with the understanding that it's really only there to demonstrate a concept; but I would definitely not disagree with a decision to wait until a better solution can be found.

In the meantime, you can feel free to clone my fork and experiment with my reqbody branch (the one you see in the PR).

@nsaunders
Copy link
Contributor Author

@maxhallinan I have also been working on Nodetrout, which is more feature-complete than Hypertrout. (As an implementation detail, it sacrifices the type safety of Hyper, but, for me, it is a reasonable tradeoff.)

@nsaunders nsaunders mentioned this pull request Mar 21, 2020
@nsaunders nsaunders closed this by deleting the head repository Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants