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: CJS handle deleted directory relative require #42384

Merged
merged 2 commits into from Oct 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 40 additions & 10 deletions lib/internal/modules/cjs/loader.js
Expand Up @@ -130,9 +130,10 @@ const { validateString } = require('internal/validators');
const pendingDeprecation = getOptionValue('--pending-deprecation');

const {
CHAR_FORWARD_SLASH,
CHAR_BACKWARD_SLASH,
CHAR_COLON
CHAR_COLON,
CHAR_DOT,
CHAR_FORWARD_SLASH,
} = require('internal/constants');

const {
Expand Down Expand Up @@ -538,7 +539,12 @@ function resolveExports(nmPath, request) {
}
}

const trailingSlashRegex = /(?:^|\/)\.?\.$/;
/**
* @param {string} request a relative or absolute file path
* @param {Array<string>} paths file system directories to search as file paths
* @param {boolean} isMain if the request is the main app entry point
* @returns {string | false}
*/
Module._findPath = function(request, paths, isMain) {
const absoluteRequest = path.isAbsolute(request);
if (absoluteRequest) {
Expand All @@ -553,18 +559,42 @@ Module._findPath = function(request, paths, isMain) {
return entry;

let exts;
let trailingSlash = request.length > 0 &&
StringPrototypeCharCodeAt(request, request.length - 1) ===
CHAR_FORWARD_SLASH;
if (!trailingSlash) {
trailingSlash = RegExpPrototypeExec(trailingSlashRegex, request) !== null;
const trailingSlash = request.length > 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

I believe storing request.length in a separate variable will result in faster code execution since the next 30~40 lines call request.length

Copy link
Member

Choose a reason for hiding this comment

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

Historically caching length lookups caused a huge perf hit, since it defeats a JIT optimization - not sure if that’s still true, but i doubt caching the length can beat the JIT.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be up for landing this as is, and open a new PR to run benchmarks and see if that changes something.

(StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || (
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use charcode instead of:

Suggested change
(StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || (
(request[request.length - 1] === '/' || (

?

Copy link
Member

Choose a reason for hiding this comment

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

I believe charcode comparison is faster

Copy link
Member

Choose a reason for hiding this comment

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

in recent v8? I’d love to see benchmarks for that!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I'd be up for landing this as is, and open a new PR to run benchmarks and see if that changes something.

Copy link
Member

Choose a reason for hiding this comment

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

Totally agree, perf changes should be done separately with benchmarks - i was thinking the simple string comparison was simpler than the charCode, and thus the better choice absent said benchmarks.

StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_DOT &&
(
request.length === 1 ||
StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_FORWARD_SLASH ||
(StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_DOT && (
request.length === 2 ||
StringPrototypeCharCodeAt(request, request.length - 3) === CHAR_FORWARD_SLASH
))
)
));

const isRelative = StringPrototypeCharCodeAt(request, 0) === CHAR_DOT &&
(
request.length === 1 ||
StringPrototypeCharCodeAt(request, 1) === CHAR_FORWARD_SLASH ||
(isWindows && StringPrototypeCharCodeAt(request, 1) === CHAR_BACKWARD_SLASH) ||
(StringPrototypeCharCodeAt(request, 1) === CHAR_DOT && ((
request.length === 2 ||
StringPrototypeCharCodeAt(request, 2) === CHAR_FORWARD_SLASH) ||
(isWindows && StringPrototypeCharCodeAt(request, 2) === CHAR_BACKWARD_SLASH)))
);
let insidePath = true;
if (isRelative) {
const normalizedRequest = path.normalize(request);
if (StringPrototypeStartsWith(normalizedRequest, '..')) {
insidePath = false;
}
}

// For each path
for (let i = 0; i < paths.length; i++) {
// Don't search further if path doesn't exist
// Don't search further if path doesn't exist and request is inside the path
const curPath = paths[i];
if (curPath && _stat(curPath) < 1) continue;
if (insidePath && curPath && _stat(curPath) < 1) continue;

if (!absoluteRequest) {
const exportsResolved = resolveExports(curPath, request);
Expand Down
30 changes: 30 additions & 0 deletions test/parallel/test-require-enoent-dir.js
@@ -0,0 +1,30 @@
'use strict';

const common = require('../common');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const fs = require('fs');
const path = require('path');

tmpdir.refresh();

const fooPath = path.join(tmpdir.path, 'foo.cjs');
fs.writeFileSync(fooPath, '');

const dirPath = path.join(tmpdir.path, 'delete_me');
fs.mkdirSync(dirPath, {
recursive: true
});

const barPath = path.join(dirPath, 'bar.cjs');
fs.writeFileSync(barPath, `
module.exports = () => require('../foo.cjs').call()
`);

const foo = require(fooPath);
const unique = Symbol('unique');
foo.call = common.mustCall(() => unique);
const bar = require(barPath);

fs.rmSync(dirPath, { recursive: true });
assert.strict.equal(bar(), unique);