Skip to content

module: revert checking for package.json before extensions #15015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bmeck wants to merge 3 commits into from
Closed

module: revert checking for package.json before extensions #15015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a43f20
module: check file ext before dir as documented
bmeck Aug 24, 2017
39f51aa
fixup: address linter & test issue
BridgeAR Feb 17, 2018
c3e5b81
fixup: remove added new line
BridgeAR Feb 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand All @@ -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);
}
Copy link
Member

@jdalton jdalton Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👆 at the moment because the trailing slash check is for forward-slash-only, on Windows paths ending in a backslash hit that code path (which is removed in this PR). This means it will now fall through to the tryExtensions call on line 234, instead of the tryPackage call on line 243.

Note: Assumes a fixed #18299


if (!filename) {
Expand All @@ -247,11 +246,13 @@ Module._findPath = function(request, paths, 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) {
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/module-extension-over-directory/inner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = {};
3 changes: 3 additions & 0 deletions test/fixtures/module-extension-over-directory/inner/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"main": "./package.json"
}
30 changes: 30 additions & 0 deletions test/parallel/test-require-extension-over-directory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'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'
Copy link
Member

@BridgeAR BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a new eslint rule in place that prevents messages without dynamic content (or it is at least planned). I do see that it makes sense to have a individual message here, so we should either explicitly accept the message here or maybe just change the inner.js export content to contain a property with a string, remove the message and use deepStrictEqual. With the latter the error message should be explicit enough.

Copy link
Member

@Trott Trott Dec 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR If that rule ever comes to pass, we can always disable it with a comment for the instances where a string literal makes sense. So this can stay as is, if the proposed lint rule is the only motivation for changing it.

);

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'
);