From 9a43f2071d0b6b755623f63b6115e7e8b680e224 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Thu, 24 Aug 2017 09:59:52 -0500 Subject: [PATCH 1/3] module: check file ext before dir as documented The documented resolution algorithm was regressed and started to search for package.json files prior to searching for file extensions when searching for a specifier. Oddly, it did not search for index files at same time it searched for package.json. This reverts that regression and restores the documented behavior of searching for file extensions prior to searching directories. Fixes: https://github.com/nodejs/node/issues/14990 PR-URL: https://github.com/nodejs/node/pull/15015 Reviewed-By: Matteo Collina Reviewed-By: Ruben Bridgewater --- lib/module.js | 16 +++++++----- .../module-extension-over-directory/inner.js | 1 + .../inner/package.json | 3 +++ .../test-require-extension-over-directory.js | 26 +++++++++++++++++++ ...require-extensions-same-filename-as-dir.js | 4 +-- 5 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/module-extension-over-directory/inner.js create mode 100644 test/fixtures/module-extension-over-directory/inner/package.json create mode 100644 test/parallel/test-require-extension-over-directory.js diff --git a/lib/module.js b/lib/module.js index be49fa8e8d7e99..5ea249d3898f7b 100644 --- a/lib/module.js +++ b/lib/module.js @@ -215,6 +215,9 @@ Module._findPath = function(request, paths, isMain) { var exts; var trailingSlash = request.length > 0 && request.charCodeAt(request.length - 1) === CHAR_FORWARD_SLASH; + if (!trailingSlash) { + trailingSlash = /(?:^|\/)\.?\.$/.test(request); + } // For each path for (var i = 0; i < paths.length; i++) { @@ -232,10 +235,6 @@ Module._findPath = function(request, paths, isMain) { } else { filename = toRealPath(basePath); } - } else if (rc === 1) { // Directory. - if (exts === undefined) - exts = Object.keys(Module._extensions); - filename = tryPackage(basePath, exts, isMain); } if (!filename) { @@ -244,14 +243,17 @@ Module._findPath = function(request, paths, isMain) { exts = Object.keys(Module._extensions); filename = tryExtensions(basePath, exts, isMain); } + } if (!filename && rc === 1) { // Directory. + // try it with each of the extensions at "index" if (exts === undefined) exts = Object.keys(Module._extensions); - filename = tryPackage(basePath, exts, isMain) || - // try it with each of the extensions at "index" - tryExtensions(path.resolve(basePath, 'index'), exts, isMain); + filename = tryPackage(basePath, exts, isMain); + if (!filename) { + filename = tryExtensions(path.resolve(basePath, 'index'), exts, isMain); + } } if (filename) { diff --git a/test/fixtures/module-extension-over-directory/inner.js b/test/fixtures/module-extension-over-directory/inner.js new file mode 100644 index 00000000000000..f053ebf7976e37 --- /dev/null +++ b/test/fixtures/module-extension-over-directory/inner.js @@ -0,0 +1 @@ +module.exports = {}; diff --git a/test/fixtures/module-extension-over-directory/inner/package.json b/test/fixtures/module-extension-over-directory/inner/package.json new file mode 100644 index 00000000000000..2ac72712f06882 --- /dev/null +++ b/test/fixtures/module-extension-over-directory/inner/package.json @@ -0,0 +1,3 @@ +{ + "main": "./package.json" +} diff --git a/test/parallel/test-require-extension-over-directory.js b/test/parallel/test-require-extension-over-directory.js new file mode 100644 index 00000000000000..2fb2d69ffbee18 --- /dev/null +++ b/test/parallel/test-require-extension-over-directory.js @@ -0,0 +1,26 @@ +'use strict'; +// fixes regression from v4 +require('../common'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); +const path = require('path'); + +const fixturesRequire = require( + fixtures.path('module-extension-over-directory', 'inner')); + +assert.strictEqual( + fixturesRequire, + require(fixtures.path('module-extension-over-directory', 'inner.js')), + 'test-require-extension-over-directory failed to import fixture' + + ' requirements' +); + +const fakePath = [fixtures.path('module-extension-over-directory', 'inner'), 'fake', '..'].join(path.sep); +const fixturesRequireDir = require(fakePath); + +assert.strictEqual( + fixturesRequireDir, + require(fixtures.path('module-extension-over-directory', 'inner/')), + 'test-require-extension-over-directory failed to import fixture' + + ' requirements' +); diff --git a/test/parallel/test-require-extensions-same-filename-as-dir.js b/test/parallel/test-require-extensions-same-filename-as-dir.js index 1084d1d4581329..a7c4b482b5156f 100644 --- a/test/parallel/test-require-extensions-same-filename-as-dir.js +++ b/test/parallel/test-require-extensions-same-filename-as-dir.js @@ -28,5 +28,5 @@ const content = require(fixtures.path('json-with-directory-name-module', 'module-stub', 'one', 'two', 'three.js')); -assert.notStrictEqual(content.rocko, 'artischocko'); -assert.strictEqual(content, 'hello from module-stub!'); +assert.strictEqual(content.rocko, 'artischocko'); +assert.notStrictEqual(content, 'hello from module-stub!'); From 39f51aa6b42fba1b5dfb0251e02002f07be81dc2 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 17 Feb 2018 06:27:48 +0100 Subject: [PATCH 2/3] fixup: address linter & test issue --- test/parallel/test-require-extension-over-directory.js | 6 +++++- .../test-require-extensions-same-filename-as-dir.js | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-require-extension-over-directory.js b/test/parallel/test-require-extension-over-directory.js index 2fb2d69ffbee18..cc896d7b61b090 100644 --- a/test/parallel/test-require-extension-over-directory.js +++ b/test/parallel/test-require-extension-over-directory.js @@ -15,7 +15,11 @@ assert.strictEqual( ' requirements' ); -const fakePath = [fixtures.path('module-extension-over-directory', 'inner'), 'fake', '..'].join(path.sep); +const fakePath = [ + fixtures.path('module-extension-over-directory', 'inner'), + 'fake', + '..' +].join(path.sep); const fixturesRequireDir = require(fakePath); assert.strictEqual( diff --git a/test/parallel/test-require-extensions-same-filename-as-dir.js b/test/parallel/test-require-extensions-same-filename-as-dir.js index a7c4b482b5156f..1084d1d4581329 100644 --- a/test/parallel/test-require-extensions-same-filename-as-dir.js +++ b/test/parallel/test-require-extensions-same-filename-as-dir.js @@ -28,5 +28,5 @@ const content = require(fixtures.path('json-with-directory-name-module', 'module-stub', 'one', 'two', 'three.js')); -assert.strictEqual(content.rocko, 'artischocko'); -assert.notStrictEqual(content, 'hello from module-stub!'); +assert.notStrictEqual(content.rocko, 'artischocko'); +assert.strictEqual(content, 'hello from module-stub!'); From c3e5b8114d4d0dce84283b3e902510b8294e6689 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 17 Feb 2018 06:31:12 +0100 Subject: [PATCH 3/3] fixup: remove added new line --- lib/module.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/module.js b/lib/module.js index 5ea249d3898f7b..dfdbbaef53b628 100644 --- a/lib/module.js +++ b/lib/module.js @@ -243,7 +243,6 @@ Module._findPath = function(request, paths, isMain) { exts = Object.keys(Module._extensions); filename = tryExtensions(basePath, exts, isMain); } - } if (!filename && rc === 1) { // Directory.