-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Comments
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. |
For what case? Why don't create own object, which looks like |
@urugator just for claryfication: we need es6 proxy support or extendable builtins only for |
@strade afaik yes ( |
@urugator proxy available at almost all browsers (http://caniuse.com/#search=proxy) |
What is the necessity of the |
I agree with @andykog, but I would still appreciate dedicated |
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 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 |
@mweststrate I suggest to add it ti the core, to achieve automatic deep observable, as iy done with arrays:
|
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. |
update: Turns out losing the option Original question: |
^^^ @damonmaria thank you! And thank you so much for your heroic effort on this project @mweststrate! |
Most Built-ins (Map and Set for example) won't work with Proxies. Their internal slots expect the Map / Set instance as the |
Thanks!
Op za 19 aug. 2017 07:08 schreef Miklos Bertalan <[email protected]>:
… 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
<#632 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhOuMo7wrcctSGK4Wl2_9nwrMUvUZks5sZm3MgaJpZM4KidEi>
.
|
@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 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" ***@***.***> 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
<#632 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGoj7phykoyikOOR8sM2QSAfewiVLnOTks5sZuVmgaJpZM4KidEi>
.
|
@solkimicreb I think I understand - lets say the map calls somewhere internally |
@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. |
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")); |
@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:
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 |
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. 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(); |
@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.
|
As you know, ES6 provide a set of useful collections, such as
Map
andSet
, It would be great if mobx will have full support to make them observables.The text was updated successfully, but these errors were encountered: