fix: round-trip properly serde_json::Value

[Ekleog]

The case that led to me looking into this, is trying to round-trip through serde_wasm_bindgen a serde_json::Value that is a simple map.

I also added a test for round-tripping an array, but it looks like that test just passed without any changes.

[RReverser]

Hm if you're using serde_json::Value you probably want Serializer::json_compatible() instead.

Can you confirm if it round-trip with that one?

If it does, I'm a bit hesitant about accepting arbitrary objects and assuming they're maps in deserialize_any, even though I see how it could be valuable for this kind of types.

[Ekleog]

I can confirm that the test I just added in my latest commit does work out fine.

In my specific case it does work out fine, because I'm not actually interacting with javascript and just sending stuff to indexeddb and back. So this should be fine for me, thank you!

However, I still do think there is value in this PR: I would expect javascript objects to be a self-describing format, and most likely other object types would require self-describing formats for serialization. So I'll let this PR open and let you decide what you want to do with it.

Anyway, this solves the immediate issue for me, so thank you! :)

[RReverser]

However, I still do think there is value in this PR: I would expect javascript objects to be a self-describing format

I can certainly see value, but the problem with allowing arbitrary classes is that the moment you add it, there's a lot more hidden complexity that you also need to add.

For example, right now something like Set (or any other iterable) would be accepted by deserialize_seq but would throw an error on deserialize_any, clearly indicating an unsupported type. After this PR, it would instead fall into the same branch as all other objects, and serde-wasm-bindgen would try to parse a Set as if it was a Map, trying to unpack each element as a tuple. That can lead to some very surprising behaviour, especially if it's already some set of nested arrays (Set<T[]>).

So now instead of Array::is_array(&self.value) branch you need to have one that catches iterables early. But not only that - you need to distinguish between regular iterables and map-like iterables so that each falls into the correct branch. If it's a built-in Map, it's doable via instanceof, but if it's some third-party Map implementation (e.g. coming from immutable.js), there's no way to guess.

And this all is just one example to demonstrate the can of worms that gets opened each time we want to implicitly accept a new type instead of erroring out :(

[Ekleog]

Hmm I see the point :'( Should we then just white-list Map as being the only allowed type to handle in deserialize_any? AFAIU it's the only type that can be generated by serialize in its default configuration, and thus should be enough to fix the regression test.

[RReverser]

Should we then just white-list Map as being the only allowed type to handle in deserialize_any?

Maps and plain objects, so that we don't break structs. Yeah I think that's reasonable compromise.

Maybe restore the previous code & comment and add Map check + extra comment to the same branch.

[Ekleog]

Done with aeb2188 :)