diff --git a/common/changes/@rushstack/node-core-library/trim-trailing-slash_2024-12-13-02-11.json b/common/changes/@rushstack/node-core-library/trim-trailing-slash_2024-12-13-02-11.json new file mode 100644 index 0000000000..bd993cd3d8 --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/common/reviews/api/node-core-library.api.md b/common/reviews/api/node-core-library.api.md index 08cbafde6d..3dc3e2214e 100644 --- a/common/reviews/api/node-core-library.api.md +++ b/common/reviews/api/node-core-library.api.md @@ -609,9 +609,9 @@ export interface IReadLinesFromIterableOptions { // @public export interface IRealNodeModulePathResolverOptions { // (undocumented) - fs: Pick; + fs?: Partial>; // (undocumented) - path: Pick; + path?: Partial>; } // @public (undocumented) diff --git a/libraries/node-core-library/src/RealNodeModulePath.ts b/libraries/node-core-library/src/RealNodeModulePath.ts index 5286430fde..85e85d5aeb 100644 --- a/libraries/node-core-library/src/RealNodeModulePath.ts +++ b/libraries/node-core-library/src/RealNodeModulePath.ts @@ -9,8 +9,8 @@ import * as nodePath from 'path'; * @public */ export interface IRealNodeModulePathResolverOptions { - fs: Pick; - path: Pick; + fs?: Partial>; + path?: Partial>; } /** @@ -38,22 +38,33 @@ export class RealNodeModulePathResolver { public readonly realNodeModulePath: (input: string) => string; private readonly _cache: Map; - private readonly _fs: IRealNodeModulePathResolverOptions['fs']; - - public constructor( - options: IRealNodeModulePathResolverOptions = { - fs: nodeFs, - path: nodePath - } - ) { + private readonly _fs: Required>; + private readonly _path: Required>; + + 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 = (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,7 +76,7 @@ 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) { @@ -73,11 +84,16 @@ export class RealNodeModulePathResolver { 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; } } } diff --git a/libraries/node-core-library/src/test/RealNodeModulePath.test.ts b/libraries/node-core-library/src/test/RealNodeModulePath.test.ts index ff90600167..eaafd593db 100644 --- a/libraries/node-core-library/src/test/RealNodeModulePath.test.ts +++ b/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,10 +178,8 @@ 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(); @@ -166,11 +187,18 @@ describe('realNodeModulePath', () => { } }); - 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);