Merge pull request #15 from jon-hall/feature/mutating-required-modules-bug

Add noMutate flag, which addresses #12 and adds the ability to…

Author: maxkorp

README.md

@@ -70,6 +70,32 @@ myObj.myMethod({ msg: "Failure!" }, null).then(null, function(err) {
 });
 ```
 
+Wrap without mutating the original:
+```javascript
+var promisify = require("promisify-node");
+
+var myObj = {
+  myMethod: function(a, b, cb) {
+    cb(a, b);
+  }
+};
+
+// Store the original method to check later
+var originalMethod = myObj.myMethod;
+
+// Now store the result, since the 'true' value means it won't mutate 'myObj'.
+var promisifiedObj = promisify(myObj, undefined, true);
+
+// Intentionally cause a failure by passing an object and inspect the message.
+promisifiedObj.myMethod({ msg: "Failure!" }, null).then(null, function(err) {
+  console.log(err.msg);
+});
+
+// The original method is still intact
+assert(myObj.myMethod === originalMethod);
+assert(promisifiedObj.myMethod !== myObj.myMethod);
+```
+
 ### Tests ###
 
 Run the tests after installing dependencies with:

index.js

@@ -1,5 +1,7 @@
 const Promise = require("nodegit-promise");
 const args = require("./utils/args");
+const cloneFunction = require("./utils/cloneFunction");
+const objectAssign = require("object-assign");
 
 // Unfortunately this list is not exhaustive, so if you find that a method does
 // not use a "standard"-ish name, you'll have to extend this list.
@@ -12,22 +14,26 @@ var callbacks = ["cb", "callback", "callback_", "done"];
  * @param {*} exports - Should be a function or an object, identity other.
  * @param {Function} test - Optional function to identify async methods.
  * @param {String} parentKeyName - Tracks the keyName in a digestable format.
+ * @param {Boolean} noMutate - if set to true then all reference properties are
+ * cloned to avoid mutating the original object.
  * @returns {*} exports - Identity.
  */
-function processExports(exports, test, cached, parentKeyName) {
-  // Return early if this object has already been processed.
-  if (cached.indexOf(exports) > -1) {
+function processExports(exports, test, cached, parentKeyName, noMutate) {
+  if(!exports) {
     return exports;
-  } else if(typeof exports === "function") {
-    // For functions, cache the original and wrapped version, else non-wrapped
-    // functions end up being given back when encountered multiple times.
-    var cacheResult = cached.filter(function(c) {
-      return c.original === exports;
-    });
+  }
 
+  if(noMutate || typeof exports === "function") {
+    // When not mutating we have to cache the original and the wrapped clone.
+    var cacheResult = cached.filter(function(c) { return c.original === exports; });
     if(cacheResult.length) {
       return cacheResult[0].wrapped;
     }
+  } else {
+    // Return early if this object has already been processed.
+    if (cached.indexOf(exports) > -1) {
+      return exports;
+    }
   }
 
   // Record this object in the cache, if it is not a function.
@@ -41,94 +47,121 @@ function processExports(exports, test, cached, parentKeyName) {
   }
 
   var name = exports.name + "#";
+  var target;
 
   // If a function, simply return it wrapped.
   if (typeof exports === "function") {
-    // Assign the new function in place.
-    var wrapped = Promise.denodeify(exports);
+    var wrapped = exports;
+    var isAsyncFunction = false;
+
+    // Check the callback either passes the test function, or accepts a callback.
+    if ((test && test(exports, exports.name, parentKeyName))
+      // If the callback name exists as the last argument, consider it an
+      // asynchronous function.  Brittle? Fragile? Effective.
+      || (callbacks.indexOf(args(exports).slice(-1)[0]) > -1)) {
+      // Assign the new function in place.
+      wrapped = Promise.denodeify(exports);
+
+      isAsyncFunction = true;
+    } else if(noMutate) {
+      // If not mutating, then we need to clone the function, even though it isn't async.
+      wrapped = cloneFunction(exports);
+    }
+
+    // Set which object we'll mutate based upon the noMutate flag.
+    target = noMutate ? wrapped : exports;
 
-    // Push the wrapped function onto the cache before processing properties,
-    // else a cyclical function property causes a stack overflow.
+    // Here we can push our cloned/wrapped function and original onto cache.
     cached.push({
       original: exports,
       wrapped: wrapped
     });
 
     // Find properties added to functions.
     for (var keyName in exports) {
-      exports[keyName] = processExports(exports[keyName], test, cached, name);
+      target[keyName] = processExports(exports[keyName], test, cached, name, noMutate);
     }
 
     // Find methods on the prototype, if there are any.
     if (Object.keys(exports.prototype).length) {
-      processExports(exports.prototype, test, cached, name);
+      // Attach the augmented prototype.
+      wrapped.prototype = processExports(exports.prototype, test, cached, name, noMutate);
     }
 
-    // Attach the augmented prototype.
-    wrapped.prototype = exports.prototype;
-
     // Ensure attached properties to the previous function are accessible.
-    wrapped.__proto__ = exports;
+    // Only do this if it's an async (wrapped) function, else we're setting
+    // __proto__ to itself, which isn't allowed.
+    if(isAsyncFunction) {
+      wrapped.__proto__ = exports;
+    }
 
     return wrapped;
   }
 
-  Object.keys(exports).map(function(keyName) {
+  // Make a shallow clone if we're not mutating and set it as the target, else just use exports
+  target = noMutate ? objectAssign({}, exports) : exports;
+
+  // We have our shallow cloned object, so put it (and the original) in the cache
+  if(noMutate) {
+    cached.push({
+      original: exports,
+      wrapped: target
+    });
+  }
+
+  Object.keys(target).map(function(keyName) {
     // Convert to values.
-    return [keyName, exports[keyName]];
+    return [keyName, target[keyName]];
   }).filter(function(keyVal) {
     var keyName = keyVal[0];
     var value = keyVal[1];
 
     // If an object is encountered, recursively traverse.
     if (typeof value === "object") {
-      processExports(exports, test, cached, keyName + ".");
-    }
-    // Filter to functions with callbacks only.
-    else if (typeof value === "function") {
+      processExports(value, test, cached, keyName + ".", noMutate);
+    } else if (typeof value === "function") {
       // If a filter function exists, use this to determine if the function
       // is asynchronous.
       if (test) {
         // Pass the function itself, its keyName, and the parent keyName.
         return test(value, keyName, parentKeyName);
       }
 
-      // If the callback name exists as the last argument, consider it an
-      // asynchronous function.  Brittle? Fragile? Effective.
-      if (callbacks.indexOf(args(value).slice(-1)[0]) > -1) {
-        return true;
-      }
+      return true;
     }
   }).forEach(function(keyVal) {
     var keyName = keyVal[0];
     var func = keyVal[1];
 
     // Wrap this function and reassign.
-    exports[keyName] = processExports(func, test, cached, parentKeyName);
+    target[keyName] = processExports(func, test, cached, parentKeyName, noMutate);
   });
 
-  return exports;
+  return target;
 }
 
 /**
  * Public API for Promisify.  Will resolve modules names using `require`.
  *
  * @param {*} name - Can be a module name, object, or function.
  * @param {Function} test - Optional function to identify async methods.
+ * @param {Boolean} noMutate - Optional set to true to avoid mutating the target.
  * @returns {*} exports - The resolved value from require or passed in value.
  */
-module.exports = function(name, test) {
+module.exports = function(name, test, noMutate) {
   var exports = name;
 
   // If the name argument is a String, will need to resovle using the built in
   // Node require function.
   if (typeof name === "string") {
     exports = require(name);
+    // Unless explicitly overridden, don't mutate when requiring modules.
+    noMutate = !(noMutate === false);
   }
 
   // Iterate over all properties and find asynchronous functions to convert to
   // promises.
-  return processExports(exports, test, []);
+  return processExports(exports, test, [], undefined, noMutate);
 };
 
 // Export callbacks to the module.

package.json

@@ -4,7 +4,8 @@
   "description": "Wrap Node-callback functions to return Promises.",
   "main": "index.js",
   "dependencies": {
-    "nodegit-promise": "~4.0.0"
+    "nodegit-promise": "~4.0.0",
+    "object-assign": "^4.0.1"
   },
   "devDependencies": {
     "mocha": "~1.18.2",

test/examples/fn-export.js

@@ -0,0 +1,2 @@
+module.exports = function(d, cb) { setTimeout(cb, d); };
+module.exports.x = function(d, cb) { setTimeout(cb, d); };

test/examples/proto-export.js

@@ -0,0 +1,3 @@
+function A(d) { this.d = d; }
+A.prototype.a = function(cb) { setTimeout(cb, this.d); };
+module.exports = A;

test/index.js

@@ -1,7 +1,13 @@
 var promisify = require("../");
-var assert = require("assert");
+var assert = require("assert"),
+    fsOriginal = require('fs');
 
 describe("Promisify", function() {
+  function isPromisified(fn, ctx) {
+    var result = fn && fn.apply(ctx, Array.prototype.slice.call(arguments, 2));
+    return result && (typeof result.then === 'function');
+  }
+
   it("can convert a basic async function", function() {
     function test(cb) {
       cb(null, true);
@@ -30,6 +36,41 @@ describe("Promisify", function() {
 
       return fs.readFile(__dirname + "/../LICENSE");
     });
+
+    it("doesn't mutate objects for other consumers", function() {
+      var fsp = promisify("fs");
+      var fs2 = require("fs");
+
+      assert(fsOriginal.readFile !== fsp.readFile, "pre-required mutated");
+      assert(fsOriginal.readFile === fs2.readFile, "post-required mutated");
+      assert(fsp.readFile !== fs2.readFile, "post-required mutated");
+    });
+
+    it("doesn't mutate functions for other consumers", function() {
+      var fn = require(__dirname + "/examples/fn-export.js");
+      var fnx = fn.x;
+      var fnp = promisify(__dirname + "/examples/fn-export.js");
+      var fn2 = require(__dirname + "/examples/fn-export.js");
+
+      assert(fn.x !== fnp, "pre-required mutated");
+      assert(fn2.x !== fnp, "post-required mutated");
+      assert(fn.x === fnx, "function property mutated");
+      assert(fnp.x !== fn, "function property not replaced");
+    });
+
+    it("doesn't mutate prototypes for other consumers", function() {
+      var A = require(__dirname + "/examples/proto-export.js");
+      var a = new A(5);
+      var Ap = promisify(__dirname + "/examples/proto-export.js");
+      var ap = new Ap(5);
+      var A2 = require(__dirname + "/examples/proto-export.js");
+      var a2 = new A2(5);
+
+      assert(isPromisified(ap.a, ap), "prototype method not promisified");
+      assert(a.a !== ap.a, "pre-required mutated");
+      assert(a2.a !== ap.a, "post-required mutated");
+      assert(a2.a === a.a, "post-required mutated");
+    });
   });
 
   describe("asynchronous method inference", function() {
@@ -137,4 +178,44 @@ describe("Promisify", function() {
       assert(typeof a.a(5).then === "function", "function property not wrapped");
     });
   });
+
+
+  describe("no mutate", function() {
+    it("can promisify an object without mutating it", function() {
+      var a = {
+        a: function(cb) { cb(); }
+      };
+
+      var b = promisify(a, undefined, true);
+
+      assert(isPromisified(b.a, b), "method not promisified");
+      assert(a.a !== b.a, "object mutated");
+    });
+
+    it("can promisify a function's properties without mutating it", function() {
+      var a = function(cb){ cb(null, 1); };
+      a.a = function(cb) { cb(); };
+
+      var b = promisify(a, undefined, true);
+
+      assert(isPromisified(b), "method not promisified");
+      assert(isPromisified(b.a, b), "method property not promisified");
+      assert(a.a !== b, "method property mutated");
+      assert(a.a !== b.a, "method property mutated");
+    });
+
+    it("can promisify a constructor without mutating it", function() {
+      var A = function(){ };
+      A.a = function(cb) { cb(); };
+      A.prototype.a = function(cb) { cb(null, 2); };
+
+      var B = promisify(A, undefined, true);
+      var b = new B();
+
+      assert(isPromisified(B.a, B), "method property not promisified");
+      assert(isPromisified(b.a, b), "prototype method not promisified");
+      assert(A.a !== B.a, "method property mutated");
+      assert(A.prototype.a !== b.a, "prototype mutated");
+    });
+  });
 });

utils/cloneFunction.js

@@ -0,0 +1,17 @@
+/**
+ * Clones a function, including copying any properties of the function to the clone.
+ *
+ * @param {Function} func - The function to clone.
+ */
+module.exports = function cloneFn(func) {
+    var temp;
+    // Check for the memoized value on the function in-case we get called to wrap the same function
+    // (or already wrapped function) again.
+    return func.__cloneFn || (temp = function() {
+      return func.apply(this, arguments);
+    }) &&
+    // Assign __proto__ as a quick way to copy function properties.
+    (temp.__proto__ = func) &&
+    // Lastly, set a cache var on the original and clone, and return the result.
+    (func.__cloneFn = temp.__cloneFn = temp);
+};