vfs: address PR review comments

- entries.js: use path.join instead of string concatenation
- router.js: use primordials (StringPrototypeLastIndexOf, StringPrototypeSplit,
  StringPrototypeReplaceAll), use path.basename, add comments explaining why
  some path module functions aren't used (VFS-specific semantics)
- fd.js: use numeric separator (10_000), use ??= operator, simplify isVirtualFd
- module_hooks.js: move path import to top level, use 'file:' prefix check
- sea.js: use kEmptyObject, lazy-load VirtualFileSystem with ??=
- mock.js: use path.join, lazy-load VirtualFileSystem with ??=

Author: mcollina

lib/internal/test_runner/mock/mock.js

@@ -50,6 +50,10 @@ const {
 const { MockTimers } = require('internal/test_runner/mock/mock_timers');
 const { Module } = require('internal/modules/cjs/loader');
 const { _load, _nodeModulePaths, _resolveFilename, isBuiltin } = Module;
+const { join } = require('path');
+
+// Lazy-load VirtualFileSystem to avoid loading VFS code if fs mocking is not used
+let VirtualFileSystem;
 function kDefaultFunction() {}
 const enableModuleMocking = getOptionValue('--experimental-test-module-mocks');
 const kSupportedFormats = [
@@ -454,8 +458,7 @@ class MockFSContext {
    * @returns {boolean}
    */
   existsSync(path) {
-    // Prepend prefix to path for VFS lookup
-    const fullPath = this.#prefix + (StringPrototypeStartsWith(path, '/') ? path : '/' + path);
+    const fullPath = join(this.#prefix, path);
     return this.#vfs.existsSync(fullPath);
   }
 
@@ -806,7 +809,7 @@ class MockTracker {
       validateObject(files, 'options.files');
     }
 
-    const { VirtualFileSystem } = require('internal/vfs/virtual_fs');
+    VirtualFileSystem ??= require('internal/vfs/virtual_fs').VirtualFileSystem;
     const vfs = new VirtualFileSystem({ __proto__: null, moduleHooks: true });
 
     // Add initial files if provided

lib/internal/vfs/entries.js

@@ -12,6 +12,7 @@ const {
     ERR_INVALID_STATE,
   },
 } = require('internal/errors');
+const { join } = require('path');
 const { createFileStats, createDirectoryStats } = require('internal/vfs/stats');
 
 // Symbols for private properties
@@ -198,7 +199,7 @@ class VirtualDirectory extends VirtualEntry {
    * This is synchronous - the populate callback must be synchronous.
    */
   ensurePopulated() {
-    if (!this[kPopulated] && this[kPopulate] !== null) {
+    if (!this[kPopulated]) {
       const scopedVfs = createScopedVFS(this, (name, entry) => {
         this[kEntries].set(name, entry);
       });
@@ -274,13 +275,13 @@ function createScopedVFS(directory, addEntry) {
   return {
     __proto__: null,
     addFile(name, content, options) {
-      const path = directory.path + '/' + name;
-      const file = new VirtualFile(path, content, options);
+      const filePath = join(directory.path, name);
+      const file = new VirtualFile(filePath, content, options);
       addEntry(name, file);
     },
     addDirectory(name, populate, options) {
-      const path = directory.path + '/' + name;
-      const dir = new VirtualDirectory(path, populate, options);
+      const dirPath = join(directory.path, name);
+      const dir = new VirtualDirectory(dirPath, populate, options);
       addEntry(name, dir);
     },
   };

lib/internal/vfs/fd.js

@@ -14,7 +14,7 @@ const kContent = Symbol('kContent');
 const kPath = Symbol('kPath');
 
 // FD range: 10000+ to avoid conflicts with real fds
-const VFS_FD_BASE = 10000;
+const VFS_FD_BASE = 10_000;
 let nextFd = VFS_FD_BASE;
 
 // Global registry of open virtual file descriptors
@@ -92,9 +92,7 @@ class VirtualFD {
    * @returns {Buffer}
    */
   getContentSync() {
-    if (this[kContent] === null) {
-      this[kContent] = this[kEntry].getContentSync();
-    }
+    this[kContent] ??= this[kEntry].getContentSync();
     return this[kContent];
   }
 
@@ -103,21 +101,11 @@ class VirtualFD {
    * @returns {Promise<Buffer>}
    */
   async getContent() {
-    if (this[kContent] === null) {
-      this[kContent] = await this[kEntry].getContent();
-    }
+    this[kContent] ??= await this[kEntry].getContent();
     return this[kContent];
   }
 }
 
-/**
- * Allocates a new virtual file descriptor.
- * @returns {number}
- */
-function allocateFd() {
-  return nextFd++;
-}
-
 /**
  * Opens a virtual file and returns its file descriptor.
  * @param {VirtualFile} entry The virtual file entry
@@ -126,7 +114,7 @@ function allocateFd() {
  * @returns {number} The file descriptor
  */
 function openVirtualFd(entry, flags, path) {
-  const fd = allocateFd();
+  const fd = nextFd++;
   const vfd = new VirtualFD(fd, entry, flags, path);
   openFDs.set(fd, vfd);
   return fd;
@@ -156,7 +144,7 @@ function closeVirtualFd(fd) {
  * @returns {boolean}
  */
 function isVirtualFd(fd) {
-  return fd >= VFS_FD_BASE && openFDs.has(fd);
+  return openFDs.has(fd);
 }
 
 /**

lib/internal/vfs/module_hooks.js

@@ -8,6 +8,7 @@ const {
   StringPrototypeStartsWith,
 } = primordials;
 
+const { dirname, resolve } = require('path');
 const { normalizePath } = require('internal/vfs/router');
 const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
 const { createENOENT } = require('internal/vfs/errors');
@@ -307,7 +308,7 @@ function getFormatFromExtension(url) {
  * @returns {string} Normalized file path
  */
 function urlToPath(urlOrPath) {
-  if (StringPrototypeStartsWith(urlOrPath, 'file://')) {
+  if (StringPrototypeStartsWith(urlOrPath, 'file:')) {
     return fileURLToPath(urlOrPath);
   }
   return urlOrPath;
@@ -328,7 +329,7 @@ function vfsResolveHook(specifier, context, nextResolve) {
 
   // Convert specifier to a path we can check
   let checkPath;
-  if (StringPrototypeStartsWith(specifier, 'file://')) {
+  if (StringPrototypeStartsWith(specifier, 'file:')) {
     checkPath = fileURLToPath(specifier);
   } else if (specifier[0] === '/') {
     // Absolute path
@@ -337,9 +338,8 @@ function vfsResolveHook(specifier, context, nextResolve) {
     // Relative path - need to resolve against parent
     if (context.parentURL) {
       const parentPath = urlToPath(context.parentURL);
-      const path = require('path');
-      const parentDir = path.dirname(parentPath);
-      checkPath = path.resolve(parentDir, specifier);
+      const parentDir = dirname(parentPath);
+      checkPath = resolve(parentDir, specifier);
     } else {
       return nextResolve(specifier, context);
     }
@@ -387,8 +387,8 @@ function vfsLoadHook(url, context, nextLoad) {
     return nextLoad(url, context);
   }
 
-  // Only handle file:// URLs
-  if (!StringPrototypeStartsWith(url, 'file://')) {
+  // Only handle file: URLs
+  if (!StringPrototypeStartsWith(url, 'file:')) {
     return nextLoad(url, context);
   }
 
@@ -429,6 +429,8 @@ function vfsLoadHook(url, context, nextLoad) {
 
 /**
  * Install hooks into Module._stat and various fs functions.
+ * Note: fs and internal modules are required here (not at top level) to avoid
+ * circular dependencies during bootstrap. This module may be loaded early.
  */
 function installHooks() {
   if (hooksInstalled) {

lib/internal/vfs/router.js

@@ -2,26 +2,30 @@
 
 const {
   StringPrototypeEndsWith,
+  StringPrototypeLastIndexOf,
+  StringPrototypeReplaceAll,
   StringPrototypeSlice,
+  StringPrototypeSplit,
   StringPrototypeStartsWith,
 } = primordials;
 
-const path = require('path');
+const { basename, resolve, sep } = require('path');
 
 /**
  * Normalizes a path for VFS lookup.
  * - Resolves to absolute path
  * - Removes trailing slashes (except for root)
- * - Normalizes separators to forward slashes
+ * - Normalizes separators to forward slashes (VFS uses '/' internally)
  * @param {string} inputPath The path to normalize
  * @returns {string} The normalized path
  */
 function normalizePath(inputPath) {
-  let normalized = path.resolve(inputPath);
+  let normalized = resolve(inputPath);
 
-  // On Windows, convert backslashes to forward slashes for consistent lookup
-  if (path.sep === '\\') {
-    normalized = normalized.replace(/\\/g, '/');
+  // On Windows, convert backslashes to forward slashes for consistent VFS lookup.
+  // VFS uses forward slashes internally regardless of platform.
+  if (sep === '\\') {
+    normalized = StringPrototypeReplaceAll(normalized, '\\', '/');
   }
 
   // Remove trailing slash (except for root)
@@ -34,27 +38,29 @@ function normalizePath(inputPath) {
 
 /**
  * Splits a path into segments.
+ * VFS paths are always normalized to use forward slashes.
  * @param {string} normalizedPath A normalized absolute path
  * @returns {string[]} Path segments
  */
 function splitPath(normalizedPath) {
   if (normalizedPath === '/') {
     return [];
   }
-  // Remove leading slash and split
-  return StringPrototypeSlice(normalizedPath, 1).split('/');
+  // Remove leading slash and split by forward slash (VFS internal format)
+  return StringPrototypeSplit(StringPrototypeSlice(normalizedPath, 1), '/');
 }
 
 /**
  * Gets the parent path of a normalized path.
+ * VFS paths are always normalized to use forward slashes.
  * @param {string} normalizedPath A normalized absolute path
  * @returns {string|null} The parent path, or null if at root
  */
 function getParentPath(normalizedPath) {
   if (normalizedPath === '/') {
     return null;
   }
-  const lastSlash = normalizedPath.lastIndexOf('/');
+  const lastSlash = StringPrototypeLastIndexOf(normalizedPath, '/');
   if (lastSlash === 0) {
     return '/';
   }
@@ -67,12 +73,14 @@ function getParentPath(normalizedPath) {
  * @returns {string} The base name
  */
 function getBaseName(normalizedPath) {
-  const lastSlash = normalizedPath.lastIndexOf('/');
-  return StringPrototypeSlice(normalizedPath, lastSlash + 1);
+  // Basename works correctly for VFS paths since they use forward slashes
+  return basename(normalizedPath);
 }
 
 /**
  * Checks if a path is under a mount point.
+ * Note: We don't use path.relative here because VFS mount point semantics
+ * require exact prefix matching with the forward-slash separator.
  * @param {string} normalizedPath A normalized absolute path
  * @param {string} mountPoint A normalized mount point path
  * @returns {boolean}
@@ -87,6 +95,8 @@ function isUnderMountPoint(normalizedPath, mountPoint) {
 
 /**
  * Gets the relative path from a mount point.
+ * Note: We don't use path.relative here because we need the VFS-internal
+ * path format (starting with /) for consistent lookup.
  * @param {string} normalizedPath A normalized absolute path
  * @param {string} mountPoint A normalized mount point path
  * @returns {string} The relative path (starting with /)
@@ -100,6 +110,8 @@ function getRelativePath(normalizedPath, mountPoint) {
 
 /**
  * Joins a mount point with a relative path.
+ * Note: We don't use path.join here because VFS relative paths start with /
+ * and path.join would treat them as absolute paths.
  * @param {string} mountPoint A normalized mount point path
  * @param {string} relativePath A relative path (starting with /)
  * @returns {string} The joined absolute path

lib/internal/vfs/sea.js

@@ -6,10 +6,14 @@ const {
 
 const { Buffer } = require('buffer');
 const { isSea, getAsset, getAssetKeys } = require('sea');
+const { kEmptyObject } = require('internal/util');
 
 // Lazy-loaded VFS
 let cachedSeaVfs = null;
 
+// Lazy-load VirtualFileSystem to avoid loading VFS code if not needed
+let VirtualFileSystem;
+
 /**
  * Creates a VirtualFileSystem populated with SEA assets.
  * Assets are mounted at the specified prefix (default: '/sea').
@@ -18,12 +22,12 @@ let cachedSeaVfs = null;
  * @param {boolean} [options.moduleHooks] Whether to enable require/import hooks
  * @returns {VirtualFileSystem|null} The VFS instance, or null if not running as SEA
  */
-function createSeaVfs(options = {}) {
+function createSeaVfs(options = kEmptyObject) {
   if (!isSea()) {
     return null;
   }
 
-  const { VirtualFileSystem } = require('internal/vfs/virtual_fs');
+  VirtualFileSystem ??= require('internal/vfs/virtual_fs').VirtualFileSystem;
   const prefix = options.prefix ?? '/sea';
   const moduleHooks = options.moduleHooks !== false;