[Mobx 4] Make ObservableMap work with non-string keys (like es6 Map does)

[andykog]

Played around with it a bit, doesn't seems to be any performance losses, so why not?
(Will add some overpatching, test, docs, if this is going to be accepted.)

Not sure about:

• Should toJS return Map instead of object (breaking change)?

TAP version 13
# Map: initializing
[mobx] Deprecated: `mobx.map` is deprecated, use `new ObservableMap` or `mobx.observable.map` instead
-Initilizing 1000000 maps: 1565 ms.
+Initilizing 1000000 maps: 1777 ms.
# Map: looking up properties
-Looking up 100 map properties 10000 times: 43 ms.
+Looking up 100 map properties 10000 times: 80 ms.
# Map: setting and deleting properties
-Setting and deleting 100 map properties 10000 times: 4461 ms.
+Setting and deleting 100 map properties 10000 times: 4226 ms.
# (anonymous)


-Completed Date suite in 6109 ms.
+Completed Date suite in 6125 ms.

[coveralls]

Coverage increased (+0.03%) to 96.219% when pulling 62b1bb1 on andykog:realMap into 185cc13 on mobxjs:master.

[coveralls]

Coverage increased (+0.03%) to 96.219% when pulling 9d92b8d on andykog:realMap into 185cc13 on mobxjs:master.

[mweststrate]

@andykog great stuff!

Some concerns / questions on compatibility:

1. toJS would be breaking indeed if it would return a Map now, let's safe that for a newt major. I agree it would be better...
2. Browser compatibility. I am aware that browser without Map support (only IE < 11 is mentionable here I think), is really small. But still sounds like a breaking change to them. We could easily add a requirement for a polyfill if Map is not natively available, but I can imagine scenario's where people would upgrade MobX from 3.0 to say the latest minor, release it into production and then break that single important customer with an ancient browser?
3. Typings compatiblity. Map will move from one to two generic arguments, I can imagine this will break the compilation for typescript users. Although I think that might be acceptable as it is not a runtime breaking change?

[andykog]

@mweststrate,

1. ok, I suppose I can make everything compatible to the current API, except the fact that maps won't convert keys to strings. In theory, people can rely on that, is it acceptable for minor update?
2. I don't see any problem in falling back to included shim, something like SimpleMap, was going to add this as a part of “overpatching”

[mweststrate]

@andykog:

1. I think the only risk is: people converting numbers to string in one code path, and not converting it in another code path, when interacting with the map. Little chance of that, but it would annoying to find out? Maybe we should consider a major upgrade for this?
2. Perfect! But if we go for a new major, then I think it is fine to even skip this and make people themselves responsible for polyfilling

[andykog]

Maybe we should consider a major upgrade for this?

Agree

[capaj]

Should toJS return Map instead of object (breaking change)?

very much yes!

[andykog]

@capaj, @mweststrate, what worrying me is that it makes tuff to convert data containing maps to json/POJOs with mobx.toJS (assuming that it should work the same way as ObservableMap#toJS). People often use maps only for the ability to observe dynamic adding of keys. What is the best solution:

• provide mobx.toJSON & ObservableMap#toJSON?
• provide mobx.stringyMap() / mobx.dynamicObject() along with mobx.map()?
• let users deal with it?

[capaj]

@andykog IMHO if you're using maps for their dynamic keys and rely on the map being converted to object when you do toJS you're doing an antipattern. MobX forces you to initialize your default props that you use in your observables. This makes sense because a derivation runs and is expected to live from the beginning. In my react apps, this usually saves me ton of needless if statements. MobX should not encourage this kind of usage for Maps. If devs have misused this behaviour, they can always write their own custom toPOJO() method.

To sum up, ObservableMaps should not be misused as "dynamic objects".

[jamiewinder]

Out of interest, can we assume this'll make MobX 4 when that lands? It'd be very handy. Would it become the new 'default' map-type with the current string-keyed map being renamed?

[coveralls]

Coverage increased (+0.03%) to 95.773% when pulling 477afcc on andykog:realMap into 1b229f5 on mobxjs:master.

[geelen]

Just found this issue when I was trying to replicate a bug! I've been using ES6 Map's enough I have this hard-wired behaviour to always convert strings to keys before I call set/get, but I deleted that out of interest and... everything still worked!

I'm probably in the minority here, but I have been constructing things using observable(new Map()) and assuming it was truly preserving the ES6 behaviour of having any keys allowed. So that was a bit unexpected.

Mind you, guaranteeing stringed keys is pretty useful most of the time, and it feels much less surprising if you're constructing it with new ObservableMap(). So I think it'd be good to have both as an option.

Btw, a common area where this can bite you is if you index into a collection based off a JSON response (integer) as well as a React Router url fragment (string). Anyway, I'm sure you're all on top of this just wanted to let you know my experience.

[mweststrate]

This branch has been merged into the mobx4 branch (#1321). MobX4 will allow working with real maps. (Needs polyfill in IE) Thanks @andykog!

[mweststrate]

toJS behavior is kept as is, but now accepts a flag to change the behavior and return native maps instead.