Mobx: observable.map replace() regression since 4.13.1

[hyperknot]

This is quite a serious regression, introduced with 4.13.1. I checked all versions on codesandbox and everything was OK till 4.13.0.

The following snippet is self explanatory:

import { observable } from "mobx";

const myMap = observable.map();
myMap.set(1, 1);
myMap.set(2, 1);
myMap.set(3, 1);

const newMap = observable.map();
newMap.set(4, 1);
newMap.set(5, 1);
newMap.set(6, 1);

myMap.replace(newMap);

console.log(myMap._data); // -> 2,4,5,6
console.log(myMap.toJS()); // -> 4,5,6
console.log(myMap.has(2));  // -> TRUE

https://codesandbox.io/s/minimal-mobx-react-project-z3q7q?expanddevtools=1&fontsize=14&hidenavigation=1&theme=dark&view=editor

Basically observable.map()'s replace() function is not clearing the array properly. It's more complicated than that though, the toJS() method produces clean output, but _data and has() returns this broken state. Basically running replace() leaves the map in a broken state. Doing a clear() first is a workaround.

4.13.0 is still ok, everything after 4.13.1 is broken.

[FredyC]

Thanks for the report. It looks like a bug indeed. There have been few PRs around the replace. It's a very tricky part of the lib to get it right it seems :)

Someone with better insight will have a look soon.

cc @urugator

[urugator]

Changing
https://github.com/mobxjs/mobx/blob/7f67c871ee2f669850eb7d85374274ede5604f60/src/v4/types/observablemap.ts#L331
to

const oldKeys = this._keys.slice()

could do the trick.

[urugator]

Wanted to PR, but something messed up code formatting (ide/command dunno) lost my nerve ... typical workflow ... 1 minute of coding, an hour dealing with tools ...

A test in case someone can get it done faster (it passes with the fix mentioned above):

test("#2274", () => {
    const myMap = mobx.observable.map()
    myMap.set(1, 1)
    myMap.set(2, 1)
    myMap.set(3, 1)

    const newMap = mobx.observable.map()
    newMap.set(4, 1)
    newMap.set(5, 1)
    newMap.set(6, 1)

    myMap.replace(newMap);

    expect(Array.from(myMap._data.keys())).toEqual([4, 5, 6])
    expect(myMap.has(2)).toBe(false)
})

[mweststrate]

Looks like #2255 fixes this one as well

[mweststrate]

@urugator's fix does seem to do the trick, so put up a quick pr. That being said, the v4 and v5 implementation of map differ more than they should, so once #2255 is done, we should really pick one of the two implementations, as for maps there is no need to have completely different implementations in v4 / v5 IIRC

[urugator]

We've already talked about this a bit here https://github.com/mobxjs/mobx/pull/2058#issuecomment-517910963 and actually I wanted to take a look at it before trying to fix #2255 (so it doesn't have to be done twice), not sure if I will find time.

[urugator]

Just to avoid confusion the V5 currently does NOT contain a fix for #1980 (so it's broken) it was only fixed for V4. (However the initially proposed solution somewhere in the comments was initially intended for V5, so a potential fix exists...)

[FredyC]

we should really pick one of the two implementations, as for maps there is no need to have completely different implementations in v4 / v5 IIRC

It might be a good idea to move shared code into common folder or something. It's probably a good place to start as any to mitigate at least some copy-pasta over time.

[mweststrate]

It might be a good idea to move shared code into common folder or something. It's probably a good place to start as any to mitigate at least some copy-pasta over time.

Yeah, my biggest concern here is how to make sure that something from common doesn't back refer to back to v4 or v5 and blow everything. If we can guarantee that, we can probably start moving some of the complexer body implementations and generic utilities. Not sure how much we can move in the end, but I'd expect no more than 30% or something, because a lot of code probably depends probably on version specific utilities, which might be hard to extract without creating unreadable code? Not sure about that, just superficial fears :)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.