Skip to content

Commit

Permalink
Fix symlinked packages not respecting package.json redirections (#1349)
Browse files Browse the repository at this point in the history
Summary:
`TreeFS.hierarchicalLookup` is a recently introduced function for finding closest package.json for an input path.

It works by traversing to the input path while "collecting ancestors" - i.e. all of the directory nodes on route to the input path, and then checking each of them in reverse order for package.json.

This gets a bit complicated when traversal includes symlinks. For the test case in this commit:

- `n_m/workspace/link-to-pkg` is a symlink to '../workspace-pkg'.
- When resolving the package scope of `n_m/workspace/link-to-pkg/subpath`, we should check:
 - `n_m/workspace/link-to-pkg` (realpath: `../workspace-pkg`)
 - `n_m/workspace` (realpath: `n_m/workspace`)

In particular, when traversing to `../workspace-pkg`, we should *not* collect `..` but we *should* collect `../workspace-pkg`.

We attempt to do this by keeping track of an `unseenPathFromIdx`, which lets us skip ancestor collection when we're traversing from the root to the symlink target.

The logic here was effectively off-by-one - we were correctly skipping segments before the symlink target, and correctly collecting segments after, but we missed the symlink target itself.

By setting `unseenPathFromIdx` to the index of the start of the target's basename, we include it correctly.

Fixes #1347

Changelog:
```
- **[Fix]**: Fix #1347, symlinked packages not respecting package.json redirections.
```

Pull Request resolved: #1349

Test Plan:
- New unit test
 - Verified e2e by patching metro-file-map in user's repro in #1347

Reviewed By: cortinico

Differential Revision: D62343439

Pulled By: robhogan

fbshipit-source-id: a823601daeffdc8dfd5b5bc3987b75889d44588f
  • Loading branch information
robhogan authored and facebook-github-bot committed Sep 7, 2024
1 parent f35992c commit f1d5cb3
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 4 deletions.
14 changes: 10 additions & 4 deletions packages/metro-file-map/src/lib/TreeFS.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ function isRegularFile(node: FileNode): boolean {
return node[H.SYMLINK] === 0;
}

type NormalizedSymlinkTarget = {ancestorOfRootIdx: ?number, normalPath: string};
type NormalizedSymlinkTarget = {
ancestorOfRootIdx: ?number,
normalPath: string,
startOfBasenameIdx: number,
};

/**
* OVERVIEW:
Expand Down Expand Up @@ -667,9 +671,10 @@ export default class TreeFS implements MutableFileSystem {
}

// For the purpose of collecting ancestors: Ignore the traversal to
// the symlink target, and start collecting ancestors only when we
// reach the remaining part of the path.
unseenPathFromIdx = normalSymlinkTarget.normalPath.length;
// the symlink target, and start collecting ancestors only
// from the target itself (ie, the basename of the normal target path)
// onwards.
unseenPathFromIdx = normalSymlinkTarget.startOfBasenameIdx;

if (seen == null) {
// Optimisation: set this lazily only when we've encountered a symlink
Expand Down Expand Up @@ -1122,6 +1127,7 @@ export default class TreeFS implements MutableFileSystem {
ancestorOfRootIdx:
this.#pathUtils.getAncestorOfRootIdx(normalSymlinkTarget),
normalPath: normalSymlinkTarget,
startOfBasenameIdx: normalSymlinkTarget.lastIndexOf(path.sep) + 1,
};
this.#cachedNormalSymlinkTargets.set(symlinkNode, result);
return result;
Expand Down
11 changes: 11 additions & 0 deletions packages/metro-file-map/src/lib/__tests__/TreeFS-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,10 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
p('a/b/c/d/link-to-A'),
['', 0, 0, 0, '', '', p('../../../../../..')],
],
[
p('n_m/workspace/link-to-pkg'),
['', 0, 0, 0, '', '', p('../../../workspace-pkg')],
],
].concat(
[
'a/package.json',
Expand All @@ -331,6 +335,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
'a/n_m/pkg/n_m/pkg2/package.json',
'../../package.json',
'../../../a/b/package.json',
'../workspace-pkg/package.json',
].map(posixPath => [p(posixPath), ['', 0, 0, 0, '', '', 0]]),
),
),
Expand Down Expand Up @@ -447,6 +452,12 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
['/A/B/C/a/n_m/pkg3/foo.js', null, null, ['/A/B/C/a/n_m/pkg3']],
// Does not look beyond n_m, if n_m does not exist
['/A/B/C/a/b/n_m/pkg/foo', null, null, ['/A/B/C/a/b/n_m']],
[
'/A/B/C/n_m/workspace/link-to-pkg/subpath',
'/A/B/workspace-pkg/package.json',
'subpath',
['/A/B/C/n_m/workspace/link-to-pkg', '/A/B/workspace-pkg/subpath'],
],
])(
'%s => %s (relative %s, invalidatedBy %s)',
(
Expand Down

0 comments on commit f1d5cb3

Please sign in to comment.