Skip to content

feat: Add impl From<Response> for http::Response<Body> for wasm. #2837

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: Add impl From<Response> for http::Response<Body> for wasm. #2837

wants to merge 4 commits into from

Conversation

@Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Oct 13, 2025 •
edited
Loading

This PR tend to add impl From<Response> for http::Response<Body> so users under wasm won't face unexpected API missing.

I also conducted an API refactor to make it possible for users to get the Body from response like we do in async_impl.

This PR will address issues like:

    |
145 |             .into();
    |              ^^^^ the trait `From<reqwest::Response>` is not implemented for `http::Response<_>`
    |
    = note: required for `reqwest::Response` to implement `Into<http::Response<_>>`

This PR was primarily authored with Codex using GPT-5-Codex and then hand-reviewed by me. I AM responsible for every change made in this PR. I aimed to keep it aligned with our goals, though I may have missed minor issues. Please flag anything that feels off, I'll fix it quickly.

Xuanwo
Xuanwo commented Oct 13, 2025
View reviewed changes
src/lib.rs
assert_send::<Error>();
assert_sync::<Error>();

#[cfg(not(target_arch = "wasm32"))]
Copy link
Contributor Author

@Xuanwo Xuanwo Oct 13, 2025

Choose a reason for hiding this comment

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

Let me know what do you think about this change.

cc @seanmonstar

Copy link
Owner

@seanmonstar seanmonstar Oct 13, 2025

Choose a reason for hiding this comment

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

This would be a breaking change. And perhaps surprising that it only is true on a certain target?

Copy link
Contributor Author

@Xuanwo Xuanwo Oct 14, 2025

Choose a reason for hiding this comment

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

Yes, that might be a breaking change for users relying on Send + Sync for Body on the wasm32 target, although I don’t think there are valid use cases for it. Also, Response has already been handled separately.

Do you have suggestions for this? Would it be a good idea to have a separate response Body type for the wasm32 target?

Copy link
Contributor Author

@Xuanwo Xuanwo Oct 15, 2025

Choose a reason for hiding this comment

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

Ok, I just realized that Body isn’t exposed at all in the wasm target. My current plan is to keep Request unchanged but replace its internal Body with RequestBody. Then introduce a new public Body type that’s non-Send and currently used only by Response.

  • Request -> pub (crate) RequestBody (Send + Sync)
  • Response -> pub Body (non-Send)

What do you think? @seanmonstar

@Xuanwo
Format code
b7c58b1 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo mentioned this pull request Oct 13, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanmonstar seanmonstar seanmonstar left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Xuanwo @seanmonstar