Simple JavaScript Module "Loader"

Question

I have written a simple JS module "loader" (loader is in quotes because it doesn't actually load the files) that is designed to resolve simple dependencies between modules. It is heavily inspired by module.js and takes some of it's design from it.

Here is the code itself:

(function() {
 function create() {
 var definitions = {},
 instances = {};
 // Returns whether or not a module with the provided id is defined.
 var defined = function(id) {
 return definitions.hasOwnProperty(id);
 };
 // Define a module with the provided id and definition.
 var define = function(id, definition) {
 if(defined(id)) {
 throw new Error('module already defined: ' + id);
 }
 definitions[id] = definition;
 };
 // Undefine a module with the provided id.
 var undefine = function(id) {
 if (!defined(id)) {
 throw new Error('module not defined: ' + id);
 }
 delete definitions[id];
 delete instances[id];
 };
 // Require a module with the provided id.
 var require = function(id) {
 var stack = [];
 var internalRequire = function(id) {
 if(!defined(id)) {
 throw new Error('module not defined: ' + id);
 }
 // If we have already seen this id on the require stack, we've got
 // some form of cyclic dependency.
 if (stack.indexOf(id) !== -1 && stack.push(id)) {
 throw new Error('cyclic dependency: ' + stack.join(' -> '));
 } else {
 stack.push(id);
 }
 // If we already have an instance for this module, return it.
 if (instances.hasOwnProperty(id)) {
 return instances[id];
 } else if (typeof(definitions[id]) === 'function') {
 // Otherwise if our definition is a function, call it and pass the
 // require method to it.
 return instances[id] = definitions[id].call(null, internalRequire);
 } else {
 // Otherwise just return the definition itself (useful for objects
 // e.g. constants).
 return instances[id] = definitions[id];
 }
 };
 return internalRequire(id);
 };
 return {
 define: define,
 undefine: undefine,
 require: require
 };
 }
 window.module = create();
})();

And some example usage:

module.define('person', function() {
 var Person = function(name) {
 Object.defineProperty(this, 'name', {
 value: name
 });
 };
 Person.prototype.speak = function(text) {
 return this.name + ': ' + text;
 };
 return Person;
});
module.define('main', function(require) {
 var Person = require('person');
 var jack = new Person('Jack'),
 rocky = new Person('Rocky');
 console.log(jack.speak('Hi Rocky!'));
 console.log(rocky.speak('Hi Jack!'));
});

I don't really have any "burning questions" about my code; I'd just like some feedback on what people think of it and whether anyone can see any improvements.

Answer 1

The names define and defined are too similar, which could lead to mistakes. I suggest renaming the latter to isDefined.

---

You could flatten some of the conditionals when all branches leave the block. For example:

// If we have already seen this id on the require stack, we've got
// some form of cyclic dependency.
if (stack.indexOf(id) !== -1 && stack.push(id)) {
 throw new Error('cyclic dependency: ' + stack.join(' -> '));
}
stack.push(id);
// If we already have an instance for this module, return it.
if (instances.hasOwnProperty(id)) {
 return instances[id];
}
if (typeof(definitions[id]) === 'function') {
 // Otherwise if our definition is a function, call it and pass the
 // require method to it.
 return instances[id] = definitions[id].call(null, internalRequire);
}
// Otherwise just return the definition itself (useful for objects
// e.g. constants).
return instances[id] = definitions[id];