JavaScript Module pattern – overused, dangerous and bloody annoying
October 5, 2009 21 Comments
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
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);
}
});
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;
};
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.
Ed Spencer 
Nice write-up an good that you bring that up.
I also wonder about the benefits of strictly private methods and variables.
One reason might be to ensure that extension developers don’t touch these to make extensions less sensitive to future internal changes and therefore HELP the Ext JS ecosystem.
On the other hand it annoys developers which HURTS the Ext JS ecosystem.
Before continuing on this benefit/problem though, I rather wait for a comment from the Ext JS team first to see of what benefits they originally thought. 🙂
Cheers,
Steffen
I had an issue with Ext.DomQuery recently where a client needed a slight over-ride to solve an IE issue.
I solved it buy use Function#createInterceptor, returning false to disable the default DomQuery method from running.
In your case, you might solve your issue by using Function#createSequence.
Ext.DomQuery.selectValue = Ext.DomQuery.selectValue.createSequence(function(path, root, defaultValue) {
// your code here
}).createDelegate(Ext.DomQuery);
@Steffen I think that because it was a fairly new and shiny pattern, and sounded like a good idea it got overused. I know that the first time I encountered it all of my classes immediately started using it, when none of them should have done.
@Chris Those Function methods are great to know about, but unfortunately wouldn’t help me in this situation – usually it would but again the private variables on DomQuery stop it from working 😦
In this specific case, there are 2 solutions:
1. pass ‘trimRe’ and ‘valueCache’ to selectValue as parameters
2. make trimRe and valueCache available
I think the reason why none of this is done is because they (ExtJS developers) do not foresee the need of overwrite selectValue method.
IMHO, this kind of problem is an implementation issue, not because the module pattern is used. As a library author, people have to take great care on designing the API. However as an application developer, you can do anything to the source, thus this should not be a problem.
Thanks for the feedback Guoliang, though I think we may have crossed wires here. In this specific case solution (1) isn’t a solution at all, because you can’t get a hold of trimRe or valueCache in the first place, meaning you can’t pass them in as parameters. Even if you could, you’d need to rewrite the selectValue method to accept them or fall back to the private variables.
I agree that the need wasn’t foreseen and suggest that the default position is to not use this pattern unless it’s *really* needed (which is almost never).
Yes, it is an implementation issue, but it’s precisely because the Module pattern was used. The pattern forms the basis of the implementation. And while you *can* do anything to the source, it’s usually a bad idea to modify it directly – far better to selectively override, which in this case is an option sadly denied to us.
Hey, nice writeup. I’m currently in the process of re-factoring Ext Core in some places to make it faster and more extendable again. In the ComponentMgr there is actually a reference to the private variable on the public ComponentMgr class called ‘all’. In the case of the selectValue in DomQuery I’ve decided to just apply your change to our codebase. I agree with you that the Module Pattern was being overused in core in some cases, but there is currently one benefit to it at the moment. Because we define all those variables not on the public object, they can be renamed by compressors (like YUI compressor). Although extendability is probably more important then a couple of Kb’s in the long run, right now refactoring classes like DomQuery will make Ext Core bigger in size, take some time, and might introduce unexpected behavior and bugs.
Thanks Tommy, glad we’re on the same page 🙂
I think the ‘all’ variable is actually the MixedCollection of all instances, rather than the registered xtypes – what I was after was the mapping of xtype string to constructor function so that I could instantiate myself rather than using Ext.ComponentMgr.create. As far as I can see from looking at the code, that ‘types’ object is absolutely private, with no externally-readable aliases.
It would be an interesting (though labour intensive) experiment to see how much space is actually saved with the private var approach. Personally I see that as pretty irrelevant unless you’re writing a library for a mobile platform… and even then it’s kinda questionable if it’s only a difference of a kb or two 🙂
This is fair. One should be careful only to privatize those functions and variables that really aren’t intended to be replaced. OTOH, one shouldn’t just expose the entire guts of one’s objects for clients to play with and tweak at their whim. As with any pattern, you just have to make decisions about how far to take it.
What you appear to be saying is that if a few library functions here and there each require a slight patch, then as clients of the library, we should be able to effectively merge in a bunch of patches at runtime, every time our program starts up. And then maybe hit ourselves over the head with hammers, for extra fun.
Your problem here is a software revision control problem, not a runtime object model problem.
The reason a library needs to make variables like ‘trimRe’ and ‘valueCache’ private is because they may not exist at all in the next version of the library. Or they may exist but have totally or slightly different meanings and uses. By making them private, the library is allowing itself the ability to change in the future without breaking client applications.
Compared with the current version of the library, your replacement function only differs in one line. But compared with future versions of the library, it may be just wildly wrong, or only subtly wrong in a way that is harder to spot.
A new version of the library will no doubt render your patch either irrelevant (as in this case) or broken, no matter how you apply your patch.
You cannot actually solve that problem by doing what you tried to do.
What you need is software revision control. You don’t even need a fancy software revision control system to do it.
1. Get the first version of the library source and add it to your source tree.
2. Make a zip/gzip/tar of that pristine original version and keep it in your source tree also. This is the “snapshot”.
3. Now apply your patches to the library source. Consider complaining about these problems on your blog.
4. Get on with your life. Keep applying patches as necessary. Leave the snapshot alone.
5. When a new library version comes out, unpack your snapshot and use xxdiff or WinMerge to compare it with your patched source, to effortlessly find all the places where you patched it.
6. For each individual change, find the corresponding place in the new library version, and identify whether the patch is still needed (the library author may have read your blog) or whether it needs to be completely reworked due to changes in the library internals. When you’ve figured this out, make the required change – if any – to the new source. With big library revisions, the change may need to go in a totally different source file, and be completely rewritten.
7. Replace your snapshot with a zip/gzip/tar of the unpatched new library.
8. Go to 4.
Note that step 6 is essential regardless of how you patch the library. And with a good merge tool, with a 3-pane view, it’s child’s play. You MUST re-examine all your patches every time you take a new library version, and probably discard or at least rework them. This is true WHATEVER technique you use to apply the patches. You don’t really gain anything by butchering the in-memory instance of the library at start-up time.
In fact using a merge tool is if anything easier. Looking at your example, only one line has been added. That one line will light up in a highlighting box in the GUI of a merging tool.
But if – as you wished to do – you made a copy of the original function and then added a line to it, and stored that somewhere separately in your own source, you have less clarity about what has changed, and no idea of the context that the patched code has to operate within, because you ripped it out of that context.
In summary: the module pattern is utterly essential in library construction. It should be used by default. When in doubt, keep stuff private. It’s the whole point of libraries. The public stuff is the part that the library author can’t change in their next version. The private stuff is the part they can change.
The problem here is software revision control, and does not actually constitute a valid criticism of private data in libraries.
Agreed with Daniel here.
Your problem isn’t really specifically with the module pattern.. it’s with private methods and data hiding in general, which is a totally different topic. Once you have a non-trivial number of collaborators, you need some way to disseminate a controlled API, and that’s what the module pattern gives you.
I’ve been frustrated in stores like Autozone where the moron behind the counter has no idea what I am talking about. It would be much easer for me to step behind the counter and get the item myself. There are good reasons why customers aren’t allowed to to do this. One is that the store needs to be confident that their inventory remains organized according to their specifications.
Modularity comes at the price of inconvenience. Over the history of software development, it has been found that building complex structures where classes that know nothing about each other have to collaborate demands this kind of insulation and that allowing short cuts can lead to chaos.
It’s partly a matter of scale. In small designs the overhead imposed by modularity is not worth the cost but as the code scales up, at some point it becomes crucial to be certain that internal data will only be modified in ways the designer expects.
It seems to me that the real problem with the XML parser you mention is that it’s badly designed. If you are designing for extensibility, you dont hard code limitations like that. You set them as a default value and provide some mechanisim to change this option.
Pingback: Patrones de Diseño y JavaScript: Module « Aijoona
Why would i want to monkey patch ExtJS in the first place? Module pattern is a good thing because it denies monkey patching of internal things. Module provides you a dynamic API, which you can use if it does the right things for you. If it doesn’t – just don’t use this module, or if you are this module’s author – refactor it. But don’t use Ext.apply and Ext.override APIs to reach immediate effect and possibly harm future developers who will need to workaround your patches. It is bloody annoying.
@Anton that’s a problem too, but in the opposite direction. The problem with a closure approach here is that it’s simply not possible to patch it without rewriting the whole thing, which leaves you utterly at the mercy of the developer and his/her timetable on fixing any problems. I don’t like being in that situation so am offering my thoughts on the approach – I’m not telling you that you have to do the same
This is interesting. Im having trouble because the team decided we should use the Module Pattern all around, and Im facing lots of trouble with it.
A friend that is my current javascript guru – he is a designer, not a programmer, but know a hell-lot about javascript – said me the following:
Your problem is that you are trying to use javascript as you use C#.
You dont need the extra complication of closures to solve your problems.
He was right. Again 🙂
Cheers, and thanks for the article.
I could capture this entire article in two sentences:
“The module pattern enforces private members, so don’t use the module pattern unless you actually want private members. Personally, I think private members are rarely necessary and it annoys me when they are created as an arbitrary by-product of the module pattern.”
@Gregt fair enough
@Ed application developers do not know the names of private members since they are not documented. When a class is inherited, naming conflicts may happen. This will cause unpredictable problems. There are two ways to solve this problem. One is to document all private members, the other is to hide them. I think the latter is better.
While after seeing the post title I thought I’d be outraged when reading (since all “good” JavaScript books promote this practice), I soon realised not only that you were right but also that I’d hit this wall before… at the time, I said to myself that there was something I was missing out but it seems that indeed, by using this pattern, one could easily create rigid modules that do not allow clients to override functionality…
Howdy great blog! Does running a blog like this take a massive
amount work? I have absolutely no expertise in computer
programming however I had been hoping to start my own blog in the near future.
Anyhow, should you have any suggestions
or techniques for new blog owners please share. I know this is off subject but I simply
wanted to ask. Thanks!
Very good information. Lucky me I found your site by chance (stumbleupon).
I’ve book marked it for later!