Skip to content
3 changes: 2 additions & 1 deletion lib/internal/freeze_intrinsics.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const {
Int32ArrayPrototype,
Int8Array,
Int8ArrayPrototype,
IteratorPrototype,
Map,
MapPrototype,
Number,
Expand Down Expand Up @@ -211,7 +212,7 @@ module.exports = function() {

// 27 Control Abstraction Objects
// 27.1 Iteration
ObjectGetPrototypeOf(ArrayIteratorPrototype), // 27.1.2 IteratorPrototype
IteratorPrototype, // 27.1.2 IteratorPrototype
// 27.1.3 AsyncIteratorPrototype
ObjectGetPrototypeOf(ObjectGetPrototypeOf(ObjectGetPrototypeOf(
(async function*() {})()
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ function copyPrototype(src, dest, prefix) {
copyPrototype(original.prototype, primordials, `${name}Prototype`);
});

primordials.IteratorPrototype = Reflect.getPrototypeOf(primordials.ArrayIteratorPrototype);

/* eslint-enable node-core/prefer-primordials */

const {
Expand Down
53 changes: 20 additions & 33 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {
ArrayPrototypeSlice,
FunctionPrototypeBind,
Int8Array,
IteratorPrototype,
Number,
ObjectCreate,
ObjectDefineProperties,
Expand All @@ -20,9 +21,11 @@ const {
ReflectGetOwnPropertyDescriptor,
ReflectOwnKeys,
String,
StringPrototypeCharAt,
StringPrototypeCharCodeAt,
StringPrototypeCodePointAt,
StringPrototypeIncludes,
StringPrototypeReplace,
StringPrototypeIndexOf,
StringPrototypeReplaceAll,
StringPrototypeSlice,
StringPrototypeSplit,
Expand Down Expand Up @@ -139,11 +142,6 @@ function lazyCryptoRandom() {
return cryptoRandom;
}

// https://tc39.github.io/ecma262/#sec-%iteratorprototype%-object
Copy link
Member

Choose a reason for hiding this comment

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

Nit: should we add this url to primordials declaration too?

const IteratorPrototype = ObjectGetPrototypeOf(
ObjectGetPrototypeOf([][SymbolIterator]())
);

// Refs: https://html.spec.whatwg.org/multipage/browsers.html#concept-origin-opaque
const kOpaqueOrigin = 'null';

Expand Down Expand Up @@ -1377,7 +1375,7 @@ defineIDLClass(URLSearchParamsIteratorPrototype, 'URLSearchParams Iterator', {
},
[]
);
const breakLn = inspect(output, innerOpts).includes('\n');
const breakLn = StringPrototypeIncludes(inspect(output, innerOpts), '\n');
const outputStrs = ArrayPrototypeMap(output, (p) => inspect(p, innerOpts));
let outputStr;
if (breakLn) {
Expand Down Expand Up @@ -1435,7 +1433,7 @@ function getPathFromURLWin32(url) {
let pathname = url.pathname;
for (let n = 0; n < pathname.length; n++) {
if (pathname[n] === '%') {
const third = pathname.codePointAt(n + 2) | 0x20;
const third = StringPrototypeCodePointAt(pathname, n + 2) | 0x20;
if ((pathname[n + 1] === '2' && third === 102) || // 2f 2F /
(pathname[n + 1] === '5' && third === 99)) { // 5c 5C \
throw new ERR_INVALID_FILE_URL_PATH(
Expand All @@ -1456,13 +1454,13 @@ function getPathFromURLWin32(url) {
return `\\\\${domainToUnicode(hostname)}${pathname}`;
}
// Otherwise, it's a local path that requires a drive letter
const letter = pathname.codePointAt(1) | 0x20;
const sep = pathname[2];
const letter = StringPrototypeCodePointAt(pathname, 1) | 0x20;
const sep = StringPrototypeCharAt(pathname, 2);
if (letter < CHAR_LOWERCASE_A || letter > CHAR_LOWERCASE_Z || // a..z A..Z
(sep !== ':')) {
throw new ERR_INVALID_FILE_URL_PATH('must be absolute');
}
return pathname.slice(1);
return StringPrototypeSlice(pathname, 1);
}

function getPathFromURLPosix(url) {
Expand All @@ -1472,7 +1470,7 @@ function getPathFromURLPosix(url) {
const pathname = url.pathname;
for (let n = 0; n < pathname.length; n++) {
if (pathname[n] === '%') {
const third = pathname.codePointAt(n + 2) | 0x20;
const third = StringPrototypeCodePointAt(pathname, n + 2) | 0x20;
if (pathname[n + 1] === '2' && third === 102) {
throw new ERR_INVALID_FILE_URL_PATH(
'must not include encoded / characters'
Expand Down Expand Up @@ -1504,50 +1502,39 @@ function fileURLToPath(path) {
// - CR: The carriage return character is also stripped out by the `pathname`
// setter.
// - TAB: The tab character is also stripped out by the `pathname` setter.
const percentRegEx = /%/g;
const backslashRegEx = /\\/g;
const newlineRegEx = /\n/g;
const carriageReturnRegEx = /\r/g;
const tabRegEx = /\t/g;

function encodePathChars(filepath) {
if (StringPrototypeIncludes(filepath, '%'))
filepath = StringPrototypeReplace(filepath, percentRegEx, '%25');
filepath = StringPrototypeReplaceAll(filepath, '%', '%25');
// In posix, backslash is a valid character in paths:
if (!isWindows && StringPrototypeIncludes(filepath, '\\'))
filepath = StringPrototypeReplace(filepath, backslashRegEx, '%5C');
if (StringPrototypeIncludes(filepath, '\n'))
filepath = StringPrototypeReplace(filepath, newlineRegEx, '%0A');
if (StringPrototypeIncludes(filepath, '\r'))
filepath = StringPrototypeReplace(filepath, carriageReturnRegEx, '%0D');
if (StringPrototypeIncludes(filepath, '\t'))
filepath = StringPrototypeReplace(filepath, tabRegEx, '%09');
if (!isWindows) filepath = StringPrototypeReplaceAll(filepath, '\\', '%5C');
filepath = StringPrototypeReplaceAll(filepath, '\n', '%0A');
filepath = StringPrototypeReplaceAll(filepath, '\r', '%0D');
filepath = StringPrototypeReplaceAll(filepath, '\t', '%09');
Copy link
Member

Choose a reason for hiding this comment

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

According to micro-benchmarks, ReplaceAll is slower than Replace. I prefer using the existing implementation. Referencing: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/RESULTS-v19.md

Copy link
Contributor Author

@aduh95 aduh95 Dec 25, 2022

Choose a reason for hiding this comment

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

The existing implementation relies on user-mutable RegExp.prototype[Symbol.replace]. According to my own research, replaceAll on a string was faster than building a robust regex. (Actually in this case we're reusing the same regex, so it should not really matter, let's go with the regex approach)

return filepath;
}

function pathToFileURL(filepath) {
const outURL = new URL('file://');
if (isWindows && StringPrototypeStartsWith(filepath, '\\\\')) {
// UNC path format: \\server\share\resource
const paths = StringPrototypeSplit(filepath, '\\');
if (paths.length <= 3) {
const hostnameEndIndex = StringPrototypeIndexOf('\\', 2);
if (hostnameEndIndex === -1) {
throw new ERR_INVALID_ARG_VALUE(
'filepath',
filepath,
'Missing UNC resource path'
);
}
const hostname = paths[2];
if (hostname.length === 0) {
if (hostnameEndIndex === 3) {
throw new ERR_INVALID_ARG_VALUE(
'filepath',
filepath,
'Empty UNC servername'
);
}
const hostname = StringPrototypeSlice(filepath, 2, hostnameEndIndex);
outURL.hostname = domainToASCII(hostname);
outURL.pathname = encodePathChars(
ArrayPrototypeJoin(ArrayPrototypeSlice(paths, 3), '/'));
StringPrototypeReplaceAll(StringPrototypeSlice(filepath, hostnameEndIndex), '\\', '/'));
Copy link
Member

@anonrig anonrig Dec 24, 2022

Choose a reason for hiding this comment

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

This pull request changes more than just refactoring primordials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StringPrototypeSplit relies on user-mutable String.prototype[Symbol.split], so it's not exactly unrelated.

} else {
let resolved = path.resolve(filepath);
// path.resolve strips trailing slashes so we must add them back
Expand Down