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

multipart for wasm32 #966

Merged
merged 3 commits into from Jul 8, 2020
Merged

Conversation

Gottox
Copy link
Contributor

@Gottox Gottox commented Jul 3, 2020

This PR introduces Request::bearer_auth for wasm32 and multipart data based on FormData

I marked this as WIP as it is currently at proof of concept stage.

solves #965.

@Gottox
Copy link
Contributor Author

Gottox commented Jul 3, 2020

Most of the code is based on the async implementation.

I believe that async and wasm (maybe even blocking) can share more code in that regard.

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

I think this looks great. What in particular do you think needs more work?

src/wasm/request.rs Outdated Show resolved Hide resolved
@@ -180,6 +190,14 @@ impl RequestBuilder {
self
}

/// TODO
Copy link
Owner

Choose a reason for hiding this comment

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

This can probably be just a simple sentence, since the main docs use the non-wasm version.

@Gottox
Copy link
Contributor Author

Gottox commented Jul 6, 2020

For me there's the issue that async_impl and wasm have lots of dublicated logic, only the serialisation to FormData for wasm and Request for async_impl are different.

Also, it is largely untested as the project I'd like to use this for is currently in the process of being ported to wasm.

Another issue is, that async_impl supports stacking multipart messages in eachother, my wasm implementation will fail during runtime when the user does this.

All in all only style questions and edge cases that I currently don't like. And as I said - basicly untested code.

@Gottox Gottox marked this pull request as ready for review July 6, 2020 20:03
src/wasm/body.rs Outdated Show resolved Hide resolved
@seanmonstar
Copy link
Owner

I hear you on the duplicated logic. It would be a good PR to try to consolidate as much as possible into some shared module. But we don't have to figure it out in this PR.

Also, if you know how we can setup unit testing for WASM, I'd be thrilled. I don't know too much about WASM testing, myself. See #657

The async_impl doesn't do this either, so let's try to stay API
compatible.
@Gottox
Copy link
Contributor Author

Gottox commented Jul 8, 2020

Well, actually I'm as new to wasm as you are, but I know that you can use wasm-pack to test the code.

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This seems good to me, if you want to learn how to test Rust WASM and deal with #657, that could be a cool next step. If not, no problem, thanks for this PR!

@seanmonstar seanmonstar merged commit a800202 into seanmonstar:master Jul 8, 2020
@Gottox
Copy link
Contributor Author

Gottox commented Jul 10, 2020

Yay! Have fun!

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.

None yet

2 participants