Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observable ES6 Map & Set #632

Closed
Strate opened this issue Oct 27, 2016 · 23 comments
Closed

Observable ES6 Map & Set #632

Strate opened this issue Oct 27, 2016 · 23 comments

Comments

@Strate
Copy link
Contributor

Strate commented Oct 27, 2016

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
Copy link

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
Copy link
Contributor Author

Strate commented Oct 30, 2016

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
Copy link
Collaborator

@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
Copy link
Contributor Author

Strate commented Oct 30, 2016

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

@urugator
Copy link
Collaborator

@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
Copy link
Contributor Author

Strate commented Oct 30, 2016

@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
Copy link
Member

andykog commented Oct 30, 2016

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
Copy link
Collaborator

urugator commented Oct 30, 2016

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
Copy link
Member

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
Copy link
Contributor Author

Strate commented Nov 4, 2016

@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
Copy link
Member

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
Copy link

damonmaria commented Apr 3, 2017

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
Copy link

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

@solkimicreb
Copy link

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
Copy link
Member

mweststrate commented Aug 19, 2017 via email

@urugator
Copy link
Collaborator

@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
Copy link

solkimicreb commented Aug 19, 2017 via email

@urugator
Copy link
Collaborator

urugator commented Aug 19, 2017

@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
Copy link

@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
Copy link
Collaborator

urugator commented Aug 19, 2017

@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
Copy link

solkimicreb commented Aug 19, 2017

@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
Copy link
Collaborator

@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
Copy link

solkimicreb commented Aug 20, 2017

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

  • It could allow subscriptions for non standard map extensions (like custom props on them).
  • It could allow a Proxy like transparent wrapping. With simple monkey patching you always expose the patched (reactive object) and you can't access a non reactive counterpart. With Proxies you can decide if you would like to expose the patched reactive or the original method on access. (Which aligns better with the transparent wrapping philosophy).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants