Conversation
Summary: `TreeFS` told directory nodes apart from file nodes with `node instanceof Map`, which holds only when the tree was built in the same realm as the code reading it. A tree restored through `TreeFS.fromDeserializedSnapshot` is used as-is, so a snapshot deserialized in another realm traverses wrongly: every directory below the root reads as a symlink and lookups through it fail with `Expected symlink target to be populated`, while `hierarchicalLookup` treats the root as a non-directory and returns null without probing it. Nothing in a Metro process crosses a realm, but Jest does: each test file runs in its own `vm` context while `node:v8` is a host module, so a file map cache read back within a test holds `Map`s from another realm. The `metro` tests that build two `DependencyGraph`s on one config have been traversing such a tree, and pass only because the walks they make happen to fail in the direction their expectations need. File nodes are metadata tuples, so `isDirectory` checks `!Array.isArray(node)` instead, which reads an internal slot and is realm-independent. The new test round-trips a snapshot through `v8.serialize`/`deserialize` and looks up a nested file in the result. Changelog: [Internal]
Contributor
|
@vzaidman has imported this pull request. If you are a Meta employee, you can view this in D120355345. |
robhogan
added a commit
that referenced
this pull request
Sep 18, 2026
…andidates Stacked on #1944. ## Summary `resolveFile` tries each candidate extension in turn, and for each one `resolveSourceFileForExt` calls `redirectModulePath`, which asks `context.getPackageForModule` for the package scope of the candidate path. Every candidate in a directory belongs to the same package, so that's the same question asked once per extension - and because Metro's `PackageCache` memoises by module path, each distinct candidate string misses the first time it's seen, mostly for files that don't exist. This asks for the package of the directory once, up front, and shares it with every candidate via the source file context. `redirectPackageSubpath` is the part of `redirectModulePath` that applies `mainFields` redirections when the containing package is already known. `redirectModulePath` delegates to it, and behaves as before for its other callers. One wrinkle: files directly inside a `node_modules` directory belong to no package, but the package of that directory itself is whatever contains it, so `getPackageForFilesIn` handles that case explicitly. `package-scope-test.js` pins the sequence of `getPackageForModule` calls the resolver makes for the common specifier shapes, so that the call count is a tested property rather than incidental. This also removes the `@deprecated` tag from `getPackageForModule` - the resolver depends on it, and its doc now says what it returns for symlinked and missing paths. ### `resetCache` in tests Four tests in `resolver-test.js` build a second `DependencyGraph` on the same config, and the second reads the first's file map cache. The integration tests do the same across test files and across runs, because their file map cache defaults to `os.tmpdir()`. Under Jest a cached tree comes back from `v8.deserialize` with `Map`s from another realm, which fail `TreeFS`'s `instanceof Map` checks. Asking for the package of a directory reaches `hierarchicalLookup`'s root-ancestor invariant, which turns what was previously a silent wrong answer into a failure, so both configs set `resetCache: true`. #1934 fixes the underlying problem - once that lands, the line in `resolver-test.js` can go. The one in the integration config is worth keeping regardless, since those tests shouldn't depend on what a previous run left in the temp directory. ## Benchmark Same harness as #1944 - replaying resolutions under a default RN config with a synthetic app, 30% of source outside projectRoot (6,935 resolutions), 7 interleaved rounds. Resolver output is byte-identical. | metric | #1944 | this diff | |---|---|---| | `getPackageForModule` calls per pass | 64,091 | **17,450 (−72.8%)** | | first pass after startup (ms) | 119.6 | **85.7 (−28.3%)** | | warm pass (ms) | 55.2 | **53.5 (−3.1%)** | | after a source change (ms) | 63.0 | **60.9 (−3.4%)** | | after a `package.json` change (ms) | 68.4 | **64.0 (−6.4%)** | The first pass is where this lands, because that's where every distinct candidate path is a `PackageCache` miss and a walk up the tree. ## Test plan ``` yarn jest packages/metro-resolver packages/metro-file-map packages/metro/src/node-haste packages/metro/src/DeltaBundler/__tests__/resolver-test.js yarn flow check yarn verify-api-snapshots ``` Changelog: Internal
robhogan
added a commit
that referenced
this pull request
Sep 18, 2026
…andidates Stacked on #1944. ## Summary `resolveFile` tries each candidate extension in turn, and for each one `resolveSourceFileForExt` calls `redirectModulePath`, which asks `context.getPackageForModule` for the package scope of the candidate path. Every candidate in a directory belongs to the same package, so that's the same question asked once per extension - and because Metro's `PackageCache` memoises by module path, each distinct candidate string misses the first time it's seen, mostly for files that don't exist. This asks for the package of the directory once, up front, and shares it with every candidate via the source file context. `redirectPackageSubpath` is the part of `redirectModulePath` that applies `mainFields` redirections when the containing package is already known. `redirectModulePath` delegates to it, and behaves as before for its other callers. One wrinkle: files directly inside a `node_modules` directory belong to no package, but the package of that directory itself is whatever contains it, so `getPackageForFilesIn` handles that case explicitly. `package-scope-test.js` pins the sequence of `getPackageForModule` calls the resolver makes for the common specifier shapes, so that the call count is a tested property rather than incidental. This also removes the `@deprecated` tag from `getPackageForModule` - the resolver depends on it, and its doc now says what it returns for symlinked and missing paths. ### `resetCache` in tests Four tests in `resolver-test.js` build a second `DependencyGraph` on the same config, and the second reads the first's file map cache. The integration tests do the same across test files and across runs, because their file map cache defaults to `os.tmpdir()`. Under Jest a cached tree comes back from `v8.deserialize` with `Map`s from another realm, which fail `TreeFS`'s `instanceof Map` checks. Asking for the package of a directory reaches `hierarchicalLookup`'s root-ancestor invariant, which turns what was previously a silent wrong answer into a failure, so both configs set `resetCache: true`. #1934 fixes the underlying problem - once that lands, the line in `resolver-test.js` can go. The one in the integration config is worth keeping regardless, since those tests shouldn't depend on what a previous run left in the temp directory. ## Benchmark This cuts package scope lookups 3.7x, and takes 28% off first-pass resolution time, on our benchmark app (described in the test plan). 7 interleaved rounds. Resolver output is byte-identical. | metric | #1944 | this diff | |---|---|---| | `getPackageForModule` calls per pass | 64k | **17k (3.7x fewer)** | | first pass after startup (ms) | 119.6 | **85.7 (−28.3%)** | | warm pass (ms) | 55.2 | **53.5 (−3.1%)** | | after a source change (ms) | 63.0 | **60.9 (−3.4%)** | | after a `package.json` change (ms) | 68.4 | **64.0 (−6.4%)** | The first pass is where this lands, because that's where every distinct candidate path is a `PackageCache` miss and a walk up the tree. ## Test plan ``` yarn jest packages/metro-resolver packages/metro-file-map packages/metro/src/node-haste packages/metro/src/DeltaBundler/__tests__/resolver-test.js yarn flow check yarn verify-api-snapshots ``` **Benchmark app** - a synthetic mid-size RN 0.87 app: 23 common dependencies (Reanimated, React Navigation, TanStack Query, lodash, etc.) and ~1,000 generated first-party modules, 60% of them in a workspace package outside `projectRoot` with its own `node_modules`. The iOS dev bundle is 2.8k modules, from a file map of 34k files. Timings replay the resolutions a real build of it makes against a fresh `DependencyGraph` per process, in interleaved rounds. Changelog: Internal
robhogan
added a commit
that referenced
this pull request
Sep 18, 2026
…andidates Stacked on #1944. ## Summary `resolveFile` tries each candidate extension in turn, and for each one `resolveSourceFileForExt` calls `redirectModulePath`, which asks `context.getPackageForModule` for the package scope of the candidate path. Every candidate in a directory belongs to the same package, so that's the same question asked once per extension - and because Metro's `PackageCache` memoises by module path, each distinct candidate string misses the first time it's seen, mostly for files that don't exist. This asks for the package of the directory once, up front, and shares it with every candidate via the source file context. `redirectPackageSubpath` is the part of `redirectModulePath` that applies `mainFields` redirections when the containing package is already known. `redirectModulePath` delegates to it, and behaves as before for its other callers. One wrinkle: files directly inside a `node_modules` directory belong to no package, but the package of that directory itself is whatever contains it, so `getPackageForFilesIn` handles that case explicitly. `package-scope-test.js` pins the sequence of `getPackageForModule` calls the resolver makes for the common specifier shapes, so that the call count is a tested property rather than incidental. This also removes the `@deprecated` tag from `getPackageForModule` - the resolver depends on it, and its doc now says what it returns for symlinked and missing paths. ### `resetCache` in tests Four tests in `resolver-test.js` build a second `DependencyGraph` on the same config, and the second reads the first's file map cache. The integration tests do the same across test files and across runs, because their file map cache defaults to `os.tmpdir()`. Under Jest a cached tree comes back from `v8.deserialize` with `Map`s from another realm, which fail `TreeFS`'s `instanceof Map` checks. Asking for the package of a directory reaches `hierarchicalLookup`'s root-ancestor invariant, which turns what was previously a silent wrong answer into a failure, so both configs set `resetCache: true`. #1934 fixes the underlying problem - once that lands, the line in `resolver-test.js` can go. The one in the integration config is worth keeping regardless, since those tests shouldn't depend on what a previous run left in the temp directory. ## Benchmark This cuts package scope lookups 3.7x, and takes 28% off first-pass resolution time, measured in isolation on our benchmark app (described in the test plan). 7 interleaved rounds. Resolver output is byte-identical. | metric | #1944 | this diff | |---|---|---| | `getPackageForModule` calls per pass | 64k | **17k (3.7x fewer)** | | first pass after startup (ms) | 119.6 | **85.7 (−28.3%)** | | warm pass (ms) | 55.2 | **53.5 (−3.1%)** | | after a source change (ms) | 63.0 | **60.9 (−3.4%)** | | after a `package.json` change (ms) | 68.4 | **64.0 (−6.4%)** | The first pass is where this lands, because that's where every distinct candidate path is a `PackageCache` miss and a walk up the tree. ## Test plan ``` yarn jest packages/metro-resolver packages/metro-file-map packages/metro/src/node-haste packages/metro/src/DeltaBundler/__tests__/resolver-test.js yarn flow check yarn verify-api-snapshots ``` **Benchmark app** - a synthetic mid-size RN 0.87 app: 23 common dependencies (Reanimated, React Navigation, TanStack Query, lodash, etc.) and ~1,000 generated first-party modules, 60% of them in a workspace package outside `projectRoot` with its own `node_modules`. The iOS dev bundle is 2.8k modules, from a file map of 34k files. Timings replay the resolutions a real build of it makes against a fresh `DependencyGraph` per process, in interleaved rounds. These timings isolate resolution - no transformation, serialisation or file reads are included. A real build of this app takes around 4s even with a warm transform cache, so resolution is about 2% of it. Changelog: Internal
This file contains hidden or 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
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.
TreeFSuses an internalinstanceof Mapcheck in its internalisDirectoryfunction, to identify whether a given node represented a directory (Map) or file (Array/tuple).The only problem with
instanceof Mapis under Jest, in tests where we're exercising the file map cache. In Jest, tests are executed in v8 contexts whereMapgets a new prototype, such that a file map cache v8-serialised in one test is deserialised but can't be traversed in another test, becauseinstanceof Mapchecks fail on the foreign maps.I discovered this while writing some unrelated tests that failed unexpectedly. We already have at least three tests in main that pass by accident (
isDirectoryreturns false despite the node being aMap, just a foreign one) and effectively weren't testing anything. (Luckily, they all pass anyway)An easy fix is to invert the implementation of
isDirectoryso that it becomes "not null and not a file", usingArray.isArray, which is portable across realms.As it turns out, this is also marginally faster.
Changelog: [Internal]
Test plan
Microbenchmark
isDirectoryis hot so just to confirm this doesn't regress:node instanceof Mapnode != null && !Array.isArray(node)Array.isArrayis marginally faster with a mix of inputs - implemented with a slot read rather than a prototype walk.