The Module Pattern is a way of using a closure in JavaScript to create private variables and functions. Here's a brief recap:

var myObject = (function() {
  //these are only accessible internally
  var privateVar = 'this is private';
  var privateFunction = function() {
    return "this is also private";
  };
  
  return {
    //these can be accessed externally
    publicVar: 'this is public',
    
    publicFunction: function() {
      return "this is also public"
    },

    //this is a 'privileged' function - it can access the internal private vars
    myFunction: function() {
      return privateVar;
    }
  };
})();

myObject.privateVar; //returns null as private var is private
myObject.myFunction(); //return the private var as myFunction has access to private properties

var myObject = (function() {
  //these are only accessible internally
  var privateVar = 'this is private';
  var privateFunction = function() {
    return "this is also private";
  };
  
  return {
    //these can be accessed externally
    publicVar: 'this is public',
    
    publicFunction: function() {
      return "this is also public"
    },

    //this is a 'privileged' function - it can access the internal private vars
    myFunction: function() {
      return privateVar;
    }
  };
})();

myObject.privateVar; //returns null as private var is private
myObject.myFunction(); //return the private var as myFunction has access to private properties

Breaking this down, we create a function which is executed immediately (via the brackets at the end) and returns an object which gets assigned to myObject.

Because this object contains references to our private variable (privateVar is referenced inside myFunction), the JavaScript engine keeps privateVar available in memory which means myFunction can still access it using what is called a closure. This pattern as a whole is usually called the Module Pattern.

Why it's bad

On the face of it, private variables sound like a good thing. We have them in other languages after all, so why not in JavaScript too?

The reason that you shouldn't use the Module pattern 90% of the time you think you should is that it entirely negates the dynamic nature of the language. If a class does 99% of what you want and you (rightly) don't want to directly modify the source code, you will be thwarted every time if the class uses this pattern.

Example

I'll share a recent example of this using a class in the Ext JS library. Ext is by no means the only library guilty of this, but it's the one I use on a daily basis, and this is not the only example of this problem in the library.

The Ext.DomQuery object is a helper which allows us to parse XML documents locally. Unfortunately, it suffers from a limitation which causes the text content of an XML node to be truncated if it is over a certain size limit (just 4kb in Firefox, though this differs by browser). This isn't actually a problem of Ext's making, though it can solve it using just 1 line of code.

Ideally, we'd just be able to do this:

Ext.apply(Ext.DomQuery, {
  selectValue : function(path, root, defaultValue){
    path = path.replace(trimRe, "");
    if(!valueCache[path]) valueCache[path] = Ext.DomQuery.compile(path, "select");
    
    var n = valueCache[path](root), v;
    n = n[0] ? n[0] : n;
    
    //this line is the only change
    if (typeof n.normalize == 'function') n.normalize();
    
    v = (n && n.firstChild ? n.firstChild.nodeValue : null);
    return ((v === null||v === undefined||v==='') ? defaultValue : v);
  }
});

Ext.apply(Ext.DomQuery, {
  selectValue : function(path, root, defaultValue){
    path = path.replace(trimRe, "");
    if(!valueCache[path]) valueCache[path] = Ext.DomQuery.compile(path, "select");
    
    var n = valueCache[path](root), v;
    n = n[0] ? n[0] : n;
    
    //this line is the only change
    if (typeof n.normalize == 'function') n.normalize();
    
    v = (n && n.firstChild ? n.firstChild.nodeValue : null);
    return ((v === null||v === undefined||v==='') ? defaultValue : v);
  }
});

All we're doing in the above is making a call to 'normalize' - a single line change which fixes the 4kb node text limitation. Sadly though, we can't actually do this because of the use of the Module pattern. In this example there are two private variables being accessed - 'trimRe' and 'valueCache'.

We can't get access to these private variables in our override, which means that our override here fails. In fact, the Module pattern means we can't actually patch this at all.

The only way to do it is to modify the source code of Ext JS itself, which is a very dangerous practice as you need to remember every change you made to ext-all.js and copy them all over next time you upgrade.

Even if there are good reasons for enforcing the privacy of variables (in this case I don't think there are), we could get around this by providing a privileged function which returns the private variable - essentially making it read-only:

Ext.DomQuery.getValueCache = function() {
  return valueCache;
};

Ext.DomQuery.getValueCache = function() {
  return valueCache;
};

Except again this needs to be defined inside the original closure - we just can't add it later. Again we would have to modify the original source code, with all the problems that entails.

Ext.ComponentMgr does the same trick when registering xtypes. An xtype is just a string that Ext maps to a constructor to allow for easy lazy instantiation. The trouble is, Ext hides the xtype lookup object inside a private variable, meaning that if you have an xtype string it is impossible to get a reference to the constructor function for that xtype. Ext provides a function to instantiate an object using that constructor, but doesn't let you get at the constructor itself. This is totally unnecessary.

Recommendations

• Think very carefully before using the Module pattern at all. Do you really need to enforce privacy of your variables? If so, why?
• If you absolutely have to use private variables, consider providing a getter function which provides read-only access to the variables
• Keep in mind that once defined, private variables defined this way cannot be overwritten at all. In other languages you can often overwrite a superclass's private variables in a subclass - here you can't

Either of the above would have solved both problems, but as neither was implemented we have to fall back to hackery.

And remember this about the Module pattern:

• It's overused - in the examples above (especially Ext.ComponentMgr) there is no benefit from employing the pattern
• It's dangerous - because of its inflexibility it forces us to modify external source code directly - changes you are almost guaranteed to forget about when it comes to updating the library in the future
• It's bloody annoying - because of both of the above.