[node-core-library] Fix semantics of RealNodeModulePath

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

@@ -611,7 +611,7 @@ export interface IRealNodeModulePathResolverOptions {
     // (undocumented)
     fs: Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>;
     // (undocumented)
-    path: Pick<typeof nodePath, 'isAbsolute' | 'normalize' | 'resolve' | 'sep'>;
+    path: Pick<typeof nodePath, 'format' | 'isAbsolute' | 'parse' | 'resolve' | 'sep'>;
 }
 
 // @public (undocumented)

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

@@ -10,7 +10,7 @@ import * as nodePath from 'path';
  */
 export interface IRealNodeModulePathResolverOptions {
   fs: Pick<typeof nodeFs, 'lstatSync' | 'readlinkSync'>;
-  path: Pick<typeof nodePath, 'isAbsolute' | 'normalize' | 'resolve' | 'sep'>;
+  path: Pick<typeof nodePath, 'format' | 'isAbsolute' | 'parse' | 'resolve' | 'sep'>;
 }
 
 /**
@@ -39,6 +39,7 @@ export class RealNodeModulePathResolver {
 
   private readonly _cache: Map<string, string>;
   private readonly _fs: IRealNodeModulePathResolverOptions['fs'];
+  private readonly _path: IRealNodeModulePathResolverOptions['path'];
 
   public constructor(
     options: IRealNodeModulePathResolverOptions = {
@@ -50,12 +51,13 @@ export class RealNodeModulePathResolver {
     const { path, fs } = options;
     const { sep: pathSeparator } = path;
     this._fs = fs;
+    this._path = path;
 
     const nodeModulesToken: string = `${pathSeparator}node_modules${pathSeparator}`;
-
-    const tryReadLink: (link: string) => string | undefined = this._tryReadLink.bind(this);
+    const self: this = this;
 
     function realNodeModulePathInternal(input: string): string {
+      input = self._normalizeTrailingSlash(input);
       // Find the last node_modules path segment
       const nodeModulesIndex: number = input.lastIndexOf(nodeModulesToken);
       if (nodeModulesIndex < 0) {
@@ -87,7 +89,7 @@ export class RealNodeModulePathResolver {
 
       const linkCandidate: string = input.slice(0, linkEnd);
       // Check if the link is a symlink
-      const linkTarget: string | undefined = tryReadLink(linkCandidate);
+      const linkTarget: string | undefined = self._tryReadLink(linkCandidate);
       if (linkTarget && path.isAbsolute(linkTarget)) {
         // Absolute path, combine the link target with any remaining path segments
         // Cache the resolution to avoid the readlink call in subsequent calls
@@ -118,7 +120,7 @@ export class RealNodeModulePathResolver {
     }
 
     this.realNodeModulePath = (input: string) => {
-      return realNodeModulePathInternal(path.normalize(input));
+      return realNodeModulePathInternal(path.format(path.parse(path.resolve(input))));
     };
   }
 
@@ -137,6 +139,8 @@ export class RealNodeModulePathResolver {
    * @returns The target of the symbolic link, or undefined if the input is not a symbolic link
    */
   private _tryReadLink(link: string): string | undefined {
+    link = this._normalizeTrailingSlash(link);
+
     const cached: string | undefined = this._cache.get(link);
     if (cached) {
       return cached;
@@ -146,7 +150,17 @@ 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');
+      const result: string = this._normalizeTrailingSlash(this._fs.readlinkSync(link, 'utf8'));
+      return result;
+    }
+  }
+
+  private _normalizeTrailingSlash(input: string): string {
+    const { _path: path } = this;
+    if (input.endsWith(path.sep)) {
+      // Logic to trim a path separator is complicated, so let the path APIs handle it.
+      return path.format(path.parse(input));
     }
+    return input;
   }
 }

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);