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: don't send request body as plain uint8 array #1358

Merged

Conversation

nwolber
Copy link
Contributor

@nwolber nwolber commented Oct 16, 2021

#1354 claims to fix the issue that bodies are sent as plain uint8 array. But, for me, this is unfortunately still the case.

This MR would add:

  • A fix for the issue
  • An improved test case that generates a request and checks the body contains what it should.

@seanmonstar seanmonstar enabled auto-merge (squash) October 17, 2021 01:03
@seanmonstar seanmonstar merged commit bb3d102 into seanmonstar:master Oct 17, 2021
@seanmonstar
Copy link
Owner

Sigh, why is github auto merge merging things that don't pass CI?

@crapStone
Copy link
Contributor

@nwolber you just reverted this (#1341) PR. Are you sure it doesn't break anything again?

@nwolber
Copy link
Contributor Author

nwolber commented Oct 19, 2021

@crapStone Yes, you are right. Thank you for pointing that out. To get hold of the issue around body formatting I created new test cases for all basic types supported by Body as well as a test case for multipart in my fork here:

I assembled the three variants of the particular code in Body.to_js_value that existed over time

let body_bytes: &[u8] = body_bytes.as_ref();
let body_uint8_array: Uint8Array = body_bytes.into();

// 1 current code on master
let js_value: &JsValue = body_uint8_array.as_ref();

// 2 code after #1354
let body_array = js_sys::Array::from(&body_uint8_array);
let js_value: &JsValue = body_array.as_ref();

// 3 code after 1341
let body_array = js_sys::Array::new();
body_array.push(&body_uint8_array);
let js_value: &JsValue = body_array.as_ref();

Ok(js_value.to_owned())

It turns out with the current code we can't have both working.

On Firefox 93.0 the results for me are the following:

Number Basic Multipart
1 ✔️
2
3 ✔️

I ran the tests on Rust stable (i.e 1.55.0) with wasm-pack test --firefox --features multipart on wasm-pack 0.10.1.

At the moment I don't see a solution how basic cases and the multipart case can share the same code in Body.to_js_value

edited for clarity

@crapStone
Copy link
Contributor

crapStone commented Oct 19, 2021

So my code didn't work at all 😅

Why does the multipart body take this code at all? It shouldn't even be supported unless you activate the multipart feature and then it should use the code of the other match-arm.

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