File map crawl retries, prefer async/await (#1668)

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<Path>,
-};
-
 type WatcherOptions = {
   abortSignal: AbortSignal,
   computeSha1: boolean,
@@ -113,50 +106,45 @@ export class Watcher extends EventEmitter {
       roots: options.roots,
     };
 
-    const retry = (error: Error): Promise<CrawlResult> => {
-      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<CrawlResult>(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<CrawlResult>(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) {

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<CanonicalPath>,
-  changedFiles: FileData,
-}> {
+export default async function nodeCrawl(
+  options: CrawlerOptions,
+): Promise<CrawlResult> {
   const {
     console,
     previousState,

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<CanonicalPath>,
-  clocks: WatchmanClocks,
-}> {
+}: CrawlerOptions): Promise<CrawlResult> {
   abortSignal?.throwIfAborted();
 
   const client = new watchman.Client();

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<Path>,
+      clocks: WatchmanClocks,
+    }
+  | {
+      changedFiles: FileData,
+      removedFiles: Set<Path>,
+    };
+
 export type DependencyExtractor = {
   extract: (
     content: string,
@@ -439,7 +450,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,

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<CanonicalPath>,
-    changedFiles: FileData,
-    clocks?: WatchmanClocks,
-  }> {
+  ): Promise<CrawlResult> {
     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<void> {

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;
   }
 
   /**

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(

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<Path>;
-};
 type WatcherOptions = {
   abortSignal: AbortSignal;
   computeSha1: boolean;

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<CanonicalPath>; changedFiles: FileData}>;
+declare function nodeCrawl(options: CrawlerOptions): Promise<CrawlResult>;
 export default nodeCrawl;