From 6564aaeee953148046a4e5de136546ed459cdac6 Mon Sep 17 00:00:00 2001 From: Alexander Gugel Date: Fri, 29 Apr 2016 02:09:35 +0100 Subject: [PATCH 1/5] module: resolve _-prefixed dependencies to real path Since #5950, `require('some-symlink')` no longer resolves the module to its real path, but instead uses the path of the symlink itself as a cache key. This change allows users to resolve dependencies to their real path when required using the _-prefix. Example using old mechanism (pre v6.0.0): --- node_modules/_real-module -> ../real-module real-modules/index.js require('./real_module') === require('real_module') Example using new mechanism (post v6.0.0): --- node_modules/real-module -> ../real-module real-modules/index.js require('./real_module') !== require('real_module') As discussed in #3402 --- lib/module.js | 89 +++++++++++-------- .../module-require-real-path/index.js | 1 + .../node_modules/real-module/index.js | 1 + .../real-module/index.js | 1 + test/parallel/test-require-real-path.js | 30 +++++++ 5 files changed, 83 insertions(+), 39 deletions(-) create mode 100644 test/fixtures/module-require-real-path/index.js create mode 100644 test/fixtures/module-require-real-path/node_modules/real-module/index.js create mode 100644 test/fixtures/module-require-real-path/real-module/index.js create mode 100644 test/parallel/test-require-real-path.js diff --git a/lib/module.js b/lib/module.js index fb233d6c4541fe..cd92b4bc726b96 100644 --- a/lib/module.js +++ b/lib/module.js @@ -103,20 +103,30 @@ function tryPackage(requestPath, exts, isMain) { if (!pkg) return false; var filename = path.resolve(requestPath, pkg); + return readPackagePath(filename, exts, isMain); +} + +function readPackagePath(filename, exts, isMain) { return tryFile(filename, isMain) || tryExtensions(filename, exts, isMain) || tryExtensions(path.resolve(filename, 'index'), exts, isMain); } +function readFilePath(requestPath, isMain) { + if (isMain) { + return fs.realpathSync(requestPath); + } + return path.resolve(requestPath); +} + // check if the file exists and is not a directory // resolve to the absolute realpath if running main module, // otherwise resolve to absolute while keeping symlinks intact. function tryFile(requestPath, isMain) { const rc = stat(requestPath); - if (isMain) { - return rc === 0 && fs.realpathSync(requestPath); + if (rc === 0) { + return readFilePath(requestPath, isMain); } - return rc === 0 && path.resolve(requestPath); } // given a path check a the file exists with any of the set extensions @@ -131,6 +141,34 @@ function tryExtensions(p, exts, isMain) { return false; } +function tryFindPath(basePath, exts, isMain, isAbsolute) { + var filename; + + if (isAbsolute) { + const rc = stat(basePath); + if (rc === 0) { // File. + filename = readFilePath(basePath, isMain); + } else if (rc === 1) { // Directory. + filename = tryPackage(basePath, exts, isMain); + } + + if (!filename) { + filename = tryExtensions(basePath, exts, isMain); + } + } + + if (!filename) { + filename = tryPackage(basePath, exts, isMain); + } + + if (!filename) { + // try it with each of the extensions at "index" + filename = tryExtensions(path.resolve(basePath, 'index'), exts, isMain); + } + + return filename; +} + var warned = false; Module._findPath = function(request, paths, isMain) { if (path.isAbsolute(request)) { @@ -144,52 +182,24 @@ Module._findPath = function(request, paths, isMain) { return Module._pathCache[cacheKey]; } - var exts; + const exts = Object.keys(Module._extensions); const trailingSlash = request.length > 0 && request.charCodeAt(request.length - 1) === 47/*/*/; + const isAbsolute = !trailingSlash; // For each path for (var i = 0; i < paths.length; i++) { // Don't search further if path doesn't exist const curPath = paths[i]; if (curPath && stat(curPath) < 1) continue; - var basePath = path.resolve(curPath, request); - var filename; - - if (!trailingSlash) { - const rc = stat(basePath); - if (rc === 0) { // File. - if (!isMain) { - filename = path.resolve(basePath); - } else { - filename = fs.realpathSync(basePath); - } - } else if (rc === 1) { // Directory. - if (exts === undefined) - exts = Object.keys(Module._extensions); - filename = tryPackage(basePath, exts, isMain); - } - if (!filename) { - // try it with each of the extensions - if (exts === undefined) - exts = Object.keys(Module._extensions); - filename = tryExtensions(basePath, exts, isMain); - } - } + // If _basePath is a symlink, use the real path rather than the path of the + // symlink as cache key. + const _basePath = path.resolve(curPath, '_' + request); + const basePath = path.resolve(curPath, request); - if (!filename) { - if (exts === undefined) - exts = Object.keys(Module._extensions); - filename = tryPackage(basePath, exts, isMain); - } - - if (!filename) { - // try it with each of the extensions at "index" - if (exts === undefined) - exts = Object.keys(Module._extensions); - filename = tryExtensions(path.resolve(basePath, 'index'), exts, isMain); - } + const filename = tryFindPath(_basePath, exts, true, isAbsolute) || + tryFindPath(basePath, exts, isMain, isAbsolute); if (filename) { // Warn once if '.' resolved outside the module dir @@ -427,6 +437,7 @@ Module._resolveFilename = function(request, parent, isMain) { } var resolvedModule = Module._resolveLookupPaths(request, parent); + var id = resolvedModule[0]; var paths = resolvedModule[1]; diff --git a/test/fixtures/module-require-real-path/index.js b/test/fixtures/module-require-real-path/index.js new file mode 100644 index 00000000000000..4cb8e20ddc6f42 --- /dev/null +++ b/test/fixtures/module-require-real-path/index.js @@ -0,0 +1 @@ +exports.internalRequire = require; diff --git a/test/fixtures/module-require-real-path/node_modules/real-module/index.js b/test/fixtures/module-require-real-path/node_modules/real-module/index.js new file mode 100644 index 00000000000000..c45ea24398672a --- /dev/null +++ b/test/fixtures/module-require-real-path/node_modules/real-module/index.js @@ -0,0 +1 @@ +exports.dirname = __dirname; diff --git a/test/fixtures/module-require-real-path/real-module/index.js b/test/fixtures/module-require-real-path/real-module/index.js new file mode 100644 index 00000000000000..c45ea24398672a --- /dev/null +++ b/test/fixtures/module-require-real-path/real-module/index.js @@ -0,0 +1 @@ +exports.dirname = __dirname; diff --git a/test/parallel/test-require-real-path.js b/test/parallel/test-require-real-path.js new file mode 100644 index 00000000000000..be5f3b0cff9208 --- /dev/null +++ b/test/parallel/test-require-real-path.js @@ -0,0 +1,30 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); + +const realModuleSymlinkPath = path.join(common.fixturesDir, + '/module-require-real-path/node_modules/_real-module'); + +const localRealModulePath = path.join(common.fixturesDir, + '/module-require-real-path/real-module'); + +// module-require-real-path exports its require function. That way we can test +// how modules get resolved **from** its. +const _require = require( + path.join(common.fixturesDir, '/module-require-real-path') +).internalRequire; + +process.on('exit', function() { + fs.unlinkSync(realModuleSymlinkPath); +}); + +fs.symlinkSync('../real-module', realModuleSymlinkPath); + +assert.equal(_require('./real-module'), _require('real-module')); +assert.equal(_require('real-module').dirname, localRealModulePath); + +// When required directly with the _-prefix, resolve to path of symlink. +assert.notEqual(_require('./real-module'), _require('_real-module')); +assert.equal(_require('_real-module').dirname, realModuleSymlinkPath); From 37b7b5eeecd5602e1e504ab5bea6d545e902333d Mon Sep 17 00:00:00 2001 From: Alexander Gugel Date: Fri, 29 Apr 2016 04:08:56 +0100 Subject: [PATCH 2/5] module: Change precedence of _-prefixed modules That way stat() only needs to be called once in most cases (assuming non _-prefixed symlinks are more common). --- lib/module.js | 4 ++-- .../node_modules/{real-module => link-module}/index.js | 0 test/parallel/test-require-real-path.js | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) rename test/fixtures/module-require-real-path/node_modules/{real-module => link-module}/index.js (100%) diff --git a/lib/module.js b/lib/module.js index cd92b4bc726b96..222fcd39ed29ae 100644 --- a/lib/module.js +++ b/lib/module.js @@ -198,8 +198,8 @@ Module._findPath = function(request, paths, isMain) { const _basePath = path.resolve(curPath, '_' + request); const basePath = path.resolve(curPath, request); - const filename = tryFindPath(_basePath, exts, true, isAbsolute) || - tryFindPath(basePath, exts, isMain, isAbsolute); + const filename = tryFindPath(basePath, exts, isMain, isAbsolute) || + tryFindPath(_basePath, exts, true, isAbsolute); if (filename) { // Warn once if '.' resolved outside the module dir diff --git a/test/fixtures/module-require-real-path/node_modules/real-module/index.js b/test/fixtures/module-require-real-path/node_modules/link-module/index.js similarity index 100% rename from test/fixtures/module-require-real-path/node_modules/real-module/index.js rename to test/fixtures/module-require-real-path/node_modules/link-module/index.js diff --git a/test/parallel/test-require-real-path.js b/test/parallel/test-require-real-path.js index be5f3b0cff9208..b6e4e1f2bfc219 100644 --- a/test/parallel/test-require-real-path.js +++ b/test/parallel/test-require-real-path.js @@ -7,6 +7,9 @@ const fs = require('fs'); const realModuleSymlinkPath = path.join(common.fixturesDir, '/module-require-real-path/node_modules/_real-module'); +const linkModuleSymlinkPath = path.join(common.fixturesDir, + '/module-require-real-path/node_modules/_link-module'); + const localRealModulePath = path.join(common.fixturesDir, '/module-require-real-path/real-module'); @@ -18,13 +21,15 @@ const _require = require( process.on('exit', function() { fs.unlinkSync(realModuleSymlinkPath); + fs.unlinkSync(linkModuleSymlinkPath); }); fs.symlinkSync('../real-module', realModuleSymlinkPath); +fs.symlinkSync('../real-module', linkModuleSymlinkPath); assert.equal(_require('./real-module'), _require('real-module')); assert.equal(_require('real-module').dirname, localRealModulePath); // When required directly with the _-prefix, resolve to path of symlink. -assert.notEqual(_require('./real-module'), _require('_real-module')); -assert.equal(_require('_real-module').dirname, realModuleSymlinkPath); +assert.notEqual(_require('./real-module'), _require('_link-module')); +assert.equal(_require('_link-module').dirname, linkModuleSymlinkPath); From 0ef459107bf1e18396f24ffc6c4a0facb27e3ee1 Mon Sep 17 00:00:00 2001 From: Alexander Gugel Date: Fri, 29 Apr 2016 04:15:36 +0100 Subject: [PATCH 3/5] module: Enable V8 performance optimizations const in for-loop can not be optimized. --- lib/module.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/module.js b/lib/module.js index 222fcd39ed29ae..a017023b0087fc 100644 --- a/lib/module.js +++ b/lib/module.js @@ -190,15 +190,15 @@ Module._findPath = function(request, paths, isMain) { // For each path for (var i = 0; i < paths.length; i++) { // Don't search further if path doesn't exist - const curPath = paths[i]; + var curPath = paths[i]; if (curPath && stat(curPath) < 1) continue; // If _basePath is a symlink, use the real path rather than the path of the // symlink as cache key. - const _basePath = path.resolve(curPath, '_' + request); - const basePath = path.resolve(curPath, request); + var _basePath = path.resolve(curPath, '_' + request); + var basePath = path.resolve(curPath, request); - const filename = tryFindPath(basePath, exts, isMain, isAbsolute) || + var filename = tryFindPath(basePath, exts, isMain, isAbsolute) || tryFindPath(_basePath, exts, true, isAbsolute); if (filename) { From ca804f56d3237cdab2c8d6dc7a9b369e9c1726dd Mon Sep 17 00:00:00 2001 From: Alexander Gugel Date: Fri, 29 Apr 2016 04:19:18 +0100 Subject: [PATCH 4/5] module: Only resolve to _-prefixed filename if needed * Fixes `test-cwd-enoent-preload.js` * Minor performance improvement --- lib/module.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/module.js b/lib/module.js index a017023b0087fc..249b01068209e8 100644 --- a/lib/module.js +++ b/lib/module.js @@ -193,13 +193,15 @@ Module._findPath = function(request, paths, isMain) { var curPath = paths[i]; if (curPath && stat(curPath) < 1) continue; - // If _basePath is a symlink, use the real path rather than the path of the - // symlink as cache key. - var _basePath = path.resolve(curPath, '_' + request); var basePath = path.resolve(curPath, request); + var filename = tryFindPath(basePath, exts, isMain, isAbsolute) - var filename = tryFindPath(basePath, exts, isMain, isAbsolute) || - tryFindPath(_basePath, exts, true, isAbsolute); + if (!filename) { + // If _basePath is a symlink, use the real path rather than the path of + // the symlink as cache key. + basePath = path.resolve(curPath, '_' + request); + filename = tryFindPath(basePath, exts, true, isAbsolute); + } if (filename) { // Warn once if '.' resolved outside the module dir From 994f7ae7bdeb3e9026acfb26bb904deb79291845 Mon Sep 17 00:00:00 2001 From: Alexander Gugel Date: Fri, 29 Apr 2016 18:05:49 +0100 Subject: [PATCH 5/5] module: Test _-require resolution for nested modules Ensure that `require('some-module/index.js')` is being resolved properly when `some-module` is a _-prefixed symlink. --- lib/module.js | 2 +- .../real-module/nested/index.js | 0 test/parallel/test-require-real-path.js | 14 ++++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/module-require-real-path/real-module/nested/index.js diff --git a/lib/module.js b/lib/module.js index 249b01068209e8..ee1a1fb10c0342 100644 --- a/lib/module.js +++ b/lib/module.js @@ -194,7 +194,7 @@ Module._findPath = function(request, paths, isMain) { if (curPath && stat(curPath) < 1) continue; var basePath = path.resolve(curPath, request); - var filename = tryFindPath(basePath, exts, isMain, isAbsolute) + var filename = tryFindPath(basePath, exts, isMain, isAbsolute); if (!filename) { // If _basePath is a symlink, use the real path rather than the path of diff --git a/test/fixtures/module-require-real-path/real-module/nested/index.js b/test/fixtures/module-require-real-path/real-module/nested/index.js new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/parallel/test-require-real-path.js b/test/parallel/test-require-real-path.js index b6e4e1f2bfc219..e21759b1a0fdd5 100644 --- a/test/parallel/test-require-real-path.js +++ b/test/parallel/test-require-real-path.js @@ -28,6 +28,20 @@ fs.symlinkSync('../real-module', realModuleSymlinkPath); fs.symlinkSync('../real-module', linkModuleSymlinkPath); assert.equal(_require('./real-module'), _require('real-module')); + +// Ensure that _-prefixed symlinks are being required properly when requesting a +// nested file. +assert.equal(_require('./real-module'), _require('real-module/index.js')); +assert.equal( + _require('./real-module/nested/index.js'), + _require('real-module/nested/index.js') +); +assert.equal( + _require('./real-module/nested'), + _require('real-module/nested') +); + +assert.equal(_require('./real-module/index.js'), _require('real-module')); assert.equal(_require('real-module').dirname, localRealModulePath); // When required directly with the _-prefix, resolve to path of symlink.