From a2f72d11f5e4350709fef2b1723dddd9ad9f184c Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Tue, 4 Jun 2024 06:34:40 -0700 Subject: [PATCH] Fix recursive require.context when the context root is an ancestor of the project root (#1281) Summary: Pull Request resolved: https://github.com/facebook/metro/pull/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 --- packages/metro-file-map/src/lib/TreeFS.js | 79 ++++++++++++++++--- .../src/lib/__tests__/TreeFS-test.js | 56 +++++++++++++ 2 files changed, 124 insertions(+), 11 deletions(-) diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index dd8dbba6a6..4770e7cc94 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -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; } @@ -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( @@ -364,6 +374,7 @@ export default class TreeFS implements MutableFileSystem { } = {followLeaf: true, makeDirectories: false}, ): | { + ancestorOfRootIdx: ?number, canonicalLinkPaths: Array, canonicalPath: string, exists: true, @@ -371,6 +382,7 @@ export default class TreeFS implements MutableFileSystem { parentNode: DirectoryNode, } | { + ancestorOfRootIdx: ?number, canonicalLinkPaths: Array, canonicalPath: string, exists: true, @@ -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); @@ -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 { @@ -433,6 +457,7 @@ export default class TreeFS implements MutableFileSystem { opts.followLeaf === false) ) { return { + ancestorOfRootIdx, canonicalLinkPaths, canonicalPath: targetNormalPath, exists: true, @@ -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, @@ -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, @@ -562,7 +604,11 @@ export default class TreeFS implements MutableFileSystem { ): Iterable { 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; } @@ -612,6 +658,8 @@ 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]), @@ -619,7 +667,16 @@ export default class TreeFS implements MutableFileSystem { } } } 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, + ); } } } 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 533d1e4841..18733ac5ae 100644 --- a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js +++ b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js @@ -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', () => { @@ -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( @@ -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'), @@ -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'), ]); });