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

wasm: fix standalone/multipart body conversion to JsValue #1364

Merged
merged 4 commits into from
Nov 19, 2021

Conversation

nwolber
Copy link
Contributor

@nwolber nwolber commented Oct 20, 2021

This is an attempt to fix the conflicting conversion code requried to turn a Body into a JsValue depending if the body is standalone or if it is part of a form (aka multipart/form-data).

The idea is to introduce a new variant to the crate::wasm::body::Inner enum that holds the actual content of a body. The new variant allows Body.to_js_value to have dedicated codepaths for standalone bodies and form parts. The new variant is contructed by the new Body.into_part method. That method is called by the crate::wasm::multipart::Part::new constructor and turns an Inner::Body into the new Inner::MultipartPart variant.

The drawback is that the new variant leads to some code duplication in the Body methods where it mostly shares code with the Inner::Bytes variant.

For clarity the PR renames the Inner::Multipart variant to Inner::MultipartForm. It also adds new test cases to cover the serialization of different bodies either standalone or multipart.

Thanks to @crapStone for pointing at the Inner enum in #1358 and @skystar-p for the original mutlipart conversion code in #1341.

Multipart(Form),
MultipartForm(Form),
#[cfg(feature = "multipart")]
MultipartPart(Bytes),
Copy link
Owner

Choose a reason for hiding this comment

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

Would you be able to add a comment to the variants to help us remember what the difference is between the two? (It's still not clear to me XD).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I give it a try. Tell me if it is clearer now or where I should go more into detail.

@SionoiS
Copy link

SionoiS commented Nov 1, 2021

Would this fix #1141 ?

@ghost
Copy link

ghost commented Nov 18, 2021

@seanmonstar This is a critical fix for sending multipart requests via WASM and review of this code should be prioritized. I can confirm through my testing that multipart requests work correctly after applying this patch.

@seanmonstar seanmonstar merged commit 0ef1a2e into seanmonstar:master Nov 19, 2021
@nwolber nwolber deleted the non-multipart-vs-multipart branch November 19, 2021 18:29
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

3 participants