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

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

Closed
maurobender opened this issue Aug 30, 2018 · 6 comments
Closed

Comments

@maurobender
Copy link
Contributor

maurobender commented Aug 30, 2018

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
Copy link
Contributor Author

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

@evankleist
Copy link

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
Copy link

jcf120 commented Nov 15, 2018

Seconded!

@zalmoxisus
Copy link
Owner

@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.

@zalmoxisus zalmoxisus reopened this Nov 20, 2018
@maurobender
Copy link
Contributor Author

maurobender commented Dec 13, 2018

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
Copy link
Owner

zalmoxisus commented Dec 13, 2018

@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.

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

No branches or pull requests

4 participants