Skip to content

refactor(metro-file-map): Pre-resolve symlink targets and store normal posix paths #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kitten wants to merge 6 commits into
base: main
Choose a base branch
from
Open

refactor(metro-file-map): Pre-resolve symlink targets and store normal posix paths #1687

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a848cd5
Prenormalize symlink target to normal and remove weakmap cache
kitten Apr 11, 2026
d3fd493
Normalize stored value to posix
kitten Apr 11, 2026
e8a016c
Add/update unit tests
kitten Apr 11, 2026
e14f40a
Rebuild ts-defs
kitten Apr 11, 2026
0a64351
Drop intermediate data structure in resolveSymlinkTargetToNormalPath
kitten Apr 22, 2026
32f7bd4
Lazily compute ancestor of root idx
kitten Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/metro-file-map/src/crawlers/watchman/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,11 @@ export default async function watchmanCrawl({
if (fileData.type === 'l') {
symlinkInfo = fileData['symlink_target'] ?? 1;
}
if (typeof symlinkInfo === 'string') {
symlinkInfo = normalizePathSeparatorsToPosix(
pathUtils.resolveSymlinkToNormal(relativeFilePath, symlinkInfo),
);
}

const nextData: FileMetadata = [
mtime,
Expand Down
6 changes: 4 additions & 2 deletions packages/metro-file-map/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ export type {
// This should be bumped whenever a code change to `metro-file-map` itself
// would cause a change to the cache data structure and/or content (for a given
// filesystem state and build parameters).
const CACHE_BREAKER = '11';
const CACHE_BREAKER = '12';

const CHANGE_INTERVAL = 30;

Expand Down Expand Up @@ -565,7 +565,9 @@ export default class FileMap extends EventEmitter {
.readlink(this.#pathUtils.normalToAbsolute(normalPath))
.then(symlinkTarget => {
fileMetadata[H.VISITED] = 1;
fileMetadata[H.SYMLINK] = symlinkTarget;
fileMetadata[H.SYMLINK] = normalizePathSeparatorsToPosix(
this.#pathUtils.resolveSymlinkToNormal(normalPath, symlinkTarget),
);
});
}
return null;
Expand Down
24 changes: 24 additions & 0 deletions packages/metro-file-map/src/lib/RootPathUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* @format
*/

import normalizePathSeparatorsToSystem from './normalizePathSeparatorsToSystem';
import invariant from 'invariant';
import * as path from 'path';

Expand Down Expand Up @@ -166,6 +167,29 @@ export class RootPathUtils {
);
}

resolveSymlinkToNormal(
symlinkNormalPath: string,
readlinkResult: string,
): string {
let target = normalizePathSeparatorsToSystem(readlinkResult);
// WARN: This only applies to Windows + Node 20 case, where the value is completely
// unnormalized and a trailing slash may be returned
if (target[target.length - 1] === path.sep) {
target = target.slice(0, -1);
}
if (path.isAbsolute(target)) {
return this.absoluteToNormal(target);
}
// Resolve relative to the symlink's containing directory, expressed as
// a root-relative (possibly non-normal) path, then normalize
const sepIdx = symlinkNormalPath.lastIndexOf(path.sep);
const rootRelativeTarget =
sepIdx === -1
? target
: symlinkNormalPath.slice(0, sepIdx) + path.sep + target;
return this.relativeToNormal(rootRelativeTarget);
}

// If a path is a direct ancestor of the project root (or the root itself),
// return a number with the degrees of separation, e.g. root=0, parent=1,..
// or null otherwise.
Expand Down
31 changes: 7 additions & 24 deletions packages/metro-file-map/src/lib/TreeFS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import type {
} from '../flow-types';

import H from '../constants';
import normalizePathSeparatorsToSystem from './normalizePathSeparatorsToSystem';
import {RootPathUtils} from './RootPathUtils';
import invariant from 'invariant';
import path from 'path';
Expand Down Expand Up @@ -124,8 +125,6 @@ type MetadataIteratorOptions = Readonly<{
* a trailing slash
*/
export default class TreeFS implements MutableFileSystem {
+#cachedNormalSymlinkTargets: WeakMap<FileNode, NormalizedSymlinkTarget> =
new WeakMap();
+#pathUtils: RootPathUtils;
+#processFile: ProcessFileFunction;
+#rootDir: Path;
Expand Down Expand Up @@ -1214,33 +1213,17 @@ export default class TreeFS implements MutableFileSystem {
symlinkNode: FileMetadata,
canonicalPathOfSymlink: Path,
): NormalizedSymlinkTarget {
const cachedResult = this.#cachedNormalSymlinkTargets.get(symlinkNode);
if (cachedResult != null) {
return cachedResult;
}

@robhogan robhogan Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the face of it, this looks like it could be a perf regression, because we're introducing a new allocation and some string manipulation every time the same symlink is traversed, vs once per symlink.

I'd be interested to see a benchmark on a symlink-heavy project. Weakmap lookups should generally be very fast.

@kitten kitten Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was admittedly tied to the on-demand file system change, which necessitated changes here anyway. It then made sense to pre-normalise values which ended up being a net neutral change (performance-wise) across all combined changes (!)

I can give benchmarking this in isolation a go next week 👍

Together with the lazy symlink resolution I would expect this to be mainly a neutral change since the old code paths assumed a low amount of symlinks. But I don't have an intuition of what this would look like in a project with many symlinks where the cost isn't negligible. My intuition overall with this change was that the cost of WeakMap lookups can add up faster than computing the path once, provided symlink resolution is lazier than it is now

@kitten kitten Apr 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to show that it's mostly neutral: kitten@eee7a82

Intuitively, the same amount of work happens (at least for macOS), since the path is prenormalised and stored in the file tree data structure, which means that the work (which was negligible to begin with) is now happening earlier and makes the WeakMap obsolete.

I didn't test this on Windows, where presumably there's a small additional cost due to the path normalisation (win -> posix -> win), which keeps the file map data structure portable. We could simply skip this and keep this denormalised, but the parity is nice imo


const literalSymlinkTarget = symlinkNode[H.SYMLINK];
let symlinkTarget = symlinkNode[H.SYMLINK];
invariant(
typeof literalSymlinkTarget === 'string',
typeof symlinkTarget === 'string',
'Expected symlink target to be populated.',
);
const absoluteSymlinkTarget = path.resolve(
this.#rootDir,
canonicalPathOfSymlink,
'..', // Symlink target is relative to its containing directory.
literalSymlinkTarget, // May be absolute, in which case the above are ignored
);
const normalSymlinkTarget = path.relative(
this.#rootDir,
absoluteSymlinkTarget,
);
symlinkTarget = normalizePathSeparatorsToSystem(symlinkTarget);
const result = {
ancestorOfRootIdx:
this.#pathUtils.getAncestorOfRootIdx(normalSymlinkTarget),
normalPath: normalSymlinkTarget,
startOfBasenameIdx: normalSymlinkTarget.lastIndexOf(path.sep) + 1,
ancestorOfRootIdx: this.#pathUtils.getAncestorOfRootIdx(symlinkTarget),
normalPath: symlinkTarget,
startOfBasenameIdx: symlinkTarget.lastIndexOf(path.sep) + 1,

@robhogan robhogan Apr 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're not caching, we may be better off computing some of this lazily (and eg, just return the normalPath here).

@kitten kitten Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the intermediate data structure in 0a64351

};
this.#cachedNormalSymlinkTargets.set(symlinkNode, result);
return result;
}

Expand Down
41 changes: 41 additions & 0 deletions packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,45 @@ describe.each([['win32'], ['posix']])('RootPathUtils on %s', platform => {
])('getAncestorOfRootIdx (%s => %s)', (input, expected) => {
expect(pathUtils.getAncestorOfRootIdx(input)).toEqual(expected);
});

describe('resolveSymlinkToNormal', () => {
beforeEach(() => {
pathUtils = new RootPathUtils(p('/project/root'));
});

test.each([
['foo/link', './target.js', p('foo/target.js')],
['foo/link', '../bar.js', 'bar.js'],
['link', 'target.js', 'target.js'],
[p('a/b/link'), p('../../c.js'), 'c.js'],
[p('a/b/link'), p('../../../outside/f.js'), p('../outside/f.js')],
])(
'resolves relative target (%s -> %s) to %s',
(symlinkPath, readlinkResult, expected) => {
expect(
pathUtils.resolveSymlinkToNormal(p(symlinkPath), readlinkResult),
).toEqual(expected);
},
);

test.each([
['link', p('/project/root/target.js'), 'target.js'],
['link', p('/project/root/a/b.js'), p('a/b.js')],
['link', p('/outside/foo.js'), p('../../outside/foo.js')],
[p('a/link'), p('/project/root'), ''],
])(
'resolves absolute target (%s -> %s) to %s',
(symlinkPath, readlinkResult, expected) => {
expect(
pathUtils.resolveSymlinkToNormal(p(symlinkPath), readlinkResult),
).toEqual(expected);
},
);

test('strips trailing separator from target', () => {
expect(
pathUtils.resolveSymlinkToNormal('link', p('/project/root/dir/')),
).toEqual('dir');
});
});
});
66 changes: 28 additions & 38 deletions packages/metro-file-map/src/lib/__tests__/TreeFS-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,18 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
rootDir: p('/project'),
files: new Map<CanonicalPath, FileMetadata>([
[p('foo/another.js'), [123, 2, 0, null, 0, 'another']],
[p('foo/owndir'), [0, 0, 0, null, '.', null]],
[p('foo/link-to-bar.js'), [0, 0, 0, null, p('../bar.js'), null]],
[p('foo/link-to-another.js'), [0, 0, 0, null, p('another.js'), null]],
[p('foo/owndir'), [0, 0, 0, null, 'foo', null]],
[p('foo/link-to-bar.js'), [0, 0, 0, null, 'bar.js', null]],
[p('foo/link-to-another.js'), [0, 0, 0, null, 'foo/another.js', null]],
[p('../outside/external.js'), [0, 0, 0, null, 0, null]],
[p('bar.js'), [234, 3, 0, null, 0, 'bar']],
[p('link-to-foo'), [456, 0, 0, null, p('./../project/foo'), null]],
[p('abs-link-out'), [456, 0, 0, null, p('/outside/./baz/..'), null]],
[p('link-to-foo'), [456, 0, 0, null, 'foo', null]],
[p('abs-link-out'), [456, 0, 0, null, '../outside', null]],
[p('root'), [0, 0, 0, null, '..', null]],
[p('link-to-nowhere'), [123, 0, 0, null, p('./nowhere'), null]],
[p('link-to-self'), [123, 0, 0, null, p('./link-to-self'), null]],
[p('link-cycle-1'), [123, 0, 0, null, p('./link-cycle-2'), null]],
[p('link-cycle-2'), [123, 0, 0, null, p('./link-cycle-1'), null]],
[p('link-to-nowhere'), [123, 0, 0, null, 'nowhere', null]],
[p('link-to-self'), [123, 0, 0, null, 'link-to-self', null]],
[p('link-cycle-1'), [123, 0, 0, null, 'link-cycle-2', null]],
[p('link-cycle-2'), [123, 0, 0, null, 'link-cycle-1', null]],
[p('node_modules/pkg/a.js'), [123, 0, 0, null, 0, 'a']],
[p('node_modules/pkg/package.json'), [123, 0, 0, null, 0, 'pkg']],
]),
Expand Down Expand Up @@ -194,7 +194,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
rootDir: p('/deep/project/root'),
files: new Map<CanonicalPath, FileMetadata>([
[p('foo/index.js'), [123, 0, 0, null, 0, null]],
[p('link-up'), [123, 0, 0, null, p('..'), null]],
[p('link-up'), [123, 0, 0, null, '..', null]],
]),
processFile: () => {
throw new Error('Not implemented');
Expand All @@ -221,7 +221,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {

describe('symlinks to an ancestor of the project root', () => {
beforeEach(() => {
tfs.addOrModify(p('foo/link-up-2'), [0, 0, 0, null, p('../..'), null]);
tfs.addOrModify(p('foo/link-up-2'), [0, 0, 0, null, '..', null]);
});

test.each([
Expand Down Expand Up @@ -277,14 +277,14 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
test('returns changed (inc. new) and removed files in given FileData', () => {
const newFiles: FileData = new Map<CanonicalPath, FileMetadata>([
[p('new-file'), [789, 0, 0, null, 0, null]],
[p('link-to-foo'), [456, 0, 0, null, p('./foo'), null]],
[p('link-to-foo'), [456, 0, 0, null, 'foo', null]],
// Different modified time, expect new mtime in changedFiles
[p('foo/another.js'), [124, 0, 0, null, 0, null]],
[p('link-cycle-1'), [123, 0, 0, null, p('./link-cycle-2'), null]],
[p('link-cycle-2'), [123, 0, 0, null, p('./link-cycle-1'), null]],
[p('link-cycle-1'), [123, 0, 0, null, 'link-cycle-2', null]],
[p('link-cycle-2'), [123, 0, 0, null, 'link-cycle-1', null]],
// Was a symlink, now a regular file
[p('link-to-self'), [123, 0, 0, null, 0, null]],
[p('link-to-nowhere'), [123, 0, 0, null, p('./nowhere'), null]],
[p('link-to-nowhere'), [123, 0, 0, null, 'nowhere', null]],
[p('node_modules/pkg/a.js'), [123, 0, 0, null, 0, 'a']],
[p('node_modules/pkg/package.json'), [123, 0, 0, null, 0, 'pkg']],
]);
Expand Down Expand Up @@ -393,24 +393,18 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
[
[
p('a/1/package.json'),
[0, 0, 0, null, './real-package.json', null],
[0, 0, 0, null, 'a/1/real-package.json', null],
],
[
p('a/2/package.json'),
[0, 0, 0, null, './notexist-package.json', null],
],
[p('a/b/c/d/link-to-C'), [0, 0, 0, null, p('../../../..'), null]],
[
p('a/b/c/d/link-to-B'),
[0, 0, 0, null, p('../../../../..'), null],
],
[
p('a/b/c/d/link-to-A'),
[0, 0, 0, null, p('../../../../../..'), null],
[0, 0, 0, null, 'a/2/notexist-package.json', null],
],
[p('a/b/c/d/link-to-C'), [0, 0, 0, null, '', null]],
[p('a/b/c/d/link-to-B'), [0, 0, 0, null, '..', null]],
[p('a/b/c/d/link-to-A'), [0, 0, 0, null, '../..', null]],
[
p('n_m/workspace/link-to-pkg'),
[0, 0, 0, null, p('../../../workspace-pkg'), null],
[0, 0, 0, null, '../workspace-pkg', null],
],
] as Array<[CanonicalPath, FileMetadata]>
).concat(
Expand Down Expand Up @@ -817,7 +811,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
new Map<CanonicalPath, FileMetadata>([
[
p('newdir/link-to-link-to-bar.js'),
[0, 0, 0, null, p('../foo/link-to-bar.js'), null],
[0, 0, 0, null, 'foo/link-to-bar.js', null],
],
[p('foo/baz.js'), [0, 0, 0, null, 0, null]],
[p('bar.js'), [999, 1, 0, null, 0, null]],
Expand Down Expand Up @@ -967,7 +961,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
{
baseName: 'link-to-bar.js',
canonicalPath: p('foo/link-to-bar.js'),
metadata: [0, 0, 0, null, p('../bar.js'), null],
metadata: [0, 0, 0, null, 'bar.js', null],
},
]),
);
Expand All @@ -983,7 +977,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
files: new Map<CanonicalPath, FileMetadata>([
[p('foo.js'), [123, 0, 0, 'def456', 0, null]],
[p('bar.js'), [123, 0, 0, null, 0, null]],
[p('link-to-bar'), [456, 0, 0, null, p('./bar.js'), null]],
[p('link-to-bar'), [456, 0, 0, null, 'bar.js', null]],
]),
processFile: mockProcessFile,
});
Expand Down Expand Up @@ -1084,7 +1078,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
files: new Map<CanonicalPath, FileMetadata>([
[p('existing.js'), [123, 0, 0, '', 0]],
[p('dir/nested.js'), [456, 0, 0, '', 0]],
[p('mylink'), [0, 0, 0, '', p('./dir')]],
[p('mylink'), [0, 0, 0, '', 'dir']],
]),
processFile: () => {
throw new Error('Not implemented');
Expand Down Expand Up @@ -1214,23 +1208,19 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
test('tracks added files when adding a symlink', () => {
simpleTfs.addOrModify(
p('link-to-existing'),
[0, 0, 0, '', p('./existing.js')],
[0, 0, 0, '', 'existing.js'],
listener,
);

expect(logChange.mock.calls).toEqual([
[
'fileAdded',
p('link-to-existing'),
[0, 0, 0, '', p('./existing.js')],
],
['fileAdded', p('link-to-existing'), [0, 0, 0, '', 'existing.js']],
]);
});

test('tracks removed symlinks with their metadata', () => {
simpleTfs.remove(p('mylink'), listener);
expect(logChange.mock.calls).toEqual([
['fileRemoved', p('mylink'), [0, 0, 0, '', p('./dir')]],
['fileRemoved', p('mylink'), [0, 0, 0, '', 'dir']],
]);
});
});
Expand Down
6 changes: 5 additions & 1 deletion packages/metro-file-map/types/lib/RootPathUtils.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*
* @noformat
* @generated SignedSource<<5ecdb559fce5f5c6ed50df6e4eaebf20>>
* @generated SignedSource<<de7026bc6d3d1406108afc1b07d26f32>>
*
* This file was translated from Flow by scripts/generateTypeScriptDefinitions.js
* Original file: packages/metro-file-map/src/lib/RootPathUtils.js
Expand All @@ -21,6 +21,10 @@ export declare class RootPathUtils {
absoluteToNormal(absolutePath: string): string;
normalToAbsolute(normalPath: string): string;
relativeToNormal(relativePath: string): string;
resolveSymlinkToNormal(
symlinkNormalPath: string,
readlinkResult: string,
): string;
getAncestorOfRootIdx(normalPath: string): null | undefined | number;
joinNormalToRelative(
normalPath: string,
Expand Down
Loading