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.

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

Jump to bottom

module: CJS handle deleted directory relative require #42384

Merged
merged 2 commits into from
Oct 31, 2022
Merged

module: CJS handle deleted directory relative require #42384

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af0ee79
module: ensure relative requires work from deleted directories
bmeck Mar 17, 2022
dc3b380
do not use regexes in module loader
aduh95 Oct 29, 2022
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
37 changes: 25 additions & 12 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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,9 +539,6 @@ function resolveExports(nmPath, request) {
}
}

const trailingSlashRegex = /(?:^|\/)\.?\.$/;
const nixRelativeCheck = /^\.\.?(?:[/]|$)/;
const windowsRelativeCheck = /^\.\.?(?:[/\\]|$)/;
/**
* @param {string} request a relative or absolute file path
* @param {Array<string>} paths file system directories to search as file paths
Expand All @@ -561,14 +559,29 @@ 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 = RegExpPrototypeExec(isWindows ? windowsRelativeCheck : nixRelativeCheck, request) !== null;
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);
Expand Down