From 49e0f14a2f9ad85ab5c9256e97d6ecb1a11fa936 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Thu, 15 Jul 2010 10:41:40 -0700
Subject: [PATCH] Cache modules based on filename rather than ID

This is ever so slightly less efficient than caching based on ID, since the
filename has to be looked up before we can check the cache.  However, it's
the most minimal approach possible to get this change in place.  Since
require() is a blocking startup-time operation anyway, a bit of slowness is
not a huge problem.

A test involving require.paths modification and absolute loading. Here's the
gist of it.

Files: /p1/foo.js /p2/foo.js

  1. Add "/p1" to require.paths.
  2. foo1 = require("foo")
  3. assert foo1 === require("/p1/foo") (fail)
  4. Remove /p1 from require.paths.
  5. Add /p2 to require.paths.
  6. foo2 = require("foo")
  7. assert foo1 !== foo2 (fail)
  8. assert foo2 === require("/p2/foo") (fail)

It's an edge case, but it affects how dependencies are mapped by npm.
If your module requires foo-1.2.3, and my module requires foo-2.3.4,
then you should expect to have require("foo") give you foo-1.2.3, and
I should expect require("foo") to give me foo-2.3.4.  However, with
module ID based caching, if your code loads *first*, then your "foo"
is THE "foo", so I'll get your version instead of mine.

It hasn't yet been a problem, but only because there are so few
modules, and everyone pretty much uses the latest version all the
time.  But as things start to get to the 1.x and 2.x versions, it'll
be an issue, I'm sure.  Dependency hell isn't fun, so this is a way to
avoid it before it strikes.
---
 lib/module.js                        | 77 +++++++++++++---------------
 test/fixtures/require-path/p1/bar.js |  8 +++
 test/fixtures/require-path/p1/foo.js |  2 +
 test/fixtures/require-path/p2/bar.js |  2 +
 test/fixtures/require-path/p2/foo.js |  2 +
 test/simple/test-module-loading.js   |  4 ++
 6 files changed, 54 insertions(+), 41 deletions(-)
 create mode 100644 test/fixtures/require-path/p1/bar.js
 create mode 100644 test/fixtures/require-path/p1/foo.js
 create mode 100644 test/fixtures/require-path/p2/bar.js
 create mode 100644 test/fixtures/require-path/p2/foo.js

diff --git a/lib/module.js b/lib/module.js
index 3d66cb19c0..313be6221f 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -22,7 +22,6 @@ function Module (id, parent) {
   } else {
     this.moduleCache = {};
   }
-  this.moduleCache[this.id] = this;
 
   this.filename = null;
   this.loaded = false;
@@ -223,52 +222,48 @@ function loadModule (request, parent, callback) {
 
   debug("loadModule REQUEST  " + (request) + " parent: " + parent.id);
 
-  var cachedModule = internalModuleCache[id] || parent.moduleCache[id];
-
-  if (!cachedModule) {
-    // Try to compile from native modules
-    if (natives[id]) {
-      debug('load native module ' + id);
-      cachedModule = loadNative(id);
-    }
+  // native modules always take precedence.
+  var cachedNative = internalModuleCache[id];
+  if (cachedNative) {
+    return callback ? callback(null, cachedNative.exports) : cachedNative.exports;
+  }
+  if (natives[id]) {
+    debug('load native module ' + id);
+    var nativeMod = loadNative(id);
+    return callback ? callback(null, nativeMod.exports) : nativeMod.exports;
   }
 
-  if (cachedModule) {
-    debug("found  " + JSON.stringify(id) + " in cache");
-    if (callback) {
-      callback(null, cachedModule.exports);
-    } else {
-      return cachedModule.exports;
+  // look up the filename first, since that's the cache key.
+  debug("looking for " + JSON.stringify(id) + " in " + JSON.stringify(paths));
+  if (!callback) {
+    // sync
+    var filename = findModulePath(request, paths);
+    if (!filename) {
+      throw new Error("Cannot find module '" + request + "'");
     }
 
-  } else {
-    // Not in cache
-    debug("looking for " + JSON.stringify(id) + " in " + JSON.stringify(paths));
-
-    if (!callback) {
-      // sync
-      var filename = findModulePath(request, paths);
-      if (!filename) {
-        throw new Error("Cannot find module '" + request + "'");
-      } else {
-        var module = new Module(id, parent);
-        module.loadSync(filename);
-        return module.exports;
-      }
+    var cachedModule = parent.moduleCache[filename];
+    if (cachedModule) return cachedModule.exports;
 
-    } else {
-      // async
-      findModulePath(request, paths, function (filename) {
-        if (!filename) {
-          var err = new Error("Cannot find module '" + request + "'");
-          callback(err);
-        } else {
-          var module = new Module(id, parent);
-          module.load(filename, callback);
-        }
-      });
-    }
+    var module = new Module(id, parent);
+    module.moduleCache[filename] = module;
+    module.loadSync(filename);
+    return module.exports;
   }
+  // async
+  findModulePath(request, paths, function (filename) {
+    if (!filename) {
+      var err = new Error("Cannot find module '" + request + "'");
+      return callback(err);
+    }
+
+    var cachedModule = parent.moduleCache[filename];
+    if (cachedModule) return callback(null, cachedModule.exports);
+
+    var module = new Module(id, parent);
+    module.moduleCache[filename] = module;
+    module.load(filename, callback);
+  });
 };
 
 
diff --git a/test/fixtures/require-path/p1/bar.js b/test/fixtures/require-path/p1/bar.js
new file mode 100644
index 0000000000..cc67811001
--- /dev/null
+++ b/test/fixtures/require-path/p1/bar.js
@@ -0,0 +1,8 @@
+var path = require("path");
+
+require.paths.unshift(path.join(__dirname,"../p2"));
+
+exports.foo = require("foo");
+
+exports.expect = require(path.join(__dirname, "../p2/bar"));
+exports.actual = exports.foo.bar;
diff --git a/test/fixtures/require-path/p1/foo.js b/test/fixtures/require-path/p1/foo.js
new file mode 100644
index 0000000000..6e926f2746
--- /dev/null
+++ b/test/fixtures/require-path/p1/foo.js
@@ -0,0 +1,2 @@
+require.paths.unshift(__dirname);
+exports.bar = require("bar");
diff --git a/test/fixtures/require-path/p2/bar.js b/test/fixtures/require-path/p2/bar.js
new file mode 100644
index 0000000000..2ffb5ed1b2
--- /dev/null
+++ b/test/fixtures/require-path/p2/bar.js
@@ -0,0 +1,2 @@
+
+exports.INBAR = __filename;
diff --git a/test/fixtures/require-path/p2/foo.js b/test/fixtures/require-path/p2/foo.js
new file mode 100644
index 0000000000..ebf2ac288c
--- /dev/null
+++ b/test/fixtures/require-path/p2/foo.js
@@ -0,0 +1,2 @@
+require.paths.unshift(__dirname);
+exports.bar = require("bar"); // surprise! this is not /p2/bar, this is /p1/bar
diff --git a/test/simple/test-module-loading.js b/test/simple/test-module-loading.js
index e2676f9e12..23574643a9 100644
--- a/test/simple/test-module-loading.js
+++ b/test/simple/test-module-loading.js
@@ -98,6 +98,10 @@ require.registerExtension('.test', function(content) {
 });
 
 assert.equal(require('../fixtures/registerExt2').custom, 'passed');
+debug("load modules by absolute id, then change require.paths, and load another module with the same absolute id.");
+// this will throw if it fails.
+var foo = require("../fixtures/require-path/p1/foo");
+process.assert(foo.bar.expect === foo.bar.actual);
 
 process.addListener("exit", function () {
   assert.equal(true, a.A instanceof Function);