Monkey-patching native JavaScript constructors

Question

I've written a small utility for monkey-patching native JavaScript constructor functions. For example, you can use it to modify input arguments before returning an instance (this can be useful for unit tests).

There is much more detail about the utility on GitHub, so it may be useful to read through the readme on there.

I would be interested to know if I've hugely over-engineered this. There may be simpler techniques that I could take advantage of. Any comments are appreciated.

Here is the code:

var patch = (function () {
    /*jshint evil: true */

    "use strict";

    var global = new Function("return this;")(), // Get a reference to the global object
        fnProps = Object.getOwnPropertyNames(Function); // Get the own ("static") properties of the Function constructor

    return function (original, originalRef, patches) {

        var ref = global[originalRef] = original, // Maintain a reference to the original constructor as a new property on the global object
            args = [],
            newRef, // This will be the new patched constructor
            i;

        patches.called = patches.called || originalRef; // If we are not patching static calls just pass them through to the original function

        for (i = 0; i < original.length; i++) { // Match the arity of the original constructor
            args[i] = "a" + i; // Give the arguments a name (native constructors don't care, but user-defined ones will break otherwise)
        }

        if (patches.constructed) { // This string is evaluated to create the patched constructor body in the case that we are patching newed calls
            args.push("'use strict'; return (!!this ? " + patches.constructed + " : " + patches.called + ").apply(null, arguments);"); 
        } else { // This string is evaluated to create the patched constructor body in the case that we are only patching static calls
            args.push("'use strict'; return (!!this ? new (Function.prototype.bind.apply(" + originalRef + ", [{}].concat([].slice.call(arguments))))() : " + patches.called + ".apply(null, arguments));");
        }

        newRef = new (Function.prototype.bind.apply(Function, [{}].concat(args)))(); // Create a new function to wrap the patched constructor
        newRef.prototype = original.prototype; // Keep a reference to the original prototype to ensure instances of the patch appear as instances of the original
        newRef.prototype.constructor = newRef; // Ensure the constructor of patched instances is the patched constructor

        Object.getOwnPropertyNames(ref).forEach(function (property) { // Add any "static" properties of the original constructor to the patched one
            if (fnProps.indexOf(property) < 0) { // Don't include static properties of Function since the patched constructor will already have them
                newRef[property] = ref[property];
            }
        });

        return newRef; // Return the patched constructor
    };

}());

And here's an example usage (this is a real-world use case for working around a bug in date parsing in PhantomJS):

Date = patch(Date, "DateOriginal", {
    constructed: function () {
        var args = [].slice.call(arguments);
        if (typeof args[0] === "string" && /^\d{4}\/\d{2}$/.test(args[0])) {
            args[0] = args[0] + "/02"; // Make sure the argument has a 'day'
        }
        return new (Function.prototype.bind.apply(DateOriginal, [{}].concat(args)));
    }
});

There are some more examples, and details of the arguments to patch in the GitHub readme.

Answer 1

I don't see any good reason why you're using some of these hacks. It took me a while to understand what you were doing and I was pretty much forced to step through it in debugger. Going to review this line by line and point out simplifications and patterns as I see fit. I'll be batching together all my suggestions into a counter proposal in this jsbin, using the unit tests from your repo.

To start, it seems quite unneccessary and non standard to get the global scope your way (and I'm going to later argue you shouldn't need it)...

var global = new Function("return this;")();

Just do what every other package does and get scope at the start of your wrapper

(function(global) {  })(this); //or use self w/e

I'm not a fan of your fnProps idea. I would prefer you store has = Object.prototype.hasOwnProperty; and filter out items on the Function prototype as follows:

var has = Object.prototype.hasOwnProperty();
/* .... */
//To avoid storing generic function properties (like length) on newRef
Object.getOwnPropertyNames(ref).forEach(function(key) {
    if(!has.call(Function, key)) newRef[key] = ref[key];
});

I'm biased here, but as a past Mootools developer I keep comparing your function with Class.refactor. Personally I would prefer you add a reference to the original function on newRef - maybe as some property _original or let the user handle storing the original. I'm not a fan of adding a global for the original function.

A question, why do you not pass this to your constructors? If you're wrapping some classes this seems like it may make usuage less intuitive.

Update (still at work and can't look into removing the new Function() approach yet but I think I found a great change... You can rewrite the extremely confusing bind.apply(...) like this (unit tests passing):

newRef = Function.apply(null, args);

I'm not a minifier but theres no good reason to do !!this ? x : y

Counter proposal:

I believe you should drop the arity requirement and drop the global - instead inform the user they should cache the original locally. I've rewritten the code with these considerations in mind. Here's a jsbin with these changes (note I haven't removed the global to respect your test cases):

var patch = (function (global) {
    /*jshint evil: true */

    "use strict";
    var has = Object.prototype.hasOwnProperty,
        slice = Array.prototype.slice,
        bind = Function.bind;
    return function (original, patches) {
        var args = [],
            newRef, // This will be the new patched constructor
            i;

        patches.called = patches.called || original; // If we are not patching static calls just pass them through to the original function

        if (patches.constructed) { // This string is evaluated to create the patched constructor body in the case that we are patching newed calls
            newRef = function(/*args*/) {
                return (this && this !== global ? patches.constructed : patches.called).apply(this, arguments);//note called with context
            };
        } else { // This string is evaluated to create the patched constructor body in the case that we are only patching static calls
            newRef = function(/*args*/) {
                return this && this !== global  ? new (bind.apply(original, [].concat({}, slice.call(arguments))))
                                                : patches.called.apply(this, arguments);
            };
        }

        // Create a new function to wrap the patched constructor
        newRef.prototype = original.prototype; // Keep a reference to the original prototype to ensure instances of the patch appear as instances of the original
        newRef.prototype.constructor = newRef; // Ensure the constructor of patched instances is the patched constructor

        Object.getOwnPropertyNames(original).forEach(function (property) { // Add any "static" properties of the original constructor to the patched one
            if (!has.call(Function, property)) { // Don't include static properties of Function since the patched constructor will already have them
                newRef[property] = original[property];
            }
        });

        return newRef; // Return the patched constructor
    };

})(this);

Edit - missed an easy code size optimization:

newRef = function(/*args*/) {
    if(this && this !== global) {
        return patches.constructed ? patches.constructed.apply(this, arguments)
                                   : new (bind.apply(original, [].concat({}, slice.call(arguments))))// create the patched constructor body in the case that we are only patching static calls;
    } else {
        return patches.called.apply(this, arguments);
    }
};