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

Allow Serde to bind Uint8Array to Vec<u8> #2017

Closed
zer0x64 opened this issue Feb 28, 2020 · 6 comments · Fixed by #3031
Closed

Allow Serde to bind Uint8Array to Vec<u8> #2017

zer0x64 opened this issue Feb 28, 2020 · 6 comments · Fixed by #3031

Comments

@zer0x64
Copy link

zer0x64 commented Feb 28, 2020

Not sure if this should be considered as a feature or a bug...

Motivation

Currently, wasm-bindgen bind Vec<u8> return type to Uint8Array on the javascript type. However, when using into_serde() to pass the data to the Rust side, the method returns an error when parsing a Uint8Array into a Vec<u8>.

#[wasm_bindgen]
pub fn test_serde(data: JsValue)-> Vec<u8> {
    let test: Vec<u8> = data.into_serde().expect("This will crash if the input is a Uint8Array");
    test
}

Although the solution in a simple case like this is to simply take a Vec<u8> directly as an input, this doesn't work for more complex type. A hack around it wouild be to convert the Uint8Array into an Array on the rust side:

// This works with either an Array or a Uint8Array
#[wasm_bindgen]
pub fn test_serde(data: JsValue)-> Vec<u8> {
    let data = if JsCast::is_instance_of::<Uint8Array>(&data) {
        JsValue::from(Array::from(&data))
    }
    else {
        data
    };

    let test: Vec<u8> = data.into_serde().unwrap();
    test
}

My specific type context was to receive a Vec<Vec<u8>>, which would be of the type Array<Uint8Array> from the Javascript side. I'll be dumping the code snippet of the hack here in case someone else need it:

fn test(values: Array) {
    let values = JsValue::from(values.map(&mut |v, _, _| {
        if JsCast::is_instance_of::<Uint8Array>(&v) {
            JsValue::from(Array::from(&v))
        }
        else {
            v
        }
    }));
    //...
}

This does work, but is pretty ugly. From a user perspective, it makes sense that deserializing a Uint8Array into a Vec<u8> should work without this boilerplate.

Alternatives

For the moment, I'm using the hack described above.

@alexcrichton
Copy link
Contributor

Thanks for the report!

This is due to the fact that JSON.stringify(new Uint8Array([1, 2, 3])) actually returns a map instead of an array. Because of that we try to deserialize from a map, not an array.

This may be fixable by updating the implementation of the intrinsics to special case typed arrays, but I'm not sure if that's the best change to make per se or how it would best be done otherwise.

@Pauan
Copy link
Contributor

Pauan commented Mar 2, 2020

@zer0x64 For passing serialized data between Rust and JS, I recommend using serde-wasm-bindgen, it should handle this situation correctly.

@alexcrichton Since into_serde and from_serde are slow and are missing a lot of useful features, I think we should just push people to use serde-wasm-bindgen instead. Maybe even consider deprecating into_serde and from_serde.

Or maybe we can change into_serde and from_serde so that they are just proxies for serde-wasm-bindgen. We would probably want to put it behind a feature flag so it doesn't affect compile times.

@RReverser
Copy link
Member

Came here to mention serde-wasm-bindgen, but @Pauan beat me to it :)

Or maybe we can change into_serde and from_serde so that they are just proxies for serde-wasm-bindgen.

That was the plan on the original PR.

The problem is, this is a potential breaking change, so until wasm-bindgen bumps significant version number, it's tracked like other breaking changes under a corresponding label in #1258.

@alexcrichton
Copy link
Contributor

I'd be fine deprecating the into_serde/from_serde for now!

@RReverser
Copy link
Member

@alexcrichton To clarify, you mean just a soft deprecation warning for now, right?

@alexcrichton
Copy link
Contributor

Indeed yeah, either removing them or eventually implementing in terms of the serde-wasm-bindgen crate I think is the way to go, so we want to discourage usage of them as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants