Inconsistencies between observablemap and observableset

[melnikov-s]

Intended outcome: observable set and observable map should have the similar behaviour in terms of when reactions are triggered.

Actual outcome: observable map has an internal map of observable values which are updated and changed as needed, therefore if a single value is retrieved from a map within a reaction, that reaction will only care about that single value, any unrelated mutation to the map will not re-trigger the reaction. Whereas in an observable set there's just one atom that manages all changes, so if a reaction only cares about whether or not a single value is in a set any unrelated mutation to that set will still trigger the reaction.

How to reproduce the issue::
https://codesandbox.io/s/mobx-mapset-behavior-t8bjg

import { observable, autorun } from "mobx";

const map = observable(new Map());

autorun(() => {
  if (map.has("foo")) {
    console.log("map: its there!");
  } else {
    console.log("map: nope");
  }
});

map.set("bar", true); // won't run
map.set("foo", true); // will run
map.delete("bar"); // won't run
map.delete("foo"); // will run

const set = observable(new Set());

autorun(() => {
  if (set.has("foo")) {
    console.log("set: its there!");
  } else {
    console.log("set: nope");
  }
});

set.add("bar"); // will run
set.add("foo"); // will run
set.delete("bar"); // will run
set.delete("foo"); // will run

Versions All versions that have set/map.

Note: can contribute PR.

[danielkcz]

Interesting observation. It should be probably fixed as part of the upcoming V6 because this change could easily break existing apps that assume such a "faulty" behavior. If you want to try a PR, it's probably best to do it against mobx6 branch (see #2327).

[mweststrate]

This is currently working as designed, the rationale being as follows:

If Sets would maintain an atom per 'value', that would allocate an atom for every has query; so if you are inspecting 1000 elements, of whom 10 can be part of that observable set, they would all end up in that set, costing a lot of memory, and introducing potential memory leaks (although that could be addressed by using a weakmap as backing data structure).

This is potentially true for maps as well, but since those are typically used as lookup maps, there are less usage scenerios where many elements are considered, but few are part of the map (this is an assumption of course). Similarly, keys used in observable map are typically primitives such as strings, so the risk of accidentally preventing the garbage collection of values is not present, like with Sets.

This is a deliberate choice, and not the most optimal one in all possible scenarios, but at least you can optimize for the case you describe by using an observable map instead, where you use the values as keys, and true / false as value.

[melnikov-s]

This is a deliberate choice, and not the most optimal one in all possible scenarios, but at least you can optimize for the case you describe by using an observable map instead, where you use the values as keys, and true / false as value.

I can if I know about this behavior, but i'm speculating many users of mobx do not (as it is not documented anywhere and even if it was the heuristics here are confusing ). So they might reach for a Set to replace a true/false Map and end up introducing performance issues as now their reactions will update far more frequently. These type of issues are not easily noticed unless you're actively profiling.

Can we consider having similar behaviors for both Map and Set, whatever they may be? I think that would be the path of least surprise. Memory leakage is of course an issue but it gets cleaned up once there are no more reactions correct? The WeakMap suggestion sounds like a good one and we'll still need regular map for primitive values but at least with WeakMap the runtime can collect before the reaction becomes inactive.

Another alternative (for both Map and Set) might be to only keep a hasMap of values that are in the collection, so no potential for memory leaks there but doing a has on a value that is not in a collection will need to re-run the reaction for any addition but not deletion.

example:

const set = observable(new Set());

autorun(() => {
  if (set.has("foo")) {
    console.log("set: its there!");
  } else {
    console.log("set: nope");
  }
});

set.add("bar"); // will run because "foo" is not in the set and we added a new element and since we don't keep track of elements that are not in the collection we need to re-run 
set.delete("bar"); // won't run because we're deleting an element that was in the collection. Elements outside of the collection can not be affected
set.add("baz"); // will run because "foo" is not in the set and we added a new element
set.add("foo"); // will run
set.add("bar"); // won't run because 'foo' is now in the set and part of the atom map
set.delete("foo"); // will run

with Map adopting the same behavior. At least this way it's consistent between Map and Set with no potential for unbounded growth while a reaction is active and no real need for onBecomeUnobserved hooks to clean up stale hasMap entries.

[mweststrate]

Closing for now as there doesn't seem to be a big audience needing this and there is a work around.