Everything tagged opinion (2 posts)

JavaScript Module pattern - overused, dangerous and bloody annoying

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.
Continue reading

Why you should be using History in your ExtJS applications

I've been making a few updates to the ExtJS API documents application recently. The actual updates include remembering which tabs you have open and using Ext.History to go between tabs (you can follow the forum post or see a beta version).

That's not quite ready yet, but what has been made very clear to me is that any ExtJS application with more than one view should be using Ext.History. With History we get urls inside the application itself, we can parse them and dispatch accordingly. For example, I'm using a Rails-like Router, which lets you define an internal url map like this:


map.connect(":controllers/:action/:id");
map.connect(":controllers/:action");

map.connect(":controllers/:action/:id");
map.connect(":controllers/:action");

The router knows how to decode urls based on the regular expression-like syntax above, and parse the matches into an object - for example:


#users/new <= becomes {controller: 'users', action: 'new'}
#users/edit/2 <= becomes {controller: 'users', action: 'edit', id: 2}
#colours <= becomes {controller: 'colours'}

#users/new <= becomes {controller: 'users', action: 'new'}
#users/edit/2 <= becomes {controller: 'users', action: 'edit', id: 2}
#colours <= becomes {controller: 'colours'}

You can of course define any url matching scheme using the connect() function. I then use a simple Dispatcher, which looks at the decoded parameters. It finds the appropriate controller and calls that action on the controller, passing any other parameters as arguments. For example:


#users/new <= calls UsersController's "new" action
#colours/edit/2 <= calls ColoursController's "edit" action, with {id: 2} as the argument

#users/new <= calls UsersController's "new" action
#colours/edit/2 <= calls ColoursController's "edit" action, with {id: 2} as the argument

And so on. Each controller knows what to do for that action. It's easy then to say to someone "go to http://myapp.com/admin#users/152/comments" - which will take them straight to the comments that user 152 has written. Compare that with saying: "go to http://myapp.com/admin, then click the List Users tab, then find the user called Joe Bloggs, then double click the bubble icon next to his name". It's obvious which approach is better.

You don't even need to use something as elaborate as a router, just a simple switch statement or some regular expressions would be enough for many applications. Once you've got Ext.History setup, you could do something as simple as:


//decodes a url and decides how to dispatch it
dispatch = function(token) {
switch (token) {
case "users" : displayUsers(); break;
case "users/new": displayNewUser(); break;
case "users/2/edit: editUser(2); break;
default: displayDefault(); break;
};
};
Ext.History.on('change', dispatch);

//Call dispatch on initial page load as Ext.History's change event is not fired here
Ext.History.init(function() {
var token = document.location.hash.replace("#", "");
dispatch(token);
});

//decodes a url and decides how to dispatch it
dispatch = function(token) {
switch (token) {
case "users" : displayUsers(); break;
case "users/new": displayNewUser(); break;
case "users/2/edit: editUser(2); break;
default: displayDefault(); break;
};
};
Ext.History.on('change', dispatch);

//Call dispatch on initial page load as Ext.History's change event is not fired here
Ext.History.init(function() {
var token = document.location.hash.replace("#", "");
dispatch(token);
});

Obviously you don't hard code user IDs like that but it's easy to see how to roll your own. With just a few lines of code, you've decoded a url into a function to call, which can do anything you need it to. All your internal navigation needs to do is call Ext.History.add("some/new/url"), which will now be picked up by your dispatch code.

It's important to only route like this for idempotent actions (i.e. actions which display data rather than change it), so that data changing actions are not repeated. This is equivalent to using GET and POST correctly in normal web applications.

When the simplest implementation takes just a few lines of code, what reason could there be not to be using it?

Continue reading