From f854f8b720962d729264eb6f385c02492e79a779 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 16 Mar 2026 19:49:50 -0700 Subject: [PATCH 1/2] File map: fix handling of a non-empty directory being moved Summary: Now that we have a change listener on `TreeFS`, we can delete multiple entries during a `remove()` call without losing information about what was removed. Use this to implement the natural behaviour of deleting a non-empty directory, where we now recursively delete contents instead of throwing. This fixes a macOS, non-Watchman bug in the handling of a non-empty directory being moved outside a watch root. Previously, we would only receive the a directory "rename" event from the OS, which we would ignore, and Metro would still "see" the directory contents at their old location. Changelog: ``` - **[Fix]**: macOS + non-Watchman: correctly detect contents as removed when a watched directory is moved. ``` Differential Revision: D96202588 --- packages/metro-file-map/src/flow-types.js | 2 +- packages/metro-file-map/src/lib/TreeFS.js | 13 +++---- .../src/lib/__tests__/TreeFS-test.js | 36 ++++++++++++++++--- packages/metro-file-map/types/flow-types.d.ts | 7 ++-- packages/metro-file-map/types/lib/TreeFS.d.ts | 7 ++-- 5 files changed, 43 insertions(+), 22 deletions(-) diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index 66868b28f6..a658ecd661 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -439,7 +439,7 @@ export interface ReadonlyFileSystemChanges<+T = FileMetadata> { } export interface MutableFileSystem extends FileSystem { - remove(filePath: Path, listener?: FileSystemListener): ?FileMetadata; + remove(filePath: Path, listener?: FileSystemListener): void; addOrModify( filePath: Path, fileMetadata: FileMetadata, diff --git a/packages/metro-file-map/src/lib/TreeFS.js b/packages/metro-file-map/src/lib/TreeFS.js index a5f27039bb..5c2b97bb4d 100644 --- a/packages/metro-file-map/src/lib/TreeFS.js +++ b/packages/metro-file-map/src/lib/TreeFS.js @@ -462,18 +462,20 @@ export default class TreeFS implements MutableFileSystem { } } - remove(mixedPath: Path, changeListener?: FileSystemListener): ?FileMetadata { + remove(mixedPath: Path, changeListener?: FileSystemListener): void { const normalPath = this.#normalizePath(mixedPath); const result = this.#lookupByNormalPath(normalPath, {followLeaf: false}); if (!result.exists) { - return null; + return; } const {parentNode, canonicalPath, node} = result; if (isDirectory(node) && node.size > 0) { - throw new Error( - `TreeFS: remove called on a non-empty directory: ${mixedPath}`, - ); + for (const basename of node.keys()) { + this.remove(canonicalPath + path.sep + basename, changeListener); + } + // Removing the last file will delete this directory + return; } if (parentNode != null) { if (changeListener != null) { @@ -493,7 +495,6 @@ export default class TreeFS implements MutableFileSystem { this.remove(path.dirname(canonicalPath), changeListener); } } - return isDirectory(node) ? null : 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 265a1a32aa..febed5e5ad 100644 --- a/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js +++ b/packages/metro-file-map/src/lib/__tests__/TreeFS-test.js @@ -776,16 +776,16 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { [p('./bar.js')], [p('./link-to-foo/.././bar.js')], [p('/outside/../project/./bar.js')], - ])('removes a file and returns its metadata: %s', mixedPath => { + ])('removes a file: %s', mixedPath => { expect(tfs.linkStats(mixedPath)).not.toBeNull(); - expect(Array.isArray(tfs.remove(mixedPath))).toBe(true); + tfs.remove(mixedPath); expect(tfs.linkStats(mixedPath)).toBeNull(); }); test('deletes a symlink, not its target', () => { expect(tfs.linkStats(p('foo/link-to-bar.js'))).not.toBeNull(); expect(tfs.linkStats(p('bar.js'))).not.toBeNull(); - expect(Array.isArray(tfs.remove(p('foo/link-to-bar.js')))).toBe(true); + tfs.remove(p('foo/link-to-bar.js')); expect(tfs.linkStats(p('foo/link-to-bar.js'))).toBeNull(); expect(tfs.linkStats(p('bar.js'))).not.toBeNull(); }); @@ -806,6 +806,21 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { expect(tfs.lookup(p('node_modules')).exists).toBe(false); }); + test('deleting a non-empty directory also removes its empty parent', () => { + // node_modules/pkg is the only child of node_modules + expect(tfs.lookup(p('node_modules/pkg')).exists).toBe(true); + expect(tfs.lookup(p('node_modules')).exists).toBe(true); + tfs.remove(p('node_modules/pkg')); + // Expect the directory and its contents to be deleted + expect(tfs.lookup(p('node_modules/pkg/a.js')).exists).toBe(false); + expect(tfs.lookup(p('node_modules/pkg/package.json')).exists).toBe( + false, + ); + expect(tfs.lookup(p('node_modules/pkg')).exists).toBe(false); + // And its parent, which is now empty + expect(tfs.lookup(p('node_modules')).exists).toBe(false); + }); + test('deleting all files leaves an empty map', () => { for (const {canonicalPath} of tfs.metadataIterator({ includeSymlinks: true, @@ -817,8 +832,8 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { expect(tfs.lookup(p('foo')).exists).toBe(false); }); - test('returns null for a non-existent file', () => { - expect(tfs.remove('notexists.js')).toBeNull(); + test('no-op for a non-existent file', () => { + expect(() => tfs.remove('notexists.js')).not.toThrow(); }); }); }); @@ -1109,6 +1124,17 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => { ]); }); + describe('remove with listener', () => { + test('tracks removed files and directories when deleting a non-empty directory', () => { + simpleTfs.remove(p('dir'), listener); + + expect(logChange.mock.calls).toEqual([ + ['fileRemoved', p('dir/nested.js'), [456, 0, 0, '', 0]], + ['directoryRemoved', p('dir')], + ]); + }); + }); + describe('symlinks with listener', () => { test('tracks added files when adding a symlink', () => { simpleTfs.addOrModify( diff --git a/packages/metro-file-map/types/flow-types.d.ts b/packages/metro-file-map/types/flow-types.d.ts index 2beee194f9..fbf780291e 100644 --- a/packages/metro-file-map/types/flow-types.d.ts +++ b/packages/metro-file-map/types/flow-types.d.ts @@ -6,7 +6,7 @@ * * @noformat * @oncall react_native - * @generated SignedSource<<20f9e0b81ccdc0bd3698417297709836>> + * @generated SignedSource<<8c5030c48241b474468d59538c2e34e3>> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/flow-types.js @@ -351,10 +351,7 @@ export interface ReadonlyFileSystemChanges { readonly removedFiles: Iterable>; } export interface MutableFileSystem extends FileSystem { - remove( - filePath: Path, - listener?: FileSystemListener, - ): null | undefined | FileMetadata; + remove(filePath: Path, listener?: FileSystemListener): void; addOrModify( filePath: Path, fileMetadata: FileMetadata, diff --git a/packages/metro-file-map/types/lib/TreeFS.d.ts b/packages/metro-file-map/types/lib/TreeFS.d.ts index 5f4c3be5b9..1b5a47efce 100644 --- a/packages/metro-file-map/types/lib/TreeFS.d.ts +++ b/packages/metro-file-map/types/lib/TreeFS.d.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. * * @noformat - * @generated SignedSource<> + * @generated SignedSource<<420bedf62829a002f71fef34d51a0b11>> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/lib/TreeFS.js @@ -132,10 +132,7 @@ declare class TreeFS implements MutableFileSystem { addedOrModifiedFiles: FileData, changeListener?: FileSystemListener, ): void; - remove( - mixedPath: Path, - changeListener?: FileSystemListener, - ): null | undefined | FileMetadata; + remove(mixedPath: Path, changeListener?: FileSystemListener): void; /** * Given a start path (which need not exist), a subpath and type, and * optionally a 'breakOnSegment', performs the following: From 0a53e4de93827178d60ad1858ebde64c60d10222 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Tue, 17 Mar 2026 09:28:49 -0700 Subject: [PATCH 2/2] File map crawl retries, prefer async/await (#1668) Summary: Pull Request resolved: https://github.com/facebook/metro/pull/1668 Modernise and cleanup this logic a little bit to reduce the noise of an upcoming change. Changelog: Internal Reviewed By: vzaidman Differential Revision: D96136859 --- packages/metro-file-map/src/Watcher.js | 86 ++++++++----------- .../metro-file-map/src/crawlers/node/index.js | 9 +- .../src/crawlers/watchman/index.js | 8 +- packages/metro-file-map/src/flow-types.js | 11 +++ packages/metro-file-map/src/index.js | 14 ++- packages/metro-file-map/types/Watcher.d.ts | 11 +-- .../types/crawlers/node/index.d.ts | 8 +- .../types/crawlers/watchman/index.d.ts | 17 ++-- packages/metro-file-map/types/flow-types.d.ts | 5 +- 9 files changed, 73 insertions(+), 96 deletions(-) diff --git a/packages/metro-file-map/src/Watcher.js b/packages/metro-file-map/src/Watcher.js index a48ea91a5d..50e96cbf67 100644 --- a/packages/metro-file-map/src/Watcher.js +++ b/packages/metro-file-map/src/Watcher.js @@ -11,12 +11,11 @@ import type { Console, CrawlerOptions, - FileData, + CrawlResult, Path, PerfLogger, WatcherBackend, WatcherBackendChangeEvent, - WatchmanClocks, } from './flow-types'; import type {WatcherOptions as WatcherBackendOptions} from './watchers/common'; @@ -37,12 +36,6 @@ const debug = require('debug')('Metro:Watcher'); const MAX_WAIT_TIME = 240000; -type CrawlResult = { - changedFiles: FileData, - clocks?: WatchmanClocks, - removedFiles: Set, -}; - type WatcherOptions = { abortSignal: AbortSignal, computeSha1: boolean, @@ -113,50 +106,45 @@ export class Watcher extends EventEmitter { roots: options.roots, }; - const retry = (error: Error): Promise => { - if (crawl === watchmanCrawl) { - crawler = 'node'; - options.console.warn( - 'metro-file-map: Watchman crawl failed. Retrying once with node ' + - 'crawler.\n' + - " Usually this happens when watchman isn't running. Create an " + - "empty `.watchmanconfig` file in your project's root folder or " + - 'initialize a git or hg repository in your project.\n' + - ' ' + - error.toString(), - ); - // $FlowFixMe[incompatible-type] Found when updating Promise type definition - return nodeCrawl(crawlerOptions).catch(e => { - throw new Error( - 'Crawler retry failed:\n' + - ` Original error: ${error.message}\n` + - ` Retry error: ${e.message}\n`, - ); - }); - } - - throw error; - }; - - const logEnd = (delta: CrawlResult): CrawlResult => { - debug( - 'Crawler "%s" returned %d added/modified, %d removed, %d clock(s).', - crawler, - delta.changedFiles.size, - delta.removedFiles.size, - delta.clocks?.size ?? 0, - ); - this.#options.perfLogger?.point('crawl_end'); - return delta; - }; - debug('Beginning crawl with "%s".', crawler); + + let delta: CrawlResult; try { - // $FlowFixMe[incompatible-type] Found when updating Promise type definition - return crawl(crawlerOptions).catch(retry).then(logEnd); - } catch (error) { - return retry(error).then(logEnd); + delta = await crawl(crawlerOptions); + } catch (firstError) { + if (crawl !== watchmanCrawl) { + throw firstError; + } + crawler = 'node'; + options.console.warn( + 'metro-file-map: Watchman crawl failed. Retrying once with node ' + + 'crawler.\n' + + " Usually this happens when watchman isn't running. Create an " + + "empty `.watchmanconfig` file in your project's root folder or " + + 'initialize a git or hg repository in your project.\n' + + ' ' + + firstError.toString(), + ); + try { + delta = await nodeCrawl(crawlerOptions); + } catch (retryError) { + throw new Error( + 'Crawler retry failed:\n' + + ` Original error: ${firstError.message}\n` + + ` Retry error: ${retryError.message}\n`, + ); + } } + + debug( + 'Crawler "%s" returned %d added/modified, %d removed, %d clock(s).', + crawler, + delta.changedFiles.size, + delta.removedFiles.size, + delta.clocks?.size ?? 0, + ); + this.#options.perfLogger?.point('crawl_end'); + return delta; } async watch(onChange: (change: WatcherBackendChangeEvent) => void) { diff --git a/packages/metro-file-map/src/crawlers/node/index.js b/packages/metro-file-map/src/crawlers/node/index.js index 2c087ba1af..b43b73dd00 100644 --- a/packages/metro-file-map/src/crawlers/node/index.js +++ b/packages/metro-file-map/src/crawlers/node/index.js @@ -10,9 +10,9 @@ */ import type { - CanonicalPath, Console, CrawlerOptions, + CrawlResult, FileData, IgnoreMatcher, } from '../../flow-types'; @@ -170,10 +170,9 @@ function findNative( }); } -export default async function nodeCrawl(options: CrawlerOptions): Promise<{ - removedFiles: Set, - changedFiles: FileData, -}> { +export default async function nodeCrawl( + options: CrawlerOptions, +): Promise { const { console, previousState, diff --git a/packages/metro-file-map/src/crawlers/watchman/index.js b/packages/metro-file-map/src/crawlers/watchman/index.js index 4b011b6518..2e45eba592 100644 --- a/packages/metro-file-map/src/crawlers/watchman/index.js +++ b/packages/metro-file-map/src/crawlers/watchman/index.js @@ -13,10 +13,10 @@ import type {WatchmanClockSpec} from '../../flow-types'; import type { CanonicalPath, CrawlerOptions, + CrawlResult, FileData, FileMetadata, Path, - WatchmanClocks, } from '../../flow-types'; import type {WatchmanQueryResponse, WatchmanWatchResponse} from 'fb-watchman'; @@ -57,11 +57,7 @@ export default async function watchmanCrawl({ previousState, rootDir, roots, -}: CrawlerOptions): Promise<{ - changedFiles: FileData, - removedFiles: Set, - clocks: WatchmanClocks, -}> { +}: CrawlerOptions): Promise { abortSignal?.throwIfAborted(); const client = new watchman.Client(); diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index a658ecd661..585c18bc44 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -127,6 +127,17 @@ export type CrawlerOptions = { onStatus: (status: WatcherStatus) => void, }; +export type CrawlResult = + | { + changedFiles: FileData, + removedFiles: Set, + clocks: WatchmanClocks, + } + | { + changedFiles: FileData, + removedFiles: Set, + }; + export type DependencyExtractor = { extract: ( content: string, diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index 5c00465c36..0828a3cb1e 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -23,6 +23,7 @@ import type { ChangeEventMetadata, Console, CrawlerOptions, + CrawlResult, FileData, FileMapPlugin, FileMapPluginWorker, @@ -506,11 +507,7 @@ export default class FileMap extends EventEmitter { */ async #buildFileDelta( previousState: CrawlerOptions['previousState'], - ): Promise<{ - removedFiles: Set, - changedFiles: FileData, - clocks?: WatchmanClocks, - }> { + ): Promise { this.#startupPerfLogger?.point('buildFileDelta_start'); const { @@ -554,10 +551,9 @@ export default class FileMap extends EventEmitter { watcher.on('status', status => this.emit('status', status)); - return watcher.crawl().then(result => { - this.#startupPerfLogger?.point('buildFileDelta_end'); - return result; - }); + const result = await watcher.crawl(); + this.#startupPerfLogger?.point('buildFileDelta_end'); + return result; } #maybeReadLink(normalPath: Path, fileMetadata: FileMetadata): ?Promise { diff --git a/packages/metro-file-map/types/Watcher.d.ts b/packages/metro-file-map/types/Watcher.d.ts index 9e1e51ab74..1c59e4f886 100644 --- a/packages/metro-file-map/types/Watcher.d.ts +++ b/packages/metro-file-map/types/Watcher.d.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. * * @noformat - * @generated SignedSource<<7537b04fdc97fb54ebddaebf60605405>> + * @generated SignedSource<<296395484c53039955e7789570880079>> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/Watcher.js @@ -17,20 +17,13 @@ import type { Console, CrawlerOptions, - FileData, - Path, + CrawlResult, PerfLogger, WatcherBackendChangeEvent, - WatchmanClocks, } from './flow-types'; import EventEmitter from 'events'; -type CrawlResult = { - changedFiles: FileData; - clocks?: WatchmanClocks; - removedFiles: Set; -}; type WatcherOptions = { abortSignal: AbortSignal; computeSha1: boolean; diff --git a/packages/metro-file-map/types/crawlers/node/index.d.ts b/packages/metro-file-map/types/crawlers/node/index.d.ts index dde34b8c78..38ddc03ecf 100644 --- a/packages/metro-file-map/types/crawlers/node/index.d.ts +++ b/packages/metro-file-map/types/crawlers/node/index.d.ts @@ -6,7 +6,7 @@ * * @noformat * @oncall react_native - * @generated SignedSource<<8851cd12d3cd8bdda798362696c830a2>> + * @generated SignedSource<<27109494e4956802ba89ac6fd22aa277>> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/crawlers/node/index.js @@ -15,9 +15,7 @@ * yarn run build-ts-defs (OSS) */ -import type {CanonicalPath, CrawlerOptions, FileData} from '../../flow-types'; +import type {CrawlerOptions, CrawlResult} from '../../flow-types'; -declare function nodeCrawl( - options: CrawlerOptions, -): Promise<{removedFiles: Set; changedFiles: FileData}>; +declare function nodeCrawl(options: CrawlerOptions): Promise; export default nodeCrawl; diff --git a/packages/metro-file-map/types/crawlers/watchman/index.d.ts b/packages/metro-file-map/types/crawlers/watchman/index.d.ts index 29b79fc688..9952e52c5d 100644 --- a/packages/metro-file-map/types/crawlers/watchman/index.d.ts +++ b/packages/metro-file-map/types/crawlers/watchman/index.d.ts @@ -6,7 +6,7 @@ * * @noformat * @oncall react_native - * @generated SignedSource<<94c8a03429d06b694e26ca524fb9f17c>> + * @generated SignedSource<> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/crawlers/watchman/index.js @@ -15,16 +15,9 @@ * yarn run build-ts-defs (OSS) */ -import type { - CanonicalPath, - CrawlerOptions, - FileData, - WatchmanClocks, -} from '../../flow-types'; +import type {CrawlerOptions, CrawlResult} from '../../flow-types'; -declare function watchmanCrawl($$PARAM_0$$: CrawlerOptions): Promise<{ - changedFiles: FileData; - removedFiles: Set; - clocks: WatchmanClocks; -}>; +declare function watchmanCrawl( + $$PARAM_0$$: CrawlerOptions, +): Promise; export default watchmanCrawl; diff --git a/packages/metro-file-map/types/flow-types.d.ts b/packages/metro-file-map/types/flow-types.d.ts index fbf780291e..3b7264820d 100644 --- a/packages/metro-file-map/types/flow-types.d.ts +++ b/packages/metro-file-map/types/flow-types.d.ts @@ -6,7 +6,7 @@ * * @noformat * @oncall react_native - * @generated SignedSource<<8c5030c48241b474468d59538c2e34e3>> + * @generated SignedSource<<51ef2d9b353f0cec5e93976e27e3e2f8>> * * This file was translated from Flow by scripts/generateTypeScriptDefinitions.js * Original file: packages/metro-file-map/src/flow-types.js @@ -107,6 +107,9 @@ export type CrawlerOptions = { roots: ReadonlyArray; onStatus: (status: WatcherStatus) => void; }; +export type CrawlResult = + | {changedFiles: FileData; removedFiles: Set; clocks: WatchmanClocks} + | {changedFiles: FileData; removedFiles: Set}; export type DependencyExtractor = { extract: ( content: string,