canjs / can-reflect

Operate on unknown data types
https://canjs.com/doc/can-reflect.html
MIT License
4 stars 4 forks source link

Update log examples from onPatches docs #148

Closed m-mujica closed 5 years ago

m-mujica commented 5 years ago
var obj = new DefineMap({});
var handler = function(patches) {
    console.log(patches);
};

Reflect.onPatches(obj, handler);
obj.set("foo", "bar");  
obj.set("foo", "baz");  

var arr = new DefineList([]);
Reflect.onPatches(arr, handler);

arr.push("foo"); 
arr.pop();  

screen shot 2018-10-11 at 3 19 55 pm

m-mujica commented 5 years ago

@justinbmeyer should the first call to obj.set("foo", "bar"); trigger an add event instead of a set event?

wondering if the add event is just an old artifact that does not apply to Maps or if there is an issue.

justinbmeyer commented 5 years ago

Yes, it should fire an add event

bmomberger-bitovi commented 5 years ago

It looks like can-define never fires "add" patches even in cases where the key did not previously exist.

https://github.com/canjs/can-define/blob/c98548b0b849bc10df0eb1e99e0cb873b902f325/can-define.js#L517 https://github.com/canjs/can-define/blob/c98548b0b849bc10df0eb1e99e0cb873b902f325/can-define.js#L1094

However, can-define does fire "add" events correctly. https://github.com/canjs/can-define/blob/c98548b0b849bc10df0eb1e99e0cb873b902f325/can-define.js#L686

Contrast with can-observe objects where having previously had the key determines "add" vs. "set" patches. https://github.com/canjs/can-observe/blob/f365a0180353851a8b76dfec010e251e2836a751/src/-make-object.js#L142

I would consider the can-observe behavior to be the correct one, and using a similar "hasOwn" selector for patches in can-define should make "add" patches align with "add" events.

m-mujica commented 5 years ago

Brad, I created this issues btw https://github.com/canjs/can-define/issues/400

m-mujica commented 5 years ago

The docs were correct. There was an issue in can-define and that was fixed.