Skip to content
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.

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

module: resolve _-prefixed dependencies to real path #6460

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
89 changes: 51 additions & 38 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)) {
Expand All @@ -144,51 +182,25 @@ Module._findPath = function(request, paths, isMain) {
return Module._pathCache[cacheKey];
}

var exts;
const exts = Object.keys(Module._extensions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the lazy-loading of the extensions removed?

Copy link
Author

Choose a reason for hiding this comment

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

Simplified the control flow, since tryFindPath was split out in order to reduce redundancy (just like readFilePath).

More than happy to add it back in, but seemed like a very marginal cost to me.

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];
var curPath = paths[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this line was fine. It was just the other 3 that were causing problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I test again on const in loop, it will be TurboFan compiler

node v6.0.0
function testFn(a, b) {
  const foo = [1, 2, 3];
  let total = 0;
  for (const a of foo) {
    const c = a;
    total += c;
  }
  return total;
}
[disabled optimization for 0x3a6acdbf0661 <SharedFunctionInfo formatValue>, reason: TryCatchStatement]
6
[didn't find optimized code in optimized code map for 0x4dca4458ee9 <SharedFunctionInfo testFn>]
[compiling method 0x3b482c0c0669 <JS Function testFn (SharedFunctionInfo 0x4dca4458ee9)> using TurboFan]
[optimizing 0x3b482c0c0669 <JS Function testFn (SharedFunctionInfo 0x4dca4458ee9)> - took 1.139, 0.000, 0.000 ms]
Function is TurboFan compiler

Copy link
Contributor

Choose a reason for hiding this comment

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

@fengmk2 I was tracing this particular function (Module._findPath()) and this particular const usage did not result in aborted or disabled optimizations for the entire containing function.

Copy link
Author

Choose a reason for hiding this comment

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

Switched back to vars.

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 (!filename) {
if (exts === undefined)
exts = Object.keys(Module._extensions);
filename = tryPackage(basePath, exts, isMain);
}
var basePath = path.resolve(curPath, request);
var filename = tryFindPath(basePath, exts, isMain, isAbsolute);

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);
// 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) {
Expand Down Expand Up @@ -427,6 +439,7 @@ Module._resolveFilename = function(request, parent, isMain) {
}

var resolvedModule = Module._resolveLookupPaths(request, parent);

var id = resolvedModule[0];
var paths = resolvedModule[1];

Expand Down
1 change: 1 addition & 0 deletions test/fixtures/module-require-real-path/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports.internalRequire = require;

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports.dirname = __dirname;
Empty file.
49 changes: 49 additions & 0 deletions test/parallel/test-require-real-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'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 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');

// 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.unlinkSync(linkModuleSymlinkPath);
});

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.
assert.notEqual(_require('./real-module'), _require('_link-module'));
assert.equal(_require('_link-module').dirname, linkModuleSymlinkPath);