diff --git a/packages/metro-file-map/src/lib/RootPathUtils.js b/packages/metro-file-map/src/lib/RootPathUtils.js index b844d847ec..c9a04bdabf 100644 --- a/packages/metro-file-map/src/lib/RootPathUtils.js +++ b/packages/metro-file-map/src/lib/RootPathUtils.js @@ -27,6 +27,12 @@ import * as path from 'path'; * Output and input paths are at least well-formed (normal where indicated by * naming). * + * Trailing path separators are preserved, except for fs roots in + * normalToAbsolute (fs roots always have a trailing separator), and the + * project root in absoluteToNormal and relativeToNormal (the project root is + * always the empty string, and is always a directory, so a trailing separator + * is redundant). + * * As of Node 20, absoluteToNormal is ~8x faster than `path.relative` and * `normalToAbsolute` is ~20x faster than `path.resolve`, benchmarked on the * real inputs from building FB's product graph. Some well-formed inputs @@ -108,10 +114,18 @@ export class RootPathUtils { absolutePath, endOfMatchingPrefix, upIndirectionsToPrepend, - ) ?? path.relative(this.#rootDir, absolutePath) + ) ?? this.#slowAbsoluteToNormal(absolutePath) ); } + #slowAbsoluteToNormal(absolutePath: string): string { + const endsWithSep = absolutePath.endsWith(path.sep); + const result = path.relative(this.#rootDir, absolutePath); + return endsWithSep && !result.endsWith(path.sep) + ? result + path.sep + : result; + } + // `normalPath` is assumed to be normal (root-relative, no redundant // indirection), per the definition above. normalToAbsolute(normalPath: string): string { @@ -199,10 +213,21 @@ export class RootPathUtils { break; } } - const right = fullPath.slice(pos); + // After collapsing we may have no more segments remaining (following + // '..' indirections). Ensure that we don't drop or add a trailing + // separator in this case by taking .slice(pos-1). In any other case, + // we know that fullPath[pos] is a separator. + if (pos >= fullPath.length) { + return totalUpIndirections > 0 + ? UP_FRAGMENT_SEP.repeat(totalUpIndirections - 1) + + '..' + + fullPath.slice(pos - 1) + : ''; + } + const right = pos > 0 ? fullPath.slice(pos) : fullPath; if ( - right === '' || - (right === '..' && totalUpIndirections >= this.#rootParts.length - 1) + right === '..' && + totalUpIndirections >= this.#rootParts.length - 1 ) { // If we have no right side (or an indirection that would take us // below the root), just ensure we don't include a trailing separtor. diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index e1cad740ae..5b3bff1645 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -31,8 +31,8 @@ type MixedNode = FileNode | DirectoryNode; // // mixedPath - a root-relative or absolute path // relativePath - a root-relative path -// normalPath - a root-relative, normalised path (no extraneous '.' or '..') -// canonicalPath - a root-relative, normalised, real path (no symlinks in dirname) +// normalPath - a root-relative, normalised path (no extraneous '.' or '..'), may have a trailing slash +// canonicalPath - a root-relative, normalised, real path (no symlinks in dirname), no trailing slash export default class TreeFS implements MutableFileSystem { +#cachedNormalSymlinkTargets: WeakMap = new WeakMap(); @@ -441,17 +441,23 @@ export default class TreeFS implements MutableFileSystem { } } - // If there are no more '/' to come, we're done unless this is a symlink - // we must follow. + // We are done if... if ( - isLastSegment && - (segmentNode instanceof Map || - segmentNode[H.SYMLINK] === 0 || - opts.followLeaf === false) + // ...at a directory node and the only subsequent character is `/`, or + (nextSepIdx === targetNormalPath.length - 1 && + segmentNode instanceof Map) || + // there are no subsequent `/`, and this node is anything but a symlink + // we're required to resolve due to followLeaf. + (isLastSegment && + (segmentNode instanceof Map || + segmentNode[H.SYMLINK] === 0 || + opts.followLeaf === false)) ) { return { ancestorOfRootIdx, - canonicalPath: targetNormalPath, + canonicalPath: isLastSegment + ? targetNormalPath + : targetNormalPath.slice(0, -1), // remove trailing `/` exists: true, node: segmentNode, parentNode, diff --git a/packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js b/packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js index ffd31b5180..e13b1bd19f 100644 --- a/packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js +++ b/packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js @@ -14,7 +14,7 @@ import type {RootPathUtils as RootPathUtilsT} from '../RootPathUtils'; let mockPathModule; jest.mock('path', () => mockPathModule); -describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => { +describe.each([['win32'], ['posix']])('RootPathUtils on %s', platform => { // Convenience function to write paths with posix separators but convert them // to system separators const p: string => string = filePath => @@ -25,10 +25,12 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => { let RootPathUtils: Class; let pathUtils: RootPathUtilsT; let pathRelative: JestMockFn<[string, string], string>; + let sep: string; beforeEach(() => { jest.resetModules(); mockPathModule = jest.requireActual<{}>('path')[platform]; + sep = mockPathModule.sep; pathRelative = jest.spyOn(mockPathModule, 'relative'); RootPathUtils = require('../RootPathUtils').RootPathUtils; }); @@ -38,21 +40,30 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => { p('/project/root/../root2/foobar'), p('/project/root/../../project2/foo'), p('/project/root/../../project/foo'), + p('/project/root/../../project/foo/'), p('/project/root/../../project/root'), + p('/project/root/../../project/root/'), p('/project/root/../../project/root/foo.js'), p('/project/bar'), + p('/project/bar/'), p('/project/../outside/bar'), p('/project/baz/foobar'), p('/project/rootfoo/baz'), p('/project'), + p('/project/'), p('/'), p('/outside'), - ])(`absoluteToNormal('%s') is correct and optimised`, normalPath => { + p('/outside/'), + ])(`absoluteToNormal('%s') is correct and optimised`, absolutePath => { const rootDir = p('/project/root'); pathUtils = new RootPathUtils(rootDir); - const expected = mockPathModule.relative(rootDir, normalPath); + let expected = mockPathModule.relative(rootDir, absolutePath); + // Unlike path.relative, we expect to preserve trailing separators. + if (absolutePath.endsWith(sep) && expected !== '') { + expected += sep; + } pathRelative.mockClear(); - expect(pathUtils.absoluteToNormal(normalPath)).toEqual(expected); + expect(pathUtils.absoluteToNormal(absolutePath)).toEqual(expected); expect(pathRelative).not.toHaveBeenCalled(); }); @@ -68,24 +79,35 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => { p('/project/root/a./../foo'), p('/project/root/../a./foo'), p('/project/root/.././foo'), - ])(`absoluteToNormal('%s') falls back to path.relative`, normalPath => { - const expected = mockPathModule.relative(rootDir, normalPath); + p('/project/root/.././foo/'), + ])(`absoluteToNormal('%s') falls back to path.relative`, absolutePath => { + let expected = mockPathModule.relative(rootDir, absolutePath); + // Unlike path.relative, we expect to preserve trailing separators. + if (absolutePath.endsWith(sep) && !expected.endsWith(sep)) { + expected += sep; + } pathRelative.mockClear(); - expect(pathUtils.absoluteToNormal(normalPath)).toEqual(expected); + expect(pathUtils.absoluteToNormal(absolutePath)).toEqual(expected); expect(pathRelative).toHaveBeenCalled(); }); test.each([ p('..'), p('../..'), + p('../../'), p('normal/path'), + p('normal/path/'), p('../normal/path'), + p('../normal/path/'), p('../../normal/path'), p('../../../normal/path'), ])(`normalToAbsolute('%s') matches path.resolve`, normalPath => { - expect(pathUtils.normalToAbsolute(normalPath)).toEqual( - mockPathModule.resolve(rootDir, normalPath), - ); + let expected = mockPathModule.resolve(rootDir, normalPath); + // Unlike path.resolve, we expect to preserve trailing separators. + if (normalPath.endsWith(sep) && !expected.endsWith(sep)) { + expected += sep; + } + expect(pathUtils.normalToAbsolute(normalPath)).toEqual(expected); }); test.each([ @@ -93,18 +115,29 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => { p('../root'), p('../root/path'), p('../project'), + p('../project/'), p('../../project/root'), + p('../../project/root/'), p('../../../normal/path'), + p('../../../normal/path/'), p('../../..'), ])( `relativeToNormal('%s') matches path.resolve + path.relative`, relativePath => { - expect(pathUtils.relativeToNormal(relativePath)).toEqual( - mockPathModule.relative( - rootDir, - mockPathModule.resolve(rootDir, relativePath), - ), + let expected = mockPathModule.relative( + rootDir, + mockPathModule.resolve(rootDir, relativePath), ); + // Unlike native path.resolve + path.relative, we expect to preserve + // trailing separators. (Consistent with path.normalize.) + if ( + relativePath.endsWith(sep) && + !expected.endsWith(sep) && + expected !== '' + ) { + expected += sep; + } + expect(pathUtils.relativeToNormal(relativePath)).toEqual(expected); }, ); }); diff --git a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js index 18733ac5ae..c4077fce54 100644 --- a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js +++ b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js @@ -132,6 +132,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { test.each([ [p('/project/bar.js/bad-parent'), [], p('/project/bar.js')], + [p('/project/bar.js/'), [], p('/project/bar.js')], [ p('/project/link-to-nowhere'), [p('/project/link-to-nowhere')], @@ -152,13 +153,16 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { }), ); - test.each([[p('/project/foo')], [p('/project/root/outside')]])( - 'returns type: d for %s', - givenPath => - expect(tfs.lookup(givenPath)).toMatchObject({ - exists: true, - type: 'd', - }), + test.each([ + [p('/project/foo'), p('/project/foo')], + [p('/project/foo/'), p('/project/foo')], + [p('/project/root/outside'), p('/outside')], + ])('returns type: d for %s', (givenPath, expectedRealPath) => + expect(tfs.lookup(givenPath)).toMatchObject({ + exists: true, + type: 'd', + realPath: expectedRealPath, + }), ); test('traversing the same symlink multiple times does not imply a cycle', () => {