Skip to content

Commit

Permalink
Fix recursive require.context when the context root is an ancestor of…
Browse files Browse the repository at this point in the history 
… the project root (#1281)

Summary:
Pull Request resolved: #1281

## Background
(Same as previous diff)

The internal implementation of `TreeFS` uses a tree of maps representing file path segments, with a "root" node at the *project root*. For paths outside the project root, we traverse through `'..'` nodes, so that `../outside` traverses from the project root through `'..'` and `'outside'`. Basing the representation around the project root (as opposed to a volume root) minimises traversal through irrelevant paths and makes the cache portable.

## Problem
However, because this map of maps is a simple (acyclic) tree, a `'..'` node has no entry for one of its logical (=on disk) children - the node closer to the project root. When (recursively) enumerating paths under an ancestor of the project root, the descendent part of the project root will be missing if we only enumerate entries of the Map.

## Observable bugs (in experimental features)
For enumerations we miss out (ancestors of) the project root where they should be included, eg `matchFiles('..', {recursive: true})` will not include the project root subtree. This is observable through the (experimental) `require.context()` API.

## This fix
`_lookupByNormalPath` now returns information on whether the returned node is an ancestor of the project root. `matchFiles` looks up the search root (context root, for `require.context()`), and uses the new `ancestorOfRootIdx` to potentially iterate over one extra node. This is a negligible constant-time increase in complexity for `_lookupByNormalPath` and `matchFiles`.

Changelog:
```
 - **[Experimental]**: Fix subtrees of the project root missed from `require.context` when using an ancestor of the project root as context root.
```

Reviewed By: huntie

Differential Revision: D57844039

fbshipit-source-id: 9150ac3d3e0629be4e11247e87b70253b2aa5ff2
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Jun 4, 2024
1 parent 37eeb0d commit a2f72d1
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 11 deletions.
79 changes: 68 additions & 11 deletions packages/metro-file-map/src/lib/TreeFS.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,12 @@ export default class TreeFS implements MutableFileSystem {
if (!contextRootResult.exists) {
return;
}
const {canonicalPath: rootRealPath, node: contextRoot} = contextRootResult;
const {
ancestorOfRootIdx,
canonicalPath: rootRealPath,
node: contextRoot,
parentNode: contextRootParent,
} = contextRootResult;
if (!(contextRoot instanceof Map)) {
return;
}
Expand All @@ -245,13 +250,18 @@ export default class TreeFS implements MutableFileSystem {
? contextRootAbsolutePath.replaceAll(path.sep, '/')
: contextRootAbsolutePath;

for (const relativePathForComparison of this._pathIterator(contextRoot, {
alwaysYieldPosix: filterComparePosix,
canonicalPathOfRoot: rootRealPath,
follow,
recursive,
subtreeOnly: rootDir != null,
})) {
for (const relativePathForComparison of this._pathIterator(
contextRoot,
contextRootParent,
ancestorOfRootIdx,
{
alwaysYieldPosix: filterComparePosix,
canonicalPathOfRoot: rootRealPath,
follow,
recursive,
subtreeOnly: rootDir != null,
},
)) {
if (
filter == null ||
filter.test(
Expand Down Expand Up @@ -364,13 +374,15 @@ export default class TreeFS implements MutableFileSystem {
} = {followLeaf: true, makeDirectories: false},
):
| {
ancestorOfRootIdx: ?number,
canonicalLinkPaths: Array<string>,
canonicalPath: string,
exists: true,
node: MixedNode,
parentNode: DirectoryNode,
}
| {
ancestorOfRootIdx: ?number,
canonicalLinkPaths: Array<string>,
canonicalPath: string,
exists: true,
Expand All @@ -393,6 +405,9 @@ export default class TreeFS implements MutableFileSystem {
let fromIdx = 0;
// The parent of the current segment
let parentNode = this.#rootNode;
// If a returned node is a strict ancestor of the root, this is the number
// of levels below the root, i.e. '..' is 1, '../..' is 2, otherwise null.
let ancestorOfRootIdx: ?number = null;

while (targetNormalPath.length > fromIdx) {
const nextSepIdx = targetNormalPath.indexOf(path.sep, fromIdx);
Expand All @@ -408,6 +423,15 @@ export default class TreeFS implements MutableFileSystem {

let segmentNode = parentNode.get(segmentName);

// In normal paths all indirections are at the prefix, so we are at the
// nth ancestor of the root iff the path so far is n '..' segments.
if (segmentName === '..') {
ancestorOfRootIdx =
ancestorOfRootIdx == null ? 1 : ancestorOfRootIdx + 1;
} else if (segmentNode != null) {
ancestorOfRootIdx = null;
}

if (segmentNode == null) {
if (opts.makeDirectories !== true && segmentName !== '..') {
return {
Expand All @@ -433,6 +457,7 @@ export default class TreeFS implements MutableFileSystem {
opts.followLeaf === false)
) {
return {
ancestorOfRootIdx,
canonicalLinkPaths,
canonicalPath: targetNormalPath,
exists: true,
Expand Down Expand Up @@ -493,6 +518,7 @@ export default class TreeFS implements MutableFileSystem {
}
invariant(parentNode === this.#rootNode, 'Unexpectedly escaped traversal');
return {
ancestorOfRootIdx: null,
canonicalLinkPaths,
canonicalPath: targetNormalPath,
exists: true,
Expand Down Expand Up @@ -544,12 +570,28 @@ export default class TreeFS implements MutableFileSystem {
: this.#pathUtils.relativeToNormal(relativeOrAbsolutePath);
}

*#directoryNodeIterator(
node: DirectoryNode,
parent: ?DirectoryNode,
ancestorOfRootIdx: ?number,
): Iterator<[string, MixedNode]> {
if (ancestorOfRootIdx != null && parent) {
yield [
this.#pathUtils.getBasenameOfNthAncestor(ancestorOfRootIdx - 1),
parent,
];
}
yield* node.entries();
}

/**
* Enumerate paths under a given node, including symlinks and through
* symlinks (if `follow` is enabled).
*/
*_pathIterator(
rootNode: DirectoryNode,
iterationRootNode: DirectoryNode,
iterationRootParentNode: ?DirectoryNode,
ancestorOfRootIdx: ?number,
opts: $ReadOnly<{
alwaysYieldPosix: boolean,
canonicalPathOfRoot: string,
Expand All @@ -562,7 +604,11 @@ export default class TreeFS implements MutableFileSystem {
): Iterable<Path> {
const pathSep = opts.alwaysYieldPosix ? '/' : path.sep;
const prefixWithSep = pathPrefix === '' ? pathPrefix : pathPrefix + pathSep;
for (const [name, node] of rootNode ?? this.#rootNode) {
for (const [name, node] of this.#directoryNodeIterator(
iterationRootNode,
iterationRootParentNode,
ancestorOfRootIdx,
)) {
if (opts.subtreeOnly && name === '..') {
continue;
}
Expand Down Expand Up @@ -612,14 +658,25 @@ export default class TreeFS implements MutableFileSystem {
// the path where we found the symlink as a prefix.
yield* this._pathIterator(
target,
resolved.parentNode,
resolved.ancestorOfRootIdx,
opts,
nodePath,
new Set([...followedLinks, node]),
);
}
}
} else if (opts.recursive) {
yield* this._pathIterator(node, opts, nodePath, followedLinks);
yield* this._pathIterator(
node,
iterationRootParentNode,
ancestorOfRootIdx != null && ancestorOfRootIdx > 1
? ancestorOfRootIdx - 1
: null,
opts,
nodePath,
followedLinks,
);
}
}
}
Expand Down
56 changes: 56 additions & 0 deletions packages/metro-file-map/src/lib/__tests__/TreeFS-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,25 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
});
},
);

test('matchFiles follows links up', () => {
const matches = [
...tfs.matchFiles({
rootDir: p('/project/foo'),
follow: true,
recursive: true,
}),
];
expect(matches).toContain(
p('/project/foo/link-up-2/project/foo/another.js'),
);
// Only follow a symlink cycle once.
expect(matches).not.toContain(
p(
'/project/foo/link-up-2/project/foo/link-up-2/project/foo/another.js',
),
);
});
});

describe('getDifference', () => {
Expand Down Expand Up @@ -312,6 +331,23 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
).toEqual([p('/outside/external.js')]);
});

test('ancestor of project root includes project root', () => {
expect(
Array.from(
tfs.matchFiles({
filter: new RegExp(
// Test starting with `./` since this is mandatory for parity with Webpack.
/^\.\/.*\/bar\.js/,
),
filterComparePosix: true,
follow: true,
recursive: true,
rootDir: p('/'),
}),
),
).toEqual([p('/project/bar.js')]);
});

test('recursive', () => {
expect(
Array.from(
Expand All @@ -337,6 +373,22 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
p('/project/link-to-foo/link-to-bar.js'),
p('/project/link-to-foo/link-to-another.js'),
p('/project/abs-link-out/external.js'),
p('/project/root/project/foo/another.js'),
p('/project/root/project/foo/owndir/another.js'),
p('/project/root/project/foo/owndir/link-to-bar.js'),
p('/project/root/project/foo/owndir/link-to-another.js'),
p('/project/root/project/foo/link-to-bar.js'),
p('/project/root/project/foo/link-to-another.js'),
p('/project/root/project/bar.js'),
p('/project/root/project/link-to-foo/another.js'),
p('/project/root/project/link-to-foo/owndir/another.js'),
p('/project/root/project/link-to-foo/owndir/link-to-bar.js'),
p('/project/root/project/link-to-foo/owndir/link-to-another.js'),
p('/project/root/project/link-to-foo/link-to-bar.js'),
p('/project/root/project/link-to-foo/link-to-another.js'),
p('/project/root/project/abs-link-out/external.js'),
p('/project/root/project/node_modules/pkg/a.js'),
p('/project/root/project/node_modules/pkg/package.json'),
p('/project/root/outside/external.js'),
p('/project/node_modules/pkg/a.js'),
p('/project/node_modules/pkg/package.json'),
Expand Down Expand Up @@ -379,6 +431,10 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
p('/project/foo/owndir/another.js'),
p('/project/link-to-foo/another.js'),
p('/project/link-to-foo/owndir/another.js'),
p('/project/root/project/foo/another.js'),
p('/project/root/project/foo/owndir/another.js'),
p('/project/root/project/link-to-foo/another.js'),
p('/project/root/project/link-to-foo/owndir/another.js'),
]);
});

Expand Down

0 comments on commit a2f72d1

Please sign in to comment.