From e8ab1033a9304f0a4e3784a0dfb7788fc5ede2ed Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Thu, 15 Oct 2026 07:09:26 +0100 Subject: [PATCH 1/3] metro-file-map: Give plugins getOrComputeFileData, record observed content https://github.com/react/metro/pull/1944 lets `FileSystem.lookup` record what it observed, but a file found only goes in `existence`. A plugin's per-file data is derived from the file's contents, so reading it should be recorded in `content`, which is also invalidated by modification. Plugins currently read their data from `lookup()`'s `pluginData`, and a lazy plugin calls `processFile` when that's `undefined`. Recording on either wouldn't be enough - on a warm start, or any read after the first, the data is already stored and `processFile` never runs. This replaces both with `getOrComputeFileData(path, observations?)`, on plugin init options and `FileDataPlugin`. It returns the stored data, or for a lazy plugin runs the worker in-band on a miss, and records the file in `content` either way. A missing path or directory is a result rather than an error, so a caller falling back to something else has still recorded it. Plugin `lookup` no longer returns data, so `getOrComputeFileData` is the only per-file read. The motivation is `package.json`, which Metro currently reads with `fs` outside the file map, leaving nothing to record: https://github.com/react/metro/blob/32ff3c9993384580ca40310f3c4024e5f250837b/packages/metro/src/node-haste/PackageCache.js#L24-L25 Changelog: Internal Test plan: New unit tests. Nothing passes observations to this yet, so it's only reachable from tests until the `package.json` plugin follows. --- packages/metro-file-map/API.md | 12 ++- .../src/__tests__/index-test.js | 82 +++++++++++++++---- packages/metro-file-map/src/flow-types.js | 45 ++++++---- packages/metro-file-map/src/index.js | 70 +++++++++------- packages/metro-file-map/src/lib/TreeFS.js | 24 +++++- .../src/lib/__tests__/TreeFS-test.js | 40 +++++++++ .../src/plugins/DependencyPlugin.js | 4 +- .../src/plugins/FileDataPlugin.js | 27 +++--- .../metro-file-map/src/plugins/HastePlugin.js | 6 +- .../plugins/__tests__/FileDataPlugin-test.js | 34 ++++++-- .../__tests__/DependencyPlugin-test.js | 55 ++++++++----- .../haste/__tests__/HastePlugin-test.js | 24 +++--- .../mocks/__tests__/MockPlugin-test.js | 2 +- 13 files changed, 304 insertions(+), 121 deletions(-) diff --git a/packages/metro-file-map/API.md b/packages/metro-file-map/API.md index 6e69a8edc1..de3591faea 100644 --- a/packages/metro-file-map/API.md +++ b/packages/metro-file-map/API.md @@ -124,12 +124,12 @@ export class FileDataPlugin['files']; + getOrComputeFileData(mixedPath: string, observations?: null | undefined | Observations): ReturnType['getOrComputeFileData']>; getSerializableSnapshot(): null; getWorker(): FileMapPluginWorker; initialize(initOptions: FileMapPluginInitOptions): Promise; readonly name: string; onChanged(_changes: ReadonlyFileSystemChanges): void; - processFile(mixedPath: string): ReturnType['processFile']>; } export type FileDataPluginOptions = Readonly< @@ -139,6 +139,16 @@ export type FileDataPluginOptions = Readonly< } >; +export type FileDataResult = + | Readonly<{exists: false}> + | Readonly<{exists: true; type: 'd'}> + | Readonly<{ + exists: true; + type: 'f'; + realPath: string; + data: null | undefined | PerFileData; + }>; + class FileMap extends EventEmitter { constructor(options: InputOptions); build(): Promise; diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index 9ac6da3611..483c1a6629 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -445,6 +445,51 @@ describe('FileMap', () => { expect(FileMap.H).toBe(require('../constants').default); }); + test('getOrComputeFileData records a file found as content, and anything else as existence', async () => { + const {dependencyPlugin} = await buildNewFileMap(); + if (dependencyPlugin == null) { + throw new Error('Expected a DependencyPlugin'); + } + const observations = { + existence: new Set(), + content: new Set(), + }; + + expect( + dependencyPlugin.getOrComputeFileData( + p('/project/fruits/Banana.js'), + observations, + ), + ).toMatchObject({exists: true, type: 'f'}); + expect( + dependencyPlugin.getOrComputeFileData(p('/project/fruits'), observations), + ).toEqual({exists: true, type: 'd'}); + expect( + dependencyPlugin.getOrComputeFileData( + p('/project/fruits/Missing.js'), + observations, + ), + ).toEqual({exists: false}); + + expect(observations).toEqual({ + content: new Set([path.join('fruits', 'Banana.js')]), + existence: new Set(['fruits', path.join('fruits', 'Missing.js')]), + }); + }); + + test("getOrComputeFileData gives null for a file a non-lazy plugin's worker skipped", async () => { + mockFs[p('/project/fruits/Pear.json')] = '{}'; + const {dependencyPlugin} = await buildNewFileMap({ + extensions: ['js', 'json'], + }); + if (dependencyPlugin == null) { + throw new Error('Expected a DependencyPlugin'); + } + expect( + dependencyPlugin.getOrComputeFileData(p('/project/fruits/Pear.json')), + ).toMatchObject({exists: true, type: 'f', data: null}); + }); + test('ignores files given a pattern', async () => { mockFs[p('/project/fruits/Kiwi.js')] = ` // Kiwi! @@ -1643,26 +1688,27 @@ describe('FileMap', () => { watch: true, plugins: [lazyPlugin], }); + const onMetadata = jest.fn(); await fileMap.build(); try { const bananaPath = p('/project/fruits/Banana.js'); - const getPluginData = () => { - const result = lazyPlugin.getFileSystem().lookup(bananaPath); - if (!result.exists || result.type !== 'f') { - throw new Error('Expected a file'); - } - return result.pluginData; - }; - const onMetadata = jest.fn(); fileMap.on('metadata', onMetadata); - // The crawl does not run a lazy plugin's worker - expect(getPluginData()).toBeUndefined(); - const expected = {length: String(mockFs[bananaPath]).length}; - expect(lazyPlugin.processFile(bananaPath)).toEqual(expected); - // The data is stored, and the cache told there is something to save - expect(getPluginData()).toEqual(expected); + expect(lazyPlugin.getOrComputeFileData(bananaPath)).toEqual({ + exists: true, + type: 'f', + realPath: bananaPath, + data: expected, + }); + // The crawl did not run the worker, so it ran now, and the cache is + // told there is something to save + expect(onMetadata).toHaveBeenCalledTimes(1); + + // The data is stored + expect(lazyPlugin.getOrComputeFileData(bananaPath)).toMatchObject({ + data: expected, + }); expect(onMetadata).toHaveBeenCalledTimes(1); mockFs[bananaPath] = '// A changed banana'; @@ -1673,11 +1719,11 @@ describe('FileMap', () => { }); await waitForItToChange(fileMap); - // Nor does a change, which leaves the file unprocessed again - expect(getPluginData()).toBeUndefined(); - expect(lazyPlugin.processFile(bananaPath)).toEqual({ - length: '// A changed banana'.length, + // A change leaves the file unprocessed again + expect(lazyPlugin.getOrComputeFileData(bananaPath)).toMatchObject({ + data: {length: '// A changed banana'.length}, }); + expect(onMetadata).toHaveBeenCalledTimes(2); } finally { await fileMap.end(); } diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index b42372a064..8b5ad5739c 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -230,26 +230,43 @@ export type FileMapPluginInitOptions< }>, lookup( mixedPath: string, - ): - | {exists: false} - | {exists: true, type: 'f', readonly pluginData: PerFileData} - | {exists: true, type: 'd'}, + ): {exists: false} | {exists: true, type: 'f' | 'd'}, }>, pluginState: ?SerializableState, /** - * Synchronously run this plugin's worker, in-band, on the regular file at - * `mixedPath`, store the result as its plugin data and return it. Throws if - * the path is not a regular file, or the plugin has no worker. + * This plugin's data for the regular file at `mixedPath`. Throws if the + * plugin has no worker. * - * Plugin data is `undefined` for a file until a worker has run on it, which - * is when a lazy plugin should call this, and is reset to `undefined` when - * the file changes. `null` is a result like any other, so what a worker - * returns is stored with `undefined` replaced by `null`, and a file is - * processed at most once while it is unchanged. + * A lazy plugin's worker is run on a file, synchronously and in-band, the + * first time its data is asked for, and again after the file changes. The + * result is stored, with `undefined` replaced by `null`. + * + * A non-lazy plugin's worker has already run on every file it can give + * non-null data for, so a file it has not run on (eg, one its filter + * excludes) has `null` data, without running it. + * + * If `observations` is given, records what the result depends upon: a + * regular file found in `content`, since its data is derived from what it + * holds, and otherwise as `FileSystem.lookup` does. This is the same whether + * the data was stored or computed. */ - processFile: (mixedPath: string) => PerFileData, + getOrComputeFileData: ( + mixedPath: string, + observations?: ?Observations, + ) => FileDataResult, }>; +export type FileDataResult = + | Readonly<{exists: false}> + | Readonly<{exists: true, type: 'd'}> + | Readonly<{ + exists: true, + type: 'f', + // The real, absolute path of the file. + realPath: string, + data: ?PerFileData, + }>; + export type FileMapPluginWorker = Readonly<{ worker: Readonly<{ modulePath: string, @@ -264,7 +281,7 @@ export type FileMapPluginWorker = Readonly<{ /** * If true, the worker is not run on files as they are crawled or changed, * and `filter` is not consulted. It runs only on the files the plugin asks - * for, through the `processFile` it is given on initialization. Use this + * for, through the `getOrComputeFileData` it is given on initialization. Use this * where a small, unpredictable subset of matching files is ever needed. */ lazy?: boolean, diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index e2a56abd43..eeb5ea6fe3 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -164,6 +164,7 @@ export type { CrawlerOptions, CrawlResult, DependencyExtractor, + FileDataResult, FileMapRoot, WatcherStatus, } from './flow-types'; @@ -459,18 +460,9 @@ export default class FileMap extends EventEmitter { files: { lookup: mixedPath => { const result = fileSystem.lookup(mixedPath); - if (!result.exists) { - return {exists: false}; - } - if (result.type === 'd') { - return {exists: true, type: 'd'}; - } - return { - exists: true, - type: 'f', - pluginData: - dataIdx != null ? result.metadata[dataIdx] : null, - }; + return result.exists + ? {exists: true, type: result.type} + : {exists: false}; }, fileIterator: opts => mapIterable( @@ -483,31 +475,49 @@ export default class FileMap extends EventEmitter { ), }, pluginState: initialData?.plugins.get(plugin.name), - processFile: mixedPath => { + getOrComputeFileData: (mixedPath, observations) => { invariant( dataIdx != null, 'metro-file-map: Plugin "%s" has no worker to process files with', plugin.name, ); - const result = fileSystem.lookup(mixedPath); - if (!result.exists || result.type !== 'f') { - throw new Error( - `metro-file-map: Cannot process ${mixedPath}, which is not a regular file`, - ); - } - const pluginData = this.#fileProcessor.processFileForPlugin( - result.realPath, - result.metadata, - dataIdx - H.PLUGINDATA, - ); - debug( - 'Lazily processed file for %s: %s', - plugin.name, + const result = fileSystem.lookupContent( mixedPath, + observations, ); - // Inform caches that there is new data to save. - this.emit('metadata'); - return pluginData; + if (!result.exists) { + return {exists: false}; + } + if (result.type === 'd') { + return {exists: true, type: 'd'}; + } + let data = result.metadata[dataIdx]; + // `undefined` means no worker has run on this file. + // eslint-disable-next-line lint/strictly-null + if (data === undefined) { + if (plugin.getWorker()?.lazy === true) { + data = this.#fileProcessor.processFileForPlugin( + result.realPath, + result.metadata, + dataIdx - H.PLUGINDATA, + ); + debug( + 'Lazily processed file for %s: %s', + plugin.name, + mixedPath, + ); + // Inform caches that there is new data to save. + this.emit('metadata'); + } else { + data = null; + } + } + return { + exists: true, + type: 'f', + realPath: result.realPath, + data, + }; }, }), ), diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index 736d2fe375..01b5b8caf6 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -287,6 +287,23 @@ export default class TreeFS implements MutableFileSystem { } lookup(mixedPath: Path, observations?: ?Observations): LookupResult { + return this.#lookup(mixedPath, observations, false); + } + + /** + * As `lookup`, for a caller that goes on to use what a regular file found + * holds. That file is recorded in `observations.content` rather than + * `existence`, since a modification can change the caller's answer. + */ + lookupContent(mixedPath: Path, observations?: ?Observations): LookupResult { + return this.#lookup(mixedPath, observations, true); + } + + #lookup( + mixedPath: Path, + observations: ?Observations, + observeContent: boolean, + ): LookupResult { const normalPath = this.#normalizePath(mixedPath); const result = this.#lookupByNormalPath(normalPath, { observations, @@ -300,7 +317,12 @@ export default class TreeFS implements MutableFileSystem { } const {canonicalPath, node} = result; if (observations) { - observations.existence.add(canonicalPath); + // `content` is invalidated by everything `existence` is, so a path + // belongs in only one of them. + (observeContent && !isDirectory(node) + ? observations.content + : observations.existence + ).add(canonicalPath); } const realPath = this.#pathUtils.normalToAbsolute(canonicalPath); if (isDirectory(node)) { 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 a429a7eb05..a2a55bce5e 100644 --- a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js +++ b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js @@ -223,6 +223,46 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { }, ); + describe('lookupContent', () => { + test('records a file found in content, not existence', () => { + const observations = emptyObservations(); + expect( + tfs.lookupContent(p('/project/bar.js'), observations), + ).toMatchObject({exists: true, type: 'f'}); + expect(observations).toEqual({ + content: new Set([canonical(p('/project/bar.js'))]), + existence: new Set(), + }); + }); + + test('records the real path of a file found through a symlink, with the link', () => { + const observations = emptyObservations(); + expect( + tfs.lookupContent(p('/project/foo/link-to-bar.js'), observations), + ).toMatchObject({exists: true, realPath: p('/project/bar.js')}); + expect(observations).toEqual({ + content: new Set([ + canonical(p('/project/foo/link-to-bar.js')), + canonical(p('/project/bar.js')), + ]), + existence: new Set(), + }); + }); + + test('records a directory found, or a missing path, in existence', () => { + const observations = emptyObservations(); + tfs.lookupContent(p('/project/foo'), observations); + tfs.lookupContent(p('/project/foo/missing.js'), observations); + expect(observations).toEqual({ + content: new Set(), + existence: new Set([ + canonical(p('/project/foo')), + canonical(p('/project/foo/missing.js')), + ]), + }); + }); + }); + test('traversing the same symlink multiple times does not imply a cycle', () => { expect( tfs.lookup(p('/project/foo/owndir/owndir/another.js')), diff --git a/packages/metro-file-map/src/plugins/DependencyPlugin.js b/packages/metro-file-map/src/plugins/DependencyPlugin.js index 50f5551ef3..30ecf2bdd0 100644 --- a/packages/metro-file-map/src/plugins/DependencyPlugin.js +++ b/packages/metro-file-map/src/plugins/DependencyPlugin.js @@ -62,9 +62,9 @@ export default class DependencyPlugin extends FileDataPlugin { - const result = this.getFileSystem().lookup(mixedPath); + const result = this.getOrComputeFileData(mixedPath); if (result.exists && result.type === 'f') { - return result.pluginData ?? []; + return result.data ?? []; } return null; } diff --git a/packages/metro-file-map/src/plugins/FileDataPlugin.js b/packages/metro-file-map/src/plugins/FileDataPlugin.js index 555e55c5e5..4781f1d561 100644 --- a/packages/metro-file-map/src/plugins/FileDataPlugin.js +++ b/packages/metro-file-map/src/plugins/FileDataPlugin.js @@ -13,6 +13,7 @@ import type { FileMapPlugin, FileMapPluginInitOptions, FileMapPluginWorker, + Observations, ReadonlyFileSystemChanges, V8Serializable, } from '../flow-types'; @@ -36,7 +37,10 @@ export default class FileDataPlugin< #worker: FileMapPluginWorker; #cacheKey: string; #files: ?FileMapPluginInitOptions['files']; - #processFile: ?FileMapPluginInitOptions['processFile']; + #getOrComputeFileData: ?FileMapPluginInitOptions< + null, + PerFileData, + >['getOrComputeFileData']; constructor(opts: FileDataPluginOptions) { this.name = opts.name; @@ -52,22 +56,25 @@ export default class FileDataPlugin< initOptions: FileMapPluginInitOptions, ): Promise { this.#files = initOptions.files; - this.#processFile = initOptions.processFile; + this.#getOrComputeFileData = initOptions.getOrComputeFileData; } /** - * Run this plugin's worker now on the file at `mixedPath`, store its data - * and return it. A lazy plugin calls this for a file whose data, as given by - * `getFileSystem().lookup()`, is `undefined`. + * This plugin's data for the file at `mixedPath`, running its worker on the + * file first if the plugin is lazy and the file has no data yet. If + * `observations` is given, a regular file found is recorded in `content`. */ - processFile( + getOrComputeFileData( mixedPath: string, - ): ReturnType['processFile']> { - const processFile = this.#processFile; - if (processFile == null) { + observations?: ?Observations, + ): ReturnType< + FileMapPluginInitOptions['getOrComputeFileData'], + > { + const getOrComputeFileData = this.#getOrComputeFileData; + if (getOrComputeFileData == null) { throw new Error(`${this.name} plugin has not been initialized`); } - return processFile(mixedPath); + return getOrComputeFileData(mixedPath, observations); } getFileSystem(): FileMapPluginInitOptions['files'] { diff --git a/packages/metro-file-map/src/plugins/HastePlugin.js b/packages/metro-file-map/src/plugins/HastePlugin.js index 9827ba6be6..c542493c38 100644 --- a/packages/metro-file-map/src/plugins/HastePlugin.js +++ b/packages/metro-file-map/src/plugins/HastePlugin.js @@ -109,11 +109,11 @@ export default class HastePlugin } } this.#getModuleNameByPath = mixedPath => { - const result = files.lookup(mixedPath); + const result = opts.getOrComputeFileData(mixedPath); return result.exists && result.type === 'f' && - typeof result.pluginData === 'string' - ? result.pluginData + typeof result.data === 'string' + ? result.data : null; }; this.#perfLogger?.point('constructHasteMap_end'); diff --git a/packages/metro-file-map/src/plugins/__tests__/FileDataPlugin-test.js b/packages/metro-file-map/src/plugins/__tests__/FileDataPlugin-test.js index e22b0c8270..5800631548 100644 --- a/packages/metro-file-map/src/plugins/__tests__/FileDataPlugin-test.js +++ b/packages/metro-file-map/src/plugins/__tests__/FileDataPlugin-test.js @@ -9,6 +9,8 @@ * @oncall react_native */ +import type {FileDataResult, Observations} from '../../flow-types'; + import FileDataPlugin from '../FileDataPlugin'; const options = { @@ -36,19 +38,37 @@ describe('FileDataPlugin', () => { ).toEqual({worker: options.worker, filter: options.filter, lazy: true}); }); - test('processFile throws before the plugin is initialized', () => { + test('getOrComputeFileData throws before the plugin is initialized', () => { const plugin = new FileDataPlugin(options); - expect(() => plugin.processFile('/project/package.json')).toThrow( + expect(() => plugin.getOrComputeFileData('/project/package.json')).toThrow( 'test-plugin plugin has not been initialized', ); }); - test('processFile asks the file map to process the file', async () => { + test('getOrComputeFileData asks the file map for the data, with observations', async () => { const plugin = new FileDataPlugin({...options, lazy: true}); - const processFile = jest.fn<[string], ?string>(() => 'file data'); - await plugin.initialize({files, pluginState: null, processFile}); + const result = { + exists: true, + type: 'f', + realPath: '/project/package.json', + data: 'file data', + } as const; + const getOrComputeFileData = jest.fn< + [string, ?Observations], + FileDataResult, + >(() => result); + await plugin.initialize({files, pluginState: null, getOrComputeFileData}); + const observations = { + existence: new Set(), + content: new Set(), + }; - expect(plugin.processFile('/project/package.json')).toBe('file data'); - expect(processFile).toHaveBeenCalledWith('/project/package.json'); + expect( + plugin.getOrComputeFileData('/project/package.json', observations), + ).toBe(result); + expect(getOrComputeFileData).toHaveBeenCalledWith( + '/project/package.json', + observations, + ); }); }); diff --git a/packages/metro-file-map/src/plugins/dependencies/__tests__/DependencyPlugin-test.js b/packages/metro-file-map/src/plugins/dependencies/__tests__/DependencyPlugin-test.js index a1205cdf2a..a2194994f5 100644 --- a/packages/metro-file-map/src/plugins/dependencies/__tests__/DependencyPlugin-test.js +++ b/packages/metro-file-map/src/plugins/dependencies/__tests__/DependencyPlugin-test.js @@ -14,13 +14,11 @@ import path from 'node:path'; describe('DependencyPlugin', () => { let plugin: DependencyPlugin; - let mockFiles; + let mockGetOrComputeFileData; beforeEach(() => { jest.resetModules(); - mockFiles = { - lookup: jest.fn(), - }; + mockGetOrComputeFileData = jest.fn(); }); describe('constructor', () => { @@ -234,64 +232,77 @@ describe('DependencyPlugin', () => { }); test('returns null for non-existent file', async () => { - mockFiles.lookup.mockReturnValue({ + mockGetOrComputeFileData.mockReturnValue({ exists: false, }); - // $FlowFixMe[unclear-type] - await plugin.initialize({files: mockFiles} as any); + await plugin.initialize({ + getOrComputeFileData: mockGetOrComputeFileData, + // $FlowFixMe[unclear-type] - Only the init options the plugin reads + } as any); expect(plugin.getDependencies('nonexistent.js')).toBeNull(); }); test('returns null for directory', async () => { - mockFiles.lookup.mockReturnValue({ + mockGetOrComputeFileData.mockReturnValue({ exists: true, type: 'd', }); - // $FlowFixMe[unclear-type] - await plugin.initialize({files: mockFiles} as any); + await plugin.initialize({ + getOrComputeFileData: mockGetOrComputeFileData, + // $FlowFixMe[unclear-type] - Only the init options the plugin reads + } as any); expect(plugin.getDependencies('src')).toBeNull(); }); test('returns dependencies from plugin data', async () => { - mockFiles.lookup.mockReturnValue({ + mockGetOrComputeFileData.mockReturnValue({ exists: true, type: 'f', - pluginData: ['React', 'lodash', './utils'], + realPath: 'src/index.js', + data: ['React', 'lodash', './utils'], }); - // $FlowFixMe[unclear-type] - await plugin.initialize({files: mockFiles} as any); + await plugin.initialize({ + getOrComputeFileData: mockGetOrComputeFileData, + // $FlowFixMe[unclear-type] - Only the init options the plugin reads + } as any); const deps = plugin.getDependencies('src/Component.js'); expect(deps).toEqual(['React', 'lodash', './utils']); }); test('returns [] when file exists but plugin data is not set', async () => { - mockFiles.lookup.mockReturnValue({ + mockGetOrComputeFileData.mockReturnValue({ exists: true, type: 'f', - pluginData: null, + realPath: 'src/index.js', + data: null, }); - // $FlowFixMe[unclear-type] - await plugin.initialize({files: mockFiles} as any); + await plugin.initialize({ + getOrComputeFileData: mockGetOrComputeFileData, + // $FlowFixMe[unclear-type] - Only the init options the plugin reads + } as any); expect(plugin.getDependencies('src/index.js')).toEqual([]); }); test('handles empty dependencies array', async () => { - mockFiles.lookup.mockReturnValue({ + mockGetOrComputeFileData.mockReturnValue({ exists: true, type: 'f', - pluginData: [], + realPath: 'src/index.js', + data: [], }); - // $FlowFixMe[unclear-type] - await plugin.initialize({files: mockFiles} as any); + await plugin.initialize({ + getOrComputeFileData: mockGetOrComputeFileData, + // $FlowFixMe[unclear-type] - Only the init options the plugin reads + } as any); expect(plugin.getDependencies('src/index.js')).toEqual([]); }); diff --git a/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js b/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js index 59adc7c55d..12a6bb8038 100644 --- a/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js +++ b/packages/metro-file-map/src/plugins/haste/__tests__/HastePlugin-test.js @@ -82,7 +82,7 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { ]), lookup: jest.fn(), }, - processFile: () => { + getOrComputeFileData: () => { throw new Error('Not implemented'); }, pluginState: null, @@ -110,7 +110,7 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { fileIterator: jest.fn().mockReturnValue(INITIAL_FILES), lookup: jest.fn(), }, - processFile: () => { + getOrComputeFileData: () => { throw new Error('Not implemented'); }, pluginState: null, @@ -145,7 +145,7 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { fileIterator: jest.fn().mockReturnValue(INITIAL_FILES), lookup: jest.fn(), }, - processFile: () => { + getOrComputeFileData: () => { throw new Error('Not implemented'); }, pluginState: null, @@ -181,37 +181,37 @@ describe.each([['win32'], ['posix']])('HastePlugin on %s', platform => { describe('getModuleNameByPath', () => { let hasteMap: HasteMapType; - let lookup; + let getOrComputeFileData; beforeEach(async () => { hasteMap = new HasteMap(opts); - lookup = jest.fn().mockReturnValue(null); + getOrComputeFileData = jest.fn().mockReturnValue(null); await hasteMap.initialize({ files: { fileIterator: jest.fn().mockReturnValue(INITIAL_FILES), - lookup, - }, - processFile: () => { - throw new Error('Not implemented'); + lookup: jest.fn(), }, + getOrComputeFileData, pluginState: null, }); }); test('returns the correct module name', () => { - lookup.mockImplementation( + getOrComputeFileData.mockImplementation( filePath => ({ [p('/root/Foo.js')]: { exists: true, type: 'f', - pluginData: 'Foo' as ?string, + realPath: p('/root/Foo.js'), + data: 'Foo' as ?string, }, [p('/root/not-haste.js')]: { exists: true, type: 'f', - pluginData: null as ?string, + realPath: p('/root/not-haste.js'), + data: null as ?string, }, })[filePath] ?? {exists: false}, ); diff --git a/packages/metro-file-map/src/plugins/mocks/__tests__/MockPlugin-test.js b/packages/metro-file-map/src/plugins/mocks/__tests__/MockPlugin-test.js index 4a9cbfead5..94316bb49d 100644 --- a/packages/metro-file-map/src/plugins/mocks/__tests__/MockPlugin-test.js +++ b/packages/metro-file-map/src/plugins/mocks/__tests__/MockPlugin-test.js @@ -136,7 +136,7 @@ Duplicate manual mock found for \`foo\`: throw new Error('should not be used'); }, }, - processFile: () => { + getOrComputeFileData: () => { throw new Error('Not implemented'); }, pluginState: { From fb4cdaf5b340d8df6fcf0e5a8035fbf287892d6c Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:13:00 +0100 Subject: [PATCH 2/3] Address review: hoist lazy check, clarify existence/content overlap Compute whether a plugin is lazy once per plugin at initialization, rather than on every miss in getOrComputeFileData. A non-lazy plugin misses on every read of a file its filter excludes. Correct the TreeFS comment that said a path belongs in only one of existence and content, and document on Observations.content that a path may be in both without changing what invalidates the result. --- packages/metro-file-map/src/flow-types.js | 4 +++- packages/metro-file-map/src/index.js | 11 ++++++----- packages/metro-file-map/src/lib/TreeFS.js | 4 ++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index 8b5ad5739c..fe46106a9b 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -510,7 +510,9 @@ export interface Observations { * Files whose content was observed (including traversed symlinks, whose * content is their target) and directories whose immediate entries were * enumerated. Invalidated by modification, as well as by addition or - * removal. + * removal, so a path here may also be in `existence` (eg, after a `lookup` + * and a content read of the same file) without changing what invalidates + * the result. */ readonly content: Set; } diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index eeb5ea6fe3..9640878818 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -455,8 +455,9 @@ export default class FileMap extends EventEmitter { fileSystem, }), Promise.all( - plugins.map(({plugin, dataIdx}) => - plugin.initialize({ + plugins.map(({plugin, dataIdx}) => { + const isLazy = plugin.getWorker()?.lazy === true; + return plugin.initialize({ files: { lookup: mixedPath => { const result = fileSystem.lookup(mixedPath); @@ -495,7 +496,7 @@ export default class FileMap extends EventEmitter { // `undefined` means no worker has run on this file. // eslint-disable-next-line lint/strictly-null if (data === undefined) { - if (plugin.getWorker()?.lazy === true) { + if (isLazy) { data = this.#fileProcessor.processFileForPlugin( result.realPath, result.metadata, @@ -519,8 +520,8 @@ export default class FileMap extends EventEmitter { data, }; }, - }), - ), + }); + }), ), ]); diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index 01b5b8caf6..dcd9690523 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -317,8 +317,8 @@ export default class TreeFS implements MutableFileSystem { } const {canonicalPath, node} = result; if (observations) { - // `content` is invalidated by everything `existence` is, so a path - // belongs in only one of them. + // `content` is invalidated by everything `existence` is, so a path in + // `content` need not also be in `existence`. (observeContent && !isDirectory(node) ? observations.content : observations.existence From 03eac5fe862165e3bdc62bff96fb5c9e631f5213 Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:20:10 +0100 Subject: [PATCH 3/3] Record content reads in getOrComputeFileData, drop TreeFS.lookupContent TreeFS.lookup now returns the canonicalPath it found, and getOrComputeFileData adds it to observations.content after a plain lookup. This removes lookupContent, leaving TreeFS with one lookup method. A file read is now recorded in existence as well as content, which is redundant but doesn't change what invalidates the result. --- .../src/__tests__/index-test.js | 8 +++- packages/metro-file-map/src/flow-types.js | 4 ++ packages/metro-file-map/src/index.js | 8 ++-- packages/metro-file-map/src/lib/TreeFS.js | 28 ++---------- .../src/lib/__tests__/TreeFS-test.js | 43 ++----------------- 5 files changed, 20 insertions(+), 71 deletions(-) diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index 483c1a6629..1dd061c432 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -445,7 +445,7 @@ describe('FileMap', () => { expect(FileMap.H).toBe(require('../constants').default); }); - test('getOrComputeFileData records a file found as content, and anything else as existence', async () => { + test('getOrComputeFileData records a file found as content, as well as what lookup records', async () => { const {dependencyPlugin} = await buildNewFileMap(); if (dependencyPlugin == null) { throw new Error('Expected a DependencyPlugin'); @@ -473,7 +473,11 @@ describe('FileMap', () => { expect(observations).toEqual({ content: new Set([path.join('fruits', 'Banana.js')]), - existence: new Set(['fruits', path.join('fruits', 'Missing.js')]), + existence: new Set([ + path.join('fruits', 'Banana.js'), + 'fruits', + path.join('fruits', 'Missing.js'), + ]), }); }); diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index fe46106a9b..cfb4f35338 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -471,6 +471,8 @@ export type LookupResult = } | { exists: true, + // The directory's path relative to the root, with symlinks resolved. + canonicalPath: CanonicalPath, // The real, normal, absolute path of the directory. realPath: string, // Currently lookup always follows symlinks, so can only return @@ -479,6 +481,8 @@ export type LookupResult = } | { exists: true, + // The file's path relative to the root, with symlinks resolved. + canonicalPath: CanonicalPath, // The real, normal, absolute path of the file. realPath: string, // Currently lookup always follows symlinks, so can only return diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index 9640878818..2f1c5da828 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -482,16 +482,16 @@ export default class FileMap extends EventEmitter { 'metro-file-map: Plugin "%s" has no worker to process files with', plugin.name, ); - const result = fileSystem.lookupContent( - mixedPath, - observations, - ); + const result = fileSystem.lookup(mixedPath, observations); if (!result.exists) { return {exists: false}; } if (result.type === 'd') { return {exists: true, type: 'd'}; } + // Plugin data is derived from what the file holds, so it + // changes when the file is modified. + observations?.content.add(result.canonicalPath); let data = result.metadata[dataIdx]; // `undefined` means no worker has run on this file. // eslint-disable-next-line lint/strictly-null diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index dcd9690523..7a00ed2e67 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -287,23 +287,6 @@ export default class TreeFS implements MutableFileSystem { } lookup(mixedPath: Path, observations?: ?Observations): LookupResult { - return this.#lookup(mixedPath, observations, false); - } - - /** - * As `lookup`, for a caller that goes on to use what a regular file found - * holds. That file is recorded in `observations.content` rather than - * `existence`, since a modification can change the caller's answer. - */ - lookupContent(mixedPath: Path, observations?: ?Observations): LookupResult { - return this.#lookup(mixedPath, observations, true); - } - - #lookup( - mixedPath: Path, - observations: ?Observations, - observeContent: boolean, - ): LookupResult { const normalPath = this.#normalizePath(mixedPath); const result = this.#lookupByNormalPath(normalPath, { observations, @@ -317,16 +300,11 @@ export default class TreeFS implements MutableFileSystem { } const {canonicalPath, node} = result; if (observations) { - // `content` is invalidated by everything `existence` is, so a path in - // `content` need not also be in `existence`. - (observeContent && !isDirectory(node) - ? observations.content - : observations.existence - ).add(canonicalPath); + observations.existence.add(canonicalPath); } const realPath = this.#pathUtils.normalToAbsolute(canonicalPath); if (isDirectory(node)) { - return {exists: true, realPath, type: 'd'}; + return {exists: true, canonicalPath, realPath, type: 'd'}; } invariant( isRegularFile(node), @@ -334,7 +312,7 @@ export default class TreeFS implements MutableFileSystem { mixedPath, canonicalPath, ); - return {exists: true, realPath, type: 'f', metadata: node}; + return {exists: true, canonicalPath, realPath, type: 'f', metadata: node}; } getAllFiles(): Array { 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 a2a55bce5e..596efdedcd 100644 --- a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js +++ b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js @@ -167,6 +167,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { const observations = emptyObservations(); expect(tfs.lookup(givenPath, observations)).toEqual({ exists: true, + canonicalPath: canonical(expectedRealPath), realPath: expectedRealPath, type: 'f', metadata: expect.any(Array), @@ -214,6 +215,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { expect(tfs.lookup(givenPath, observations)).toEqual({ exists: true, type: 'd', + canonicalPath: canonical(expectedRealPath), realPath: expectedRealPath, }); expect(observations).toEqual({ @@ -223,46 +225,6 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { }, ); - describe('lookupContent', () => { - test('records a file found in content, not existence', () => { - const observations = emptyObservations(); - expect( - tfs.lookupContent(p('/project/bar.js'), observations), - ).toMatchObject({exists: true, type: 'f'}); - expect(observations).toEqual({ - content: new Set([canonical(p('/project/bar.js'))]), - existence: new Set(), - }); - }); - - test('records the real path of a file found through a symlink, with the link', () => { - const observations = emptyObservations(); - expect( - tfs.lookupContent(p('/project/foo/link-to-bar.js'), observations), - ).toMatchObject({exists: true, realPath: p('/project/bar.js')}); - expect(observations).toEqual({ - content: new Set([ - canonical(p('/project/foo/link-to-bar.js')), - canonical(p('/project/bar.js')), - ]), - existence: new Set(), - }); - }); - - test('records a directory found, or a missing path, in existence', () => { - const observations = emptyObservations(); - tfs.lookupContent(p('/project/foo'), observations); - tfs.lookupContent(p('/project/foo/missing.js'), observations); - expect(observations).toEqual({ - content: new Set(), - existence: new Set([ - canonical(p('/project/foo')), - canonical(p('/project/foo/missing.js')), - ]), - }); - }); - }); - test('traversing the same symlink multiple times does not imply a cycle', () => { expect( tfs.lookup(p('/project/foo/owndir/owndir/another.js')), @@ -329,6 +291,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { const observations = emptyObservations(); expect(tfs.lookup(mixedPath, observations)).toEqual({ exists: true, + canonicalPath: canonical(expectedRealPath), realPath: expectedRealPath, type: 'f', metadata: expect.any(Array),