Mobx: ObservableMap.replace() - breaks entities order!

[sergii-aleksieiev-ring]

According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Specifications

The Map object holds key-value pairs and remembers the original insertion order of the keys.

The Map should guarantee the order of entities, but when I use method ObservableMap.replace() I got Map with broken order of entities.

The doc (https://mobx.js.org/refguide/map.html) says:

replace(values). Replaces the entire contents of this map with the provided values. Short hand for .clear().merge(values)

So as I understand it should physically replace old Map by a new one, but in fact, it is trying to do a "smart" merge inside:

replace(values: ObservableMap<K, V> | IKeyValueMap<V> | any): ObservableMap<K, V> {
    transaction(() => {
        // grab all the keys that are present in the new map but not present in the current map
        // and delete them from the map, then merge the new map
        // this will cause reactions only on changed values
        const newKeys = (getMapLikeKeys(values) as any) as K[]
        const oldKeys = Array.from(this.keys())
        const missingKeys = oldKeys.filter(k => newKeys.indexOf(k) === -1)
        missingKeys.forEach(k => this.delete(k))
        this.merge(values)
    })
    return this
}

As a workaround I can use the following:

myMap.clear();
myMap.merge(newMap);

but it calls observe twice during one operation.

[ItamarShDev]

Hi!
can you please create a CodeSandbox with a reproduction of this?
This will greatly help us to see and understand the case.

thank you

[urugator]

Reproduction: https://codesandbox.io/s/mobx-mapreplace-breaks-entities-order-cebhk

[FredyC]

Since the replace is the addition of MobX, it doesn't necessarily need to adhere to official Map behavior imo.

Besides that "smart merge" is probably much more important to minimalize reactions for things that haven't changed.

Ultimately, I think we should just add a warning to docs with the workaround. Or perhaps introduce an additional method that will do what that workaround does? It's not very often you need to rely on the order in the Map in my opinion.

[urugator]

These replace methods are mainly intended as an alternative to reassigning new array/map.
The problem with re-assigment is that it will completely unnecessarily invalidate everything which depends on the array/map reference alone (without touching it's content).
So imo the behavior shouldn't differ from ordinary assigment.
I am not entirely sure, but the "smart merging" should be still possible, a bit more complicated however.

[sergii-aleksieiev-ring]

Reproduction: https://codesandbox.io/s/mobx-mapreplace-breaks-entities-order-cebhk

Thanks a lot for reproduction!

Since the replace is the addition of MobX, it doesn't necessarily need to adhere to official Map behavior imo.

Besides that "smart merge" is probably much more important to minimalize reactions for things that haven't changed.

Ultimately, I think we should just add a warning to docs with the workaround. Or perhaps introduce an additional method that will do what that workaround does? It's not very often you need to rely on the order in the Map in my opinion.

Since the replace is the addition of MobX, it should not contradict the official definition of Map, which says that the Map guarantees the order of entities.

"smart merge" !== "replace" methods should be named correctly. "replace" should replace the Map. "smartMerge" should "smart merge".

I am always relying on the order in Maps, especially when it is a list of thousands of items and I need a way to instant access to a specific one of them. Also, I am relying on the order as I know from the definition of Map that it guarantees the order of entities.
When I see the replace method I made an assumption that it will replace one map with another one. otherwise, it should not be called "replace".

[urugator]

This could do the trick, what do you think?

class ObservableMap {
  /* .... */

  replace(values: ObservableMap<K, V> | IKeyValueMap<V> | any): ObservableMap<K, V> {
    transaction(() => {
      const replacementMap = ObservableMap.convertToMap(values);
      const orderedData = new Map();
      const oldKeys = this.keys();
      const newKeys = replacementMap.keys();
      for (let i = 0; i < oldKeys.length; i++) {
        const oldKey = oldKeys[i];
        // detect key order change
        // if the length differs, reportChanged is called by this.delete (key removal) or this.set (key addition)
        if (oldKeys.length === newKeys.length && oldKey !== newKeys[i]) {
          this._keysAtom.reportChanged();
        }
        // delete missing key
        if (!replacementMap.has(oldKey)) {
          this.delete(oldKey);
        }
      }
      // merge
      replacementMap.forEach((value, key) => {
        this.set(key, value);
        orderedData.set(key, this._data.get(key));
      })
      // use data with correct order
      this._data = orderedData;
    })
    return this
  }

  static function convertToMap(thing) {
    if (isES6Map(thing) || isObservableMap(thing)) {
      return thing;
    } else if (Array.isArray(thing)) {
      return new Map(thing);
    } else if (isPlainObject(thing)) {
      return new Map(Object.entries(thing));
    } else {
      fail("Cannot convert " + thing + " to map");
    }
  }

[FredyC]

@urugator Probably the best approach would write failing tests first in the PR and then we can worry about an actual implementation :)

[urugator]

It's not an actual implementation, it's an idea expressed by code.
I don't see a reason why it couldn't be discussed/criticized or used as a source of inspiration without existence of tests.
Especially if there is no concensus about what the acceptable solution is, writing such tests could also end up being a waste of time.

[mweststrate]

I think it is a good idea to respect insertion order indeed, and make the necessary changes.

I think the most important reason we didn't worry about it before is a) nobody run into it and b) in mobx 3 order wasn't guaranteed as the backing data structure was an object, not a map

[FredyC]

PR #2057 merged and waiting for release.

[Bnaya]

published

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