Merge #22: Reflect mutations before invoking the callback

alshakero · web-flow · commit a674d60898bc · 2017-11-08T10:33:40.000+01:00
diff --git a/.travis.yml b/.travis.yml
@@ -6,5 +6,5 @@ before_script:
   - 'export PATH=$PWD/node_modules/.bin:$PATH'
 node_js: 6
 script:
-  - xvfb-run npm test
-  - xvfb-run npm run bench
+  - npm run test
+  - npm run bench
diff --git a/src/jsonpatcherproxy.js b/src/jsonpatcherproxy.js
@@ -35,8 +35,8 @@ const JSONPatcherProxy = (function() {
    * @param {Object} obj the object you need to find its path
    */
   function findObjectPath(instance, obj) {
-    var pathComponents = [];
-    var parentAndPath = instance.parenthoodMap.get(obj);
+    const pathComponents = [];
+    let parentAndPath = instance.parenthoodMap.get(obj);
     while (parentAndPath && parentAndPath.path) {
       // because we're walking up-tree, we need to use the array as a stack
       pathComponents.unshift(parentAndPath.path);
@@ -59,17 +59,17 @@ const JSONPatcherProxy = (function() {
   function setTrap(instance, target, key, newValue) {
     const parentPath = findObjectPath(instance, target);
 
-    var destinationPropKey = parentPath + '/' + escapePathComponent(key);
-    {
-      if (instance.proxifiedObjectsMap.has(newValue)) {
-        var newValueOriginalObject = instance.proxifiedObjectsMap.get(newValue);
+    const destinationPropKey = parentPath + '/' + escapePathComponent(key);
 
-        instance.parenthoodMap.set(newValueOriginalObject.originalObject, {
-          parent: target,
-          path: key
-        });
-      }
-      /*
+    if (instance.proxifiedObjectsMap.has(newValue)) {
+      const newValueOriginalObject = instance.proxifiedObjectsMap.get(newValue);
+
+      instance.parenthoodMap.set(newValueOriginalObject.originalObject, {
+        parent: target,
+        path: key
+      });
+    }
+    /*
         mark already proxified values as inherited.
         rationale: proxy.arr.shift()
         will emit
@@ -79,22 +79,22 @@ const JSONPatcherProxy = (function() {
         by default, the second operation would revoke the proxy, and this renders arr revoked.
         That's why we need to remember the proxies that are inherited.
       */
-      const revokableInstance = instance.proxifiedObjectsMap.get(newValue);
-      /*
-          Why do we need to check instance.isProxifyingTreeNow?
+    const revokableInstance = instance.proxifiedObjectsMap.get(newValue);
+    /*
+    Why do we need to check instance.isProxifyingTreeNow?
 
-          We need to make sure we mark revokables as inherited ONLY when we're observing,
-          because throughout the first proxification, a sub-object is proxified and then assigned to 
-          its parent object. This assignment of a pre-proxified object can fool us into thinking
-          that it's a proxified object moved around, while in fact it's the first assignment ever. 
+    We need to make sure we mark revokables as inherited ONLY when we're observing,
+    because throughout the first proxification, a sub-object is proxified and then assigned to 
+    its parent object. This assignment of a pre-proxified object can fool us into thinking
+    that it's a proxified object moved around, while in fact it's the first assignment ever. 
 
-          Checking isProxifyingTreeNow ensures this is not happening in the first proxification, 
-          but in fact is is a proxified object moved around the tree
-          */
-      if (revokableInstance && !instance.isProxifyingTreeNow) {
-        revokableInstance.inherited = true;
-      }
+    Checking isProxifyingTreeNow ensures this is not happening in the first proxification, 
+    but in fact is is a proxified object moved around the tree
+    */
+    if (revokableInstance && !instance.isProxifyingTreeNow) {
+      revokableInstance.inherited = true;
     }
+
     // if the new value is an object, make sure to watch it
     if (
       newValue &&
@@ -107,70 +107,46 @@ const JSONPatcherProxy = (function() {
       });
       newValue = instance._proxifyObjectTreeRecursively(target, newValue, key);
     }
+    // let's start with this operation, and may or may not update it later
+    const operation = {
+      op: 'remove',
+      path: destinationPropKey
+    };
     if (typeof newValue == 'undefined') {
-      if (target.hasOwnProperty(key)) {
+      // applying De Morgan's laws would be a tad faster, but less readable
+      if (!Array.isArray(target) && !target.hasOwnProperty(key)) {
+        // `undefined` is being set to an already undefined value, keep silent
+        return Reflect.set(target, key, newValue);
+      } else {
         // when array element is set to `undefined`, should generate replace to `null`
         if (Array.isArray(target)) {
-          //undefined array elements are JSON.stringified to `null`
-          instance.defaultCallback({
-            op: 'replace',
-            path: destinationPropKey,
-            value: null
-          });
-        } else {
-          instance.defaultCallback({
-            op: 'remove',
-            path: destinationPropKey
-          });
+          // undefined array elements are JSON.stringified to `null`
+          (operation.op = 'replace'), (operation.value = null);
         }
         const oldValue = instance.proxifiedObjectsMap.get(target[key]);
         // was the deleted a proxified object?
-        if(oldValue) { 
+        if (oldValue) {
           instance.parenthoodMap.delete(target[key]);
           instance.disableTrapsForProxy(oldValue);
           instance.proxifiedObjectsMap.delete(oldValue);
         }
-        return Reflect.set(target, key, newValue);
-      } else if (!Array.isArray(target)) {
+      }
+    } else {
+      if (Array.isArray(target) && !Number.isInteger(+key.toString())) {
+        /* array props (as opposed to indices) don't emit any patches, to avoid needless `length` patches */
         return Reflect.set(target, key, newValue);
       }
-    }
-    /* array props don't emit any patches, to avoid needless `length` patches */
-    if (Array.isArray(target) && !Number.isInteger(+key.toString())) {
-      return Reflect.set(target, key, newValue);
-    }
-    if (target.hasOwnProperty(key)) {
-      if (typeof target[key] == 'undefined') {
-        if (Array.isArray(target)) {
-          instance.defaultCallback({
-            op: 'replace',
-            path: destinationPropKey,
-            value: newValue
-          });
-        } else {
-          instance.defaultCallback({
-            op: 'add',
-            path: destinationPropKey,
-            value: newValue
-          });
+      operation.op = 'add';
+      if (target.hasOwnProperty(key)) {
+        if (typeof target[key] !== 'undefined' || Array.isArray(target)) {
+          operation.op = 'replace'; // setting `undefined` array elements is a `replace` op
         }
-        return Reflect.set(target, key, newValue);
-      } else {
-        instance.defaultCallback({
-          op: 'replace',
-          path: destinationPropKey,
-          value: newValue
-        });
-        return Reflect.set(target, key, newValue);
       }
-    } else {
-      instance.defaultCallback({
-        op: 'add',
-        path: destinationPropKey,
-        value: newValue
-      });
-      return Reflect.set(target, key, newValue);
+      operation.value = newValue;
     }
+    const reflectionResult = Reflect.set(target, key, newValue);
+    instance.defaultCallback(operation);
+    return reflectionResult;
   }
   /**
    * A callback to be used as th proxy delete trap callback.
@@ -182,13 +158,8 @@ const JSONPatcherProxy = (function() {
   function deleteTrap(instance, target, key) {
     if (typeof target[key] !== 'undefined') {
       const parentPath = findObjectPath(instance, target);
-
       const destinationPropKey = parentPath + '/' + escapePathComponent(key);
 
-      instance.defaultCallback({
-        op: 'remove',
-        path: destinationPropKey
-      });
       const revokableProxyInstance = instance.proxifiedObjectsMap.get(
         target[key]
       );
@@ -197,7 +168,7 @@ const JSONPatcherProxy = (function() {
         if (revokableProxyInstance.inherited) {
           /*
             this is an inherited proxy (an already proxified object that was moved around), 
-            we shouldn't revoke it, because even though it was removed from path1, it is indeed used in path2.
+            we shouldn't revoke it, because even though it was removed from path1, it is still used in path2.
             And we know that because we mark moved proxies with `inherited` flag when we move them
 
             it is a good idea to remove this flag if we come across it here, in deleteProperty trap.
@@ -210,8 +181,15 @@ const JSONPatcherProxy = (function() {
           instance.proxifiedObjectsMap.delete(target[key]);
         }
       }
+      const reflectionResult = Reflect.deleteProperty(target, key);
+
+      instance.defaultCallback({
+        op: 'remove',
+        path: destinationPropKey
+      });
+
+      return reflectionResult;
     }
-    return Reflect.deleteProperty(target, key);
   }
   /* pre-define resume and pause functions to enhance constructors performance */
   function resume() {
@@ -263,11 +241,10 @@ const JSONPatcherProxy = (function() {
     if (!obj) {
       return obj;
     }
-    const instance = this;
     const traps = {
       set: (target, key, value, receiver) =>
-        setTrap(instance, target, key, value, receiver),
-      deleteProperty: (target, key) => deleteTrap(instance, target, key)
+        setTrap(this, target, key, value, receiver),
+      deleteProperty: (target, key) => deleteTrap(this, target, key)
     };
     const revocableInstance = Proxy.revocable(obj, traps);
     // cache traps object to disable them later.
diff --git a/test/spec/proxySpec.js b/test/spec/proxySpec.js
