zalmoxisus / remotedev-serialize

Serialize "unserializable" data and parse it back.
25 stars 21 forks source link

Wrong Immutable.Map serialization / parsing when keys are integers #8

Closed maurobender closed 1 year ago

maurobender commented 6 years ago

Issue

When the keys of an Immutable.Map object are integers and the map is serialized they are converted to strings and then when the serialization is parsed the map is created with string keys instead of numeric ones.

Test

To reproduce the issue you can use the next script:

import Immutable from 'immutable';
import Serialize from 'remotedev-serialize';
const { stringify, parse } =  Serialize.immutable(Immutable);

const data = new Immutable.Map( [ [ 1, "one" ], [ 2, "two" ] ] );
console.log("Original", data);
const serialized = stringify(data);
console.log("Serialized", serialized);
const parsed = parse(serialized);
console.log("Parsed", parsed);
console.log(Immutable.is(parsed, data));

The result of that script is:

Original Map { 1: "one", 2: "two" }
Serialized {"data":{"1":"one","2":"two"},"__serializedType__":"ImmutableMap"}
Parsed Map { "1": "one", "2": "two" }
false

As you can see the parsed Map has string keys instead of the original numeric ones.

Posible fix

Serialize the Map as an array of tuples instead of a JS object.

Change the next line in immutable/serialize.js:

if (Immutable.Map.isMap(value)) return mark(value, 'ImmutableMap', 'toObject' );

to

Immutable < 4
if (Immutable.Map.isMap(value)) return mark(value.entrySeq().toArray(), 'ImmutableMap');
Immutable 4
if (Immutable.Map.isMap(value)) return mark(value, 'ImmutableMap', 'toArray' );
maurobender commented 6 years ago

Hi @zalmoxisus I've created a Pull Request with the fix.

evankleist commented 6 years ago

Is this repository no longer maintained? It would be great if this pull request could be merged and a new version released. This issue is over a month old with a fix ready to go

jcf120 commented 6 years ago

Seconded!

zalmoxisus commented 6 years ago

@maurobender thanks again for working on this. It fixes the issue, but breaks how it is displayed.

A solution would be to transform it into ES6 Map, serializing like so:

- if (Immutable.OrderedMap.isOrderedMap(value)) return mark(value.entrySeq().toArray(), 'ImmutableOrderedMap');
+ if (Immutable.OrderedMap.isOrderedMap(value)) return mark(new Map(value.entrySeq().toArray()), 'ImmutableOrderedMap');
- if (Immutable.Map.isMap(value)) return mark(value.entrySeq().toArray(), 'ImmutableMap');
+ if (Immutable.Map.isMap(value)) return mark(new Map(value.entrySeq().toArray()), 'ImmutableMap');

and parsing:

- case 'ImmutableMap': return Immutable.Map(data);
+ case 'ImmutableMap': return Immutable.Map(Array.from(data));
- case 'ImmutableOrderedMap': return Immutable.OrderedMap(data);
+ case 'ImmutableOrderedMap': return Immutable.OrderedMap(Array.from(data));

Not sure about OrderedMap.

However, there's an issue with reviver on jsan and for Chart Monitor it should be supported in map2tree.

maurobender commented 5 years ago

Hi @zalmoxisus! Sorry for my late response, I've just saw the notification about this. I've gone through the issue you mentioned but that is a matter only of how the data is displayed when the Map is created from an array instead of a map, I don't think is up to this library to care about this but caring only about the right serialization / deserialization of the data. Displaying the Map in a pretty way disregarding if it was created from an Array or a Map should be handled directly by immutable itself or redux-devtools-extension. What do you think?

zalmoxisus commented 5 years ago

@maurobender we are not including immutable inside the extension, it's agnostic and shows exactly what receives from client part. That allows to handle other libraries the same way as well. So the intention in this library is also to care of how the data will be shown, not only to provide a way to deserialize it back.

redux-devtools-extension supports ES6 Map, so serializing ImmutableJs Map to ES6 Map which is handled by jsan will solve this issue on both parts. The problem is only that it should be implemented in map2tree to navigate it like with objects. We'd have to do that for the proposed solution as well, as it just throws now if the tree is not an object.

We should check if it will work well with ImmutableOrderedMap. ES6 Map object iterates its elements in insertion order, so that should do the trick. Or we can do new Map([...map.entries()]).sort().

When Map is not present in the browser we could fall back to the implementation with Array. That's not the case of the extension, but for others which are relying on this library.

If you'll have time to work on a modified PR, if not I'll come back to it after modifying map2tree.