[node-core-library] Fix semantics of RealNodeModulePath (#5042)

* [node-core-library] Fix semantics of RealNodeModulePath

* Add tests for relative paths

* Use `path.join`

* Optionalize, use path.join instead of parse/format

* Fix parameter optionality

---------

Co-authored-by: David Michon <[email protected]>

Author: dmichon-msft

common/changes/@rushstack/node-core-library/trim-trailing-slash_2024-12-13-02-11.json

@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@rushstack/node-core-library",
+      "comment": "Fix handling of trailing slashes and relative paths in RealNodeModulePath to match semantics of `fs.realpathSync.native`.",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@rushstack/node-core-library"
+}

common/reviews/api/node-core-library.api.md

@@ -609,9 +609,9 @@ export interface IReadLinesFromIterableOptions {
 // @public
 export interface IRealNodeModulePathResolverOptions {
     // (undocumented)
-    fs: Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>;
+    fs?: Partial<Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>>;
     // (undocumented)
-    path: Pick<typeof nodePath, 'isAbsolute' | 'normalize' | 'resolve' | 'sep'>;
+    path?: Partial<Pick<typeof nodePath, 'isAbsolute' | 'join' | 'resolve' | 'sep'>>;
 }
 
 // @public (undocumented)

libraries/node-core-library/src/RealNodeModulePath.ts

@@ -9,8 +9,8 @@ import * as nodePath from 'path';
  * @public
  */
 export interface IRealNodeModulePathResolverOptions {
-  fs: Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>;
-  path: Pick<typeof nodePath, 'isAbsolute' | 'normalize' | 'resolve' | 'sep'>;
+  fs?: Partial<Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>>;
+  path?: Partial<Pick<typeof nodePath, 'isAbsolute' | 'join' | 'resolve' | 'sep'>>;
 }
 
 /**
@@ -38,22 +38,33 @@ export class RealNodeModulePathResolver {
   public readonly realNodeModulePath: (input: string) => string;
 
   private readonly _cache: Map<string, string>;
-  private readonly _fs: IRealNodeModulePathResolverOptions['fs'];
-
-  public constructor(
-    options: IRealNodeModulePathResolverOptions = {
-      fs: nodeFs,
-      path: nodePath
-    }
-  ) {
+  private readonly _fs: Required<NonNullable<IRealNodeModulePathResolverOptions['fs']>>;
+  private readonly _path: Required<NonNullable<IRealNodeModulePathResolverOptions['path']>>;
+
+  public constructor(options: IRealNodeModulePathResolverOptions = {}) {
+    const {
+      fs: { lstatSync = nodeFs.lstatSync, readlinkSync = nodeFs.readlinkSync } = nodeFs,
+      path: {
+        isAbsolute = nodePath.isAbsolute,
+        join = nodePath.join,
+        resolve = nodePath.resolve,
+        sep = nodePath.sep
+      } = nodePath
+    } = options;
     const cache: Map<string, string> = (this._cache = new Map());
-    const { path, fs } = options;
-    const { sep: pathSeparator } = path;
-    this._fs = fs;
-
-    const nodeModulesToken: string = `${pathSeparator}node_modules${pathSeparator}`;
+    this._fs = {
+      lstatSync,
+      readlinkSync
+    };
+    this._path = {
+      isAbsolute,
+      join,
+      resolve,
+      sep
+    };
 
-    const tryReadLink: (link: string) => string | undefined = this._tryReadLink.bind(this);
+    const nodeModulesToken: string = `${sep}node_modules${sep}`;
+    const self: this = this;
 
     function realNodeModulePathInternal(input: string): string {
       // Find the last node_modules path segment
@@ -65,19 +76,24 @@ export class RealNodeModulePathResolver {
 
       // First assume that the next path segment after node_modules is a symlink
       let linkStart: number = nodeModulesIndex + nodeModulesToken.length - 1;
-      let linkEnd: number = input.indexOf(pathSeparator, linkStart + 1);
+      let linkEnd: number = input.indexOf(sep, linkStart + 1);
       // If the path segment starts with a '@', then it is a scoped package
       const isScoped: boolean = input.charAt(linkStart + 1) === '@';
       if (isScoped) {
         // For a scoped package, the scope is an ordinary directory, so we need to find the next path segment
         if (linkEnd < 0) {
           // Symlink missing, so see if anything before the last node_modules needs resolving,
           // and preserve the rest of the path
-          return `${realNodeModulePathInternal(input.slice(0, nodeModulesIndex))}${input.slice(nodeModulesIndex)}`;
+          return join(
+            realNodeModulePathInternal(input.slice(0, nodeModulesIndex)),
+            input.slice(nodeModulesIndex + 1),
+            // Joining to `.` will clean up any extraneous trailing slashes
+            '.'
+          );
         }
 
         linkStart = linkEnd;
-        linkEnd = input.indexOf(pathSeparator, linkStart + 1);
+        linkEnd = input.indexOf(sep, linkStart + 1);
       }
 
       // No trailing separator, so the link is the last path segment
@@ -87,13 +103,14 @@ export class RealNodeModulePathResolver {
 
       const linkCandidate: string = input.slice(0, linkEnd);
       // Check if the link is a symlink
-      const linkTarget: string | undefined = tryReadLink(linkCandidate);
-      if (linkTarget && path.isAbsolute(linkTarget)) {
+      const linkTarget: string | undefined = self._tryReadLink(linkCandidate);
+      if (linkTarget && isAbsolute(linkTarget)) {
         // Absolute path, combine the link target with any remaining path segments
         // Cache the resolution to avoid the readlink call in subsequent calls
         cache.set(linkCandidate, linkTarget);
         cache.set(linkTarget, linkTarget);
-        return `${linkTarget}${input.slice(linkEnd)}`;
+        // Joining to `.` will clean up any extraneous trailing slashes
+        return join(linkTarget, input.slice(linkEnd + 1), '.');
       }
 
       // Relative path or does not exist
@@ -102,23 +119,26 @@ export class RealNodeModulePathResolver {
       const realpathBeforeNodeModules: string = realNodeModulePathInternal(input.slice(0, nodeModulesIndex));
       if (linkTarget) {
         // Relative path in symbolic link. Should be resolved relative to real path of base path.
-        const resolvedTarget: string = path.resolve(
-          `${realpathBeforeNodeModules}${input.slice(nodeModulesIndex, linkStart)}`,
+        const resolvedTarget: string = resolve(
+          realpathBeforeNodeModules,
+          input.slice(nodeModulesIndex + 1, linkStart),
           linkTarget
         );
         // Cache the result of the combined resolution to avoid the readlink call in subsequent calls
         cache.set(linkCandidate, resolvedTarget);
         cache.set(resolvedTarget, resolvedTarget);
-        return `${resolvedTarget}${input.slice(linkEnd)}`;
+        // Joining to `.` will clean up any extraneous trailing slashes
+        return join(resolvedTarget, input.slice(linkEnd + 1), '.');
       }
 
       // No symlink, so just return the real path before the last node_modules combined with the
       // subsequent path segments
-      return `${realpathBeforeNodeModules}${input.slice(nodeModulesIndex)}`;
+      // Joining to `.` will clean up any extraneous trailing slashes
+      return join(realpathBeforeNodeModules, input.slice(nodeModulesIndex + 1), '.');
     }
 
     this.realNodeModulePath = (input: string) => {
-      return realNodeModulePathInternal(path.normalize(input));
+      return realNodeModulePathInternal(resolve(input));
     };
   }
 
@@ -146,7 +166,9 @@ export class RealNodeModulePathResolver {
     // of an lstat call.
     const stat: nodeFs.Stats | undefined = this._fs.lstatSync(link);
     if (stat.isSymbolicLink()) {
-      return this._fs.readlinkSync(link, 'utf8');
+      // path.join(x, '.') will trim trailing slashes, if applicable
+      const result: string = this._path.join(this._fs.readlinkSync(link, 'utf8'), '.');
+      return result;
     }
   }
 }

libraries/node-core-library/src/test/RealNodeModulePath.test.ts

@@ -35,8 +35,8 @@ describe('realNodeModulePath', () => {
       resolver.clearCache();
     });
 
-    it('should return the input path if it does not contain node_modules', () => {
-      for (const input of ['/foo/bar', '/', 'ab', '../foo/bar/baz']) {
+    it('should return the input path if it is absolute and does not contain node_modules', () => {
+      for (const input of ['/foo/bar', '/']) {
         expect(realNodeModulePath(input)).toBe(input);
 
         expect(mocklstatSync).not.toHaveBeenCalled();
@@ -54,6 +54,16 @@ describe('realNodeModulePath', () => {
       expect(mockReadlinkSync).toHaveBeenCalledTimes(0);
     });
 
+    it('should trim a trailing slash from the input path if it is not a symbolic link', () => {
+      mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => false } as unknown as fs.Stats);
+
+      expect(realNodeModulePath('/foo/node_modules/foo/')).toBe('/foo/node_modules/foo');
+
+      expect(mocklstatSync).toHaveBeenCalledWith('/foo/node_modules/foo');
+      expect(mocklstatSync).toHaveBeenCalledTimes(1);
+      expect(mockReadlinkSync).toHaveBeenCalledTimes(0);
+    });
+
     it('Should handle absolute link targets', () => {
       mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
       mockReadlinkSync.mockReturnValueOnce('/link/target');
@@ -66,12 +76,25 @@ describe('realNodeModulePath', () => {
       expect(mockReadlinkSync).toHaveBeenCalledTimes(1);
     });
 
+    it('Should trim trailing slash from absolute link targets', () => {
+      mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
+      mockReadlinkSync.mockReturnValueOnce('/link/target/');
+
+      expect(realNodeModulePath('/foo/node_modules/link/bar')).toBe('/link/target/bar');
+
+      expect(mocklstatSync).toHaveBeenCalledWith('/foo/node_modules/link');
+      expect(mocklstatSync).toHaveBeenCalledTimes(1);
+      expect(mockReadlinkSync).toHaveBeenCalledWith('/foo/node_modules/link', 'utf8');
+      expect(mockReadlinkSync).toHaveBeenCalledTimes(1);
+    });
+
     it('Caches resolved symlinks', () => {
       mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
       mockReadlinkSync.mockReturnValueOnce('/link/target');
 
       expect(realNodeModulePath('/foo/node_modules/link')).toBe('/link/target');
       expect(realNodeModulePath('/foo/node_modules/link/bar')).toBe('/link/target/bar');
+      expect(realNodeModulePath('/foo/node_modules/link/')).toBe('/link/target');
 
       expect(mocklstatSync).toHaveBeenCalledWith('/foo/node_modules/link');
       expect(mocklstatSync).toHaveBeenCalledTimes(1);
@@ -155,22 +178,27 @@ describe('realNodeModulePath', () => {
       resolver.clearCache();
     });
 
-    it('should return the input path if it does not contain node_modules', () => {
-      for (const input of ['C:\\foo\\bar', 'C:\\', 'ab', '..\\foo\\bar\\baz']) {
-        mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
-
+    it('should return the input path if it is absolute and does not contain node_modules', () => {
+      for (const input of ['C:\\foo\\bar', 'C:\\']) {
         expect(realNodeModulePath(input)).toBe(input);
 
         expect(mocklstatSync).not.toHaveBeenCalled();
         expect(mockReadlinkSync).not.toHaveBeenCalled();
       }
     });
 
-    it('should return the normalized input path if it does not contain node_modules', () => {
-      for (const input of ['C:/foo/bar', 'C:/', 'ab', '../foo/bar/baz']) {
+    it('should trim extra trailing separators from the root', () => {
+      expect(realNodeModulePath('C:////')).toBe('C:\\');
+
+      expect(mocklstatSync).not.toHaveBeenCalled();
+      expect(mockReadlinkSync).not.toHaveBeenCalled();
+    });
+
+    it('should return the resolved input path if it is absolute and does not contain node_modules', () => {
+      for (const input of ['C:/foo/bar', 'C:/', 'ab', '../b/c/d']) {
         mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
 
-        expect(realNodeModulePath(input)).toBe(path.win32.normalize(input));
+        expect(realNodeModulePath(input)).toBe(path.win32.resolve(input));
 
         expect(mocklstatSync).not.toHaveBeenCalled();
         expect(mockReadlinkSync).not.toHaveBeenCalled();
@@ -187,11 +215,33 @@ describe('realNodeModulePath', () => {
       expect(mockReadlinkSync).toHaveBeenCalledTimes(0);
     });
 
+    it('Should trim a trailing path separator if the target is not a symbolic link', () => {
+      mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => false } as unknown as fs.Stats);
+
+      expect(realNodeModulePath('C:\\foo\\node_modules\\foo\\')).toBe('C:\\foo\\node_modules\\foo');
+
+      expect(mocklstatSync).toHaveBeenCalledWith('C:\\foo\\node_modules\\foo');
+      expect(mocklstatSync).toHaveBeenCalledTimes(1);
+      expect(mockReadlinkSync).toHaveBeenCalledTimes(0);
+    });
+
     it('Should handle absolute link targets', () => {
       mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
       mockReadlinkSync.mockReturnValueOnce('C:\\link\\target');
 
-      expect(realNodeModulePath('C:\\foo\\node_modules\\link')).toBe('C:\\link\\target');
+      expect(realNodeModulePath('C:\\foo\\node_modules\\link\\relative')).toBe('C:\\link\\target\\relative');
+
+      expect(mocklstatSync).toHaveBeenCalledWith('C:\\foo\\node_modules\\link');
+      expect(mocklstatSync).toHaveBeenCalledTimes(1);
+      expect(mockReadlinkSync).toHaveBeenCalledWith('C:\\foo\\node_modules\\link', 'utf8');
+      expect(mockReadlinkSync).toHaveBeenCalledTimes(1);
+    });
+
+    it('Should trim a trailing path separator from an absolute link target', () => {
+      mocklstatSync.mockReturnValueOnce({ isSymbolicLink: () => true } as unknown as fs.Stats);
+      mockReadlinkSync.mockReturnValueOnce('C:\\link\\target\\');
+
+      expect(realNodeModulePath('C:\\foo\\node_modules\\link\\relative')).toBe('C:\\link\\target\\relative');
 
       expect(mocklstatSync).toHaveBeenCalledWith('C:\\foo\\node_modules\\link');
       expect(mocklstatSync).toHaveBeenCalledTimes(1);