From 73f82166172ce1f83ba7432b46dc71fe76b17553 Mon Sep 17 00:00:00 2001 From: Jon Hall Date: Sun, 29 Nov 2015 00:05:17 +0000 Subject: [PATCH] Add noMutate flag, which addresses #12 and adds the ability to disable object mutation through the public API. --- README.md | 26 +++++++++ index.js | 101 ++++++++++++++++++++++------------ package.json | 3 +- test/examples/fn-export.js | 2 + test/examples/proto-export.js | 3 + test/index.js | 83 +++++++++++++++++++++++++++- utils/cloneFunction.js | 17 ++++++ 7 files changed, 199 insertions(+), 36 deletions(-) create mode 100644 test/examples/fn-export.js create mode 100644 test/examples/proto-export.js create mode 100644 utils/cloneFunction.js diff --git a/README.md b/README.md index 98815f3..ca81b7c 100644 --- a/README.md +++ b/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: diff --git a/index.js b/index.js index 411b31f..0e455d9 100644 --- a/index.js +++ b/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,14 +47,31 @@ 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 @@ -56,36 +79,47 @@ function processExports(exports, test, cached, parentKeyName) { // 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) { @@ -93,21 +127,17 @@ function processExports(exports, test, cached, parentKeyName) { 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; } /** @@ -115,20 +145,23 @@ function processExports(exports, test, cached, parentKeyName) { * * @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. diff --git a/package.json b/package.json index abcdf7f..65edb53 100644 --- a/package.json +++ b/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", diff --git a/test/examples/fn-export.js b/test/examples/fn-export.js new file mode 100644 index 0000000..8f7d76d --- /dev/null +++ b/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); }; diff --git a/test/examples/proto-export.js b/test/examples/proto-export.js new file mode 100644 index 0000000..e32c9ef --- /dev/null +++ b/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; diff --git a/test/index.js b/test/index.js index 22f0a5c..8f1a67d 100644 --- a/test/index.js +++ b/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"); + }); + }); }); diff --git a/utils/cloneFunction.js b/utils/cloneFunction.js new file mode 100644 index 0000000..d93a1a2 --- /dev/null +++ b/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); +};