Fix recursive require.context when the context root is an ancestor of the project root #1281
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D57844039 |
… the project root (facebook#1281) Summary: ## 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
robhogan
force-pushed
the
export-D57844039
branch
from
24adcf1
to
4c23dc0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57844039 |
robhogan
added a commit
to robhogan/metro
that referenced
this pull request
Jun 4, 2024
… the project root (facebook#1281) Summary: ## 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
|
This pull request has been merged in a2f72d1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Background
(Same as previous diff)
The internal implementation of
TreeFSuses 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../outsidetraverses 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
_lookupByNormalPathnow returns information on whether the returned node is an ancestor of the project root.matchFileslooks up the search root (context root, forrequire.context()), and uses the newancestorOfRootIdxto potentially iterate over one extra node. This is a negligible constant-time increase in complexity for_lookupByNormalPathandmatchFiles.Changelog:
Reviewed By: huntie
Differential Revision: D57844039