mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.56k stars 1.78k forks source link

Observable ES6 Map & Set #632

Closed Strate closed 7 years ago

Strate commented 8 years ago

As you know, ES6 provide a set of useful collections, such as Map and Set, It would be great if mobx will have full support to make them observables.

tcrognon commented 8 years ago

I want this very badly, too. The reason this isn't implemented yet is because it requires ES6 Proxy to work. I'm guessing everything will be rewritten to use proxies eventually. For now, map() and asMap() exist because of this limitation.

Strate commented 8 years ago

The reason this isn't implemented yet is because it requires ES6 Proxy to work.

For what case? Why don't create own object, which looks like Map or Set, but observable? Such as ObservableArray done.

urugator commented 8 years ago

@Strate There is a Map (only string keys are supported), no Set however. Proxies or extendable built-ins are needed just for built-in type preservation.

Strate commented 8 years ago

@urugator just for claryfication: we need es6 proxy support or extendable builtins only for instanceof operator, yes?

urugator commented 8 years ago

@Strade afaik yes (Array.isArray in case of arrays). Theoretically built-ins could also provide better performance. Proxies are generally attractive, because they could allow to keep the original object intact and move the observable logic outside. However I don't see proxies reliably usable in browsers anytime soon.

Strate commented 8 years ago

@urugator proxy available at almost all browsers (http://caniuse.com/#search=proxy) Also we can break instanceof operator for that, or just inherit original Map & Set constructors to get instanceof works. More genrally, I think it could be useful to hook into mobx to convert any value to observable with custom logic. For example, I use my own implementation of collections (replacement for arrays, map & sets), and for me that functionality would be useful.

andykog commented 8 years ago

What is the necessity of the instanceof Map check? I think, it would be just fine to allow using anything as ObservableMap key, in other aspects its seems to comply with standard Map. Don't like the idea of using proxies for now, there are many people who need ie9+ support.

urugator commented 8 years ago

I agree with @andykog, but I would still appreciate dedicated ObservableSet EDIT: Also supporting other than string keys might not worth it, if it would hurt performance too much.

mweststrate commented 8 years ago

The current observableMaps primarily usecase is providing an alternative to dynamically keyed observable object. For that reason it is string based and doesn't require a Map (polyfill), but works on all ES5 environments.

For that reason it might be interesting to be able have separate concept that makes ES6 Maps / Sets observable. I think this could done for example by having function in mobx-utils that monkey patch ES6 Maps / Sets etc to be observable. The implementation would probably be similar to the current maps, but using the original map as backing data :)

If anybody likes to implement this, please let me know! Implementation could be along the current lines of ObservableMap. But by either monkey patching or creating a new collection that inherits from Map, instanceof checks etc would work as expected.

Strate commented 8 years ago

@mweststrate I suggest to add it ti the core, to achieve automatic deep observable, as iy done with arrays:

const store = observable({
  items: [], // array here become ObservableArray
  itemsHash: new Map() // this map should be ObservableMap
  itemsSet: new Set() // this set should be ObservableSet
})
mweststrate commented 7 years ago

MobX 3 will automatically convert (string keyed) ES 6 maps to observable maps, just like arrays. For observable sets, see #69. So closing this issue for now.

damonmaria commented 7 years ago

update: Turns out losing the option polyfill: false for transform-runtime in my .babelrc was the actual problem. Mobx must instanceof and the core-js map (which transform-runtime injects) doesn't match the built in Map. I will leave this here tho in case it helps someone else.

Original question: @observable x = new Map() was working for me with 3.1.0. But now (on 3.1.7) it doesn't (x stays an ES6 Map, is not converted to an observable.map). Is this the intention? Maps are mentioned in the recent changelog but doesn't directly refer to this.

nicodjimenez commented 7 years ago

^^^ @damonmaria thank you! And thank you so much for your heroic effort on this project @mweststrate!

solkimicreb commented 7 years ago

Most Built-ins (Map and Set for example) won't work with Proxies. Their internal slots expect the Map / Set instance as the this, but they receive the Proxy object and throw an invalid receiver error on method invocations. The only viable solution I found for this is monkey patching the Map / Set methods (luckily this can be done) with reactivity logic. Arrays can be wrapped with Proxies, but have a somewhat strange behavior with length changes.

mweststrate commented 7 years ago

Thanks!

Op za 19 aug. 2017 07:08 schreef Miklos Bertalan notifications@github.com:

Most Built-ins (Map and Set for example) won't work with Proxies. Their internal slots expect the Map / Set instance as the this, but they receive the Proxy object and throw an invalid receiver error on method invocations. The only viable solution I found for this is monkey patching the Map / Set methods (luckily this can be done) with reactivity logic. Arrays can be wrapped with Proxies, but have a somewhat strange behavior with length changes.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/632#issuecomment-323501122, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhOuMo7wrcctSGK4Wl2_9nwrMUvUZks5sZm3MgaJpZM4KidEi .

urugator commented 7 years ago

@solkimicreb Can you please help me to replicate the problem? I did a very trivial test in node (not in browser) and it seems ok:

let m = new Map();
let p = new Proxy(m, {
  get(map, prop, proxy) {        
    let result = Reflect.get(map, prop, map);            
    if (typeof result === "function") {      
      result = result.bind(map);
    }    
    return result;
  }
});
p.set("a", "aVal");   
console.log(p.has("a"));
console.log(p.get("a"));
console.log(p.size);
p.forEach((val, key) => {
  console.log(key, val);
});
solkimicreb commented 7 years ago

you are binding to returned methods to the map before returning them so it wont throw an error. the proxy wont do much in this case though. the returned function uses the raw map as the this context so it wont intrcept operations done internally by the map methods. alas you still have to patch them. a combination of proxy and monkey patching can be used to intercept builtin methods and non standard expando props (for metadata for example) on maps and sets. i dont have my laptop with me now, i can write a proper answer with code in a few hours. sorry for the potato spelling and formatting

On Aug 19, 2017 15:38, "urugator" notifications@github.com wrote:

@solkimicreb https://github.com/solkimicreb Can you please help me to replicate the problem? I did a very trivial test in node (not in browser) and it seems ok:

let m = new Map();let p = new Proxy(m, { get(map, prop, proxy) { let result = Reflect.get(map, prop, map); if (typeof result === "function") { result = result.bind(map); } return result; } });p.set("a", "aVal"); console.log(p.has("a"));console.log(p.get("a"));console.log(p.size);p.forEach((val, key) => { console.log(key, val); });

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/632#issuecomment-323523696, or mute the thread https://github.com/notifications/unsubscribe-auth/AGoj7phykoyikOOR8sM2QSAfewiVLnOTks5sZuVmgaJpZM4KidEi .

urugator commented 7 years ago

@solkimicreb I think I understand - lets say the map calls somewhere internally this.get(key) the call won't be intercepted, therefore no subscription can occur. But I am not sure where is it neccessary. I think that Map api has basically two types of subscriptions - subscribe for any change (entries/forEach/...) and subscribe for a specific key change (has/get). Maybe a third kind for keys change only. In other words ... in which map method, we can't say which elements of the map will (or will not) be accessed internally in advance?

solkimicreb commented 7 years ago

@urugator Your are correct, two keys should be enough. I mean that Proxies add no value in this case.

You still have to monkey patch the methods to determine when they are called (and when to register subscriptions). The Proxy simply provides information about getting the methods and not about invoking them. It adds a slight overhead by the traps and a pretty big one by a new bound function on every method access.

Proxy would be nice if it could correctly intercept internals for builtins - as it would allow them to be handled like normal object - but this is not the case. The use case for Proxies for Maps and Sets are to allow users to use them like POJOs - by bypassing their standard API - which is not a pattern to be encouraged anyways.

urugator commented 7 years ago

@solkimicreb

a new bound function on every method access.

I think you can create the functions only once (per map) at proxy creation:

function get(key) {
  console.log(`SUBSCRIBE FOR ${key}`);  
  return this.get(key);
}
let m = new Map();
const boundGet = get.bind(m);
let p = new Proxy(m, {
  get(map, prop, proxy) {        
    if (prop === "get") {
      return boundGet;
    }    
  }
});
m.set("a", "aVal");
console.log(p.get("a")); 
solkimicreb commented 7 years ago

@urugator I am not sure that I understood your point. So here is my issue in whole.

The base idea of transparent reactivity is this:

  1. Detect if a function uses a property.
  2. Call that function automatically if the property changes later.

For this we have to know when a property is accessed and mutated.

Map props are accessed and mutated by standard methods. These methods can be thought of as boxes that need to be cracked to intercept the access / mutate operation inside them. We have to know when a property is accessed or mutated and we have to know which one it was to register or trigger the appropriate reactions.

In your example you replace the box with another sealed box, that still can't provide information about when and with what arguments it was called. Monkey patching is inevitable to crack the box in my opinion.

Where and how would you register subscriptions in your examples?

EDIT: I missed the first part 😄 Sorry about it. I see where you intercept. It is just a fancier monkey patching in this case though. Instead of replacing a method generally you replace it on every method access. Wouldn't it be simpler to do myMap.get = patchedGetWithSubScription instead?

urugator commented 7 years ago

@solkimicreb

I tried to create a quick naive prototype and came up with something partially working. I realized that you have to subscribe for keys which have been accessed, but don't exist yet (see this), which kinda complicates the whole thing, so that's not supported. I haven't bothered with methods returning iterator. Also traps should be probably wrapped in actions, but it breaks forEach not sure why... I bet there are other problems as well, but here is the code just to give the idea:

const Mobx = require("mobx");

let m = new Map();
// Creates traps for specific map
function createTraps(map) {
  const anyChangedAtom = new Mobx.Atom("AnyChanged");
  const allChangedAtom = new Mobx.Atom("AllChanged");
  const mapHas = map.has.bind(map);
  const mapGet = map.get.bind(map);
  const mapSet = map.set.bind(map);
  const mapClear = map.clear.bind(map);
  const mapDelete = map.delete.bind(map);
  const mapForEach = map.forEach.bind(map);
  const traps = {
    get(key) {      
      allChangedAtom.reportObserved(); // subscribes for operations which affects all elements
      if (mapHas(key)) {
        return mapGet(key).get(); // subscribes for get(key)
      }
      return undefined; // WRONG we have to subscribe in advance for this key
    },
    set(key, value) {
      if (mapHas(key)) {
        mapGet(key).set(value); // reports change for get(key)        
      } else {
        mapSet(key, Mobx.observable.box(value));
      }
      anyChangedAtom.reportChanged();
    },
    has(key) {
      if (mapHas(key)) {
        return mapGet(key).get(); // reports change for get(key) 
      } else {
        return undefined; // WRONG we have to subscribe in advance for this key
      }
      allChangedAtom.reportObserved();      
    },
    delete(key) {      
      if (mapHas(key)) {
        const box = mapGet(key); 
        mapDelete(key); 
        box.set(undefined); // reports change for get(key)
      }      
      anyChangedAtom.reportChanged(); // report to forEach
    },
    clear() {
      mapClear();
      anyChangedAtom.reportChanged();
      allChangedAtom.reportChanged();      
    },
    forEach(callback, thisArg) { // all keys and values are accessed so we want to subscribe for any change      
      mapForEach(callback, thisArg);
      anyChangedAtom.reportObserved();
    }
  }  
  return traps;
}
//
const traps = createTraps(m);
let p = new Proxy(m, {
  get(map, prop, proxy) {
     return traps[prop];
  }
});
// Subscribed for specific key
console.log(["TEST get()","EXPECTING:", "aVal", "aVal1", "aVal2", "undefined", "GOT:"].join("\n"));
p.set("a", "aVal"); // must be created in advance :(
let dispose = Mobx.autorun(() => {  
  console.log(p.get("a"));  
})
p.set("a", "aVal1");
p.set("b", "bVal"); // does not affect autorun
p.set("a", "aVal2");
p.delete("b"); // does not affect autorun
p.delete("a");
//p.clear();
dispose()
// ForEach affected by everything
console.log(["TEST forEach()","EXPECTING:", "1", "2", "3", "4", "5", "GOT:"].join("\n"));
let it = 0; 
dispose = Mobx.autorun(() => {  
  console.log(++it);
  p.forEach((value, key) => {})

})
p.set("a", "aVal1");
p.set("b", "bVal1");
p.delete("a");
p.clear();
dispose();
solkimicreb commented 7 years ago

@urugator That looks perfectly functional, I think we are speaking about the same thing here. I would still consider changing this:

const traps = createTraps(m);
let p = new Proxy(m, {
  get(map, prop, proxy) {
     return traps[prop];
  }
});

to this though.

const traps = createTraps(m);
for (let method in m) {
  m[method] = traps[method];
}
// Side note: be sure to overwrite the `size` accessors and `Symbol.iterator` too.

Proxies add the benefit of intercepting new props or operations other than get / set. Neither is needed here.

Proxies could provide 2 benefits here though.