metro-file-map: Do not miss files changed while the file map builds

packages/metro-file-map/src/Watcher.js

@@ -17,11 +17,16 @@ import type {
   PerfLogger,
   WatcherBackend,
   WatcherBackendChangeEvent,
+  WatchmanClocks,
+  WatchmanClockSpec,
 } from './flow-types';
 import type {WatcherOptions as WatcherBackendOptions} from './watchers/common';
 
 import nodeCrawl from './crawlers/node';
 import watchmanCrawl from './crawlers/watchman';
+import normalizePathSeparatorsToPosix from './lib/normalizePathSeparatorsToPosix';
+import normalizePathSeparatorsToSystem from './lib/normalizePathSeparatorsToSystem';
+import {RootPathUtils} from './lib/RootPathUtils';
 import {TOUCH_EVENT} from './watchers/common';
 import FallbackWatcher from './watchers/FallbackWatcher';
 import NativeWatcher from './watchers/NativeWatcher';
@@ -82,24 +87,43 @@ export type HealthCheckResult =
 export class Watcher extends EventEmitter {
   #activeWatcher: ?string;
   #backends: ReadonlyArray<WatcherBackend> = [];
+  // Events received by started backends before watch() is called, to be
+  // replayed once it is.
+  #bufferedEvents: Array<WatcherBackendChangeEvent> = [];
   readonly #instanceId: number;
   #nextHealthCheckId: number = 0;
+  #onChange: ?(change: WatcherBackendChangeEvent) => void;
   readonly #options: WatcherOptions;
   readonly #pendingHealthChecks: Map<
     /* basename */ string,
     /* resolve */ () => void,
   > = new Map();
+  // Whether watch() uses a Watchman backend. False if the Watchman crawl fell
+  // back to the node crawler, which returns no clock to subscribe `since`.
+  #useWatchmanWatcher: boolean;
 
   constructor(options: WatcherOptions) {
     super();
     this.#options = options;
     this.#instanceId = nextInstanceId++;
+    this.#useWatchmanWatcher = options.useWatchman;
   }
 
   async crawl(): Promise<CrawlResult> {
     this.#options.perfLogger?.point('crawl_start');
     const options = this.#options;
 
+    // Non-Watchman backends have no clock to subscribe `since`, so they must
+    // already be watching before the crawl begins, and their events buffered,
+    // for changes made while crawling not to be missed. Watchman backends are
+    // started by watch(), which subscribes `since` the crawl's clock to cover
+    // the same window. If the Watchman crawl falls back to the node crawler,
+    // there is no clock, so #crawl() starts non-Watchman backends before the
+    // node crawl instead.
+    if (!options.useWatchman) {
+      await this.#startBackendsBeforeCrawl();
+    }
+
     const result = await this.#crawl({
       previousState: options.previousState,
       roots: options.roots,
@@ -110,6 +134,17 @@ export class Watcher extends EventEmitter {
     return result;
   }
 
+  async #startBackendsBeforeCrawl(): Promise<void> {
+    if (!this.#options.watch) {
+      return;
+    }
+    await this.#startBackends();
+    // end() may have closed us before the backends were set - don't leak them.
+    if (this.#options.abortSignal.aborted) {
+      await this.close();
+    }
+  }
+
   async recrawl(
     subpath: string,
     currentFileSystem: CrawlerOptions['previousState']['fileSystem'],
@@ -178,6 +213,10 @@ export class Watcher extends EventEmitter {
           '  ' +
           firstError.toString(),
       );
+      // Watch with a non-Watchman backend, started before the node crawl so
+      // that changes made during it are not missed.
+      this.#useWatchmanWatcher = false;
+      await this.#startBackendsBeforeCrawl();
       try {
         delta = await nodeCrawl(crawlerOptions);
       } catch (retryError) {
@@ -199,8 +238,28 @@ export class Watcher extends EventEmitter {
     return delta;
   }
 
-  async watch(onChange: (change: WatcherBackendChangeEvent) => void) {
-    const {extensions, ignorePatternForWatch, useWatchman} = this.#options;
+  async watch(
+    onChange: (change: WatcherBackendChangeEvent) => void,
+    clocks?: WatchmanClocks,
+  ): Promise<void> {
+    this.#onChange = onChange;
+
+    // Replay anything a backend started before the crawl reported in the
+    // meantime - these are handled exactly like live events.
+    const bufferedEvents = this.#bufferedEvents;
+    this.#bufferedEvents = [];
+    for (const event of bufferedEvents) {
+      onChange(event);
+    }
+
+    if (this.#backends.length === 0) {
+      await this.#startBackends(clocks);
+    }
+  }
+
+  async #startBackends(clocks?: WatchmanClocks): Promise<void> {
+    const {extensions, ignorePatternForWatch} = this.#options;
+    const useWatchman = this.#useWatchmanWatcher;
 
     // WatchmanWatcher > NativeWatcher > FallbackWatcher
     const WatcherImpl = useWatchman
@@ -219,6 +278,22 @@ export class Watcher extends EventEmitter {
     this.#options.perfLogger?.annotate({string: {watcher}});
     this.#activeWatcher = watcher;
 
+    // Crawl clocks are keyed by path relative to the project root, but a
+    // backend only learns its Watchman watch root after `watch-project`, so
+    // hand each the whole map keyed by absolute watch root to look up.
+    let watchmanClocks: ?Map<string, WatchmanClockSpec>;
+    if (useWatchman && clocks != null) {
+      const pathUtils = new RootPathUtils(this.#options.rootDir);
+      watchmanClocks = new Map(
+        [...clocks].map(([root, clockSpec]) => [
+          normalizePathSeparatorsToPosix(
+            pathUtils.normalToAbsolute(normalizePathSeparatorsToSystem(root)),
+          ),
+          clockSpec,
+        ]),
+      );
+    }
+
     const createWatcherBackend = (root: Path): Promise<WatcherBackend> => {
       const watcherOptions: WatcherBackendOptions = {
         included: {
@@ -231,6 +306,7 @@ export class Watcher extends EventEmitter {
         },
         ignored: ignorePatternForWatch,
         watchmanDeferStates: this.#options.watchmanDeferStates,
+        clocks: watchmanClocks,
       };
       const watcher: WatcherBackend = new WatcherImpl(root, watcherOptions);
 
@@ -262,7 +338,12 @@ export class Watcher extends EventEmitter {
             );
             return;
           }
-          onChange(change);
+          const onChange = this.#onChange;
+          if (onChange != null) {
+            onChange(change);
+          } else {
+            this.#bufferedEvents.push(change);
+          }
         });
         await watcher.startWatching();
         clearTimeout(rejectTimeout);

packages/metro-file-map/src/__tests__/Watcher-test.js

@@ -15,6 +15,7 @@ import type {
   CrawlResult,
   FileData,
   FileMetadata,
+  WatcherBackendChangeEvent,
 } from '../flow-types';
 
 import TreeFS from '../lib/TreeFS';
@@ -32,6 +33,44 @@ jest.mock(
   () => (options: CrawlerOptions) => mockNodeCrawl(options),
 );
 
+type MockBackend = {
+  kind: 'native' | 'watchman',
+  root: string,
+  emit: (event: WatcherBackendChangeEvent) => void,
+};
+const mockStartedBackends: Array<MockBackend> = [];
+function mockBackendClass(kind: MockBackend['kind']) {
+  return class {
+    #root: string;
+    #listener: ?(event: WatcherBackendChangeEvent) => void;
+    static isSupported(): boolean {
+      return true;
+    }
+    constructor(root: string) {
+      this.#root = root;
+    }
+    onFileEvent(listener: (event: WatcherBackendChangeEvent) => void) {
+      this.#listener = listener;
+    }
+    async startWatching() {
+      mockStartedBackends.push({
+        kind,
+        root: this.#root,
+        emit: event => this.#listener?.(event),
+      });
+    }
+    async stopWatching() {}
+  };
+}
+jest.mock('../watchers/NativeWatcher', () => ({
+  __esModule: true,
+  default: mockBackendClass('native'),
+}));
+jest.mock('../watchers/WatchmanWatcher', () => ({
+  __esModule: true,
+  default: mockBackendClass('watchman'),
+}));
+
 const rootDir = path.join(path.sep, 'project');
 
 function crawlResult(canonicalPaths: ReadonlyArray<string>): CrawlResult {
@@ -55,7 +94,11 @@ const rejectWith =
     throw new Error(message);
   };
 
-function createWatcher(useWatchman: boolean, crawl?: ?Crawler): Watcher {
+function createWatcher(
+  useWatchman: boolean,
+  crawl?: ?Crawler,
+  watch: boolean = false,
+): Watcher {
   return new Watcher({
     abortSignal: new AbortController().signal,
     computeSha1: false,
@@ -79,12 +122,13 @@ function createWatcher(useWatchman: boolean, crawl?: ?Crawler): Watcher {
     rootDir,
     roots: [rootDir],
     useWatchman,
-    watch: false,
+    watch,
     watchmanDeferStates: [],
   });
 }
 
 beforeEach(() => {
+  mockStartedBackends.length = 0;
   mockWatchmanCrawl.mockReset();
   mockNodeCrawl.mockReset();
   jest.spyOn(console, 'warn').mockImplementation(() => {});
@@ -169,6 +213,45 @@ describe('Watcher crawl fallback', () => {
   });
 });
 
+describe('Watcher backend choice in watch mode', () => {
+  const event: WatcherBackendChangeEvent = {
+    event: 'touch',
+    relativePath: 'index.js',
+    root: rootDir,
+    metadata: {modifiedTime: 1, size: 1, type: 'f'},
+  };
+
+  test('a successful Watchman crawl starts a Watchman backend only on watch()', async () => {
+    mockWatchmanCrawl.mockImplementation(async () => crawlResult([]));
+    const watcher = createWatcher(true, null, true);
+
+    await watcher.crawl();
+    expect(mockStartedBackends).toEqual([]);
+
+    await watcher.watch(() => {});
+    expect(mockStartedBackends.map(b => b.kind)).toEqual(['watchman']);
+  });
+
+  test('a failing Watchman crawl starts a native backend before the node crawl, and watch() replays its events', async () => {
+    mockWatchmanCrawl.mockImplementation(rejectWith('watchman unavailable'));
+    mockNodeCrawl.mockImplementation(async () => {
+      // The backend is already watching while the node crawler runs.
+      expect(mockStartedBackends.map(b => b.kind)).toEqual(['native']);
+      mockStartedBackends[0].emit(event);
+      return crawlResult([]);
+    });
+    const watcher = createWatcher(true, null, true);
+
+    await watcher.crawl();
+    const onChange = jest.fn<[WatcherBackendChangeEvent], void>();
+    await watcher.watch(onChange);
+
+    expect(onChange.mock.calls).toEqual([[event]]);
+    // No Watchman backend is started - the node crawl has no clock for it.
+    expect(mockStartedBackends.map(b => b.kind)).toEqual(['native']);
+  });
+});
+
 describe('Watcher crawl option', () => {
   const stubCrawler =
     (canonicalPaths: ReadonlyArray<string>): Crawler =>

packages/metro-file-map/src/__tests__/index-test.js

@@ -22,13 +22,13 @@ import type {
   HasteMap,
   MockMap,
   ReadonlyFileSystemChanges,
-  WatcherBackendOptions,
   WorkerSetupArgs,
 } from '../flow-types';
 import type {DependencyPlugin} from '../index';
 import type {default as FileMapT} from '../index';
 import type {HasteMapOptions} from '../plugins/HastePlugin';
 import type {MockMapOptions} from '../plugins/MockPlugin';
+import type {WatcherOptions} from '../watchers/common';
 import typeof WorkerModule from '../worker';
 
 import {AbstractWatcher} from '../watchers/AbstractWatcher';
@@ -52,6 +52,11 @@ const toPosixMessage = (message: string): string =>
     ? message.replaceAll('C:\\', '/').replaceAll('\\', '/')
     : message;
 
+// Format a posix path as a Watchman-native path on the current platform, i.e.,
+// on Windows, drive letters on absolute paths, but posix-style separators.
+const wp = (filePath: string): string =>
+  process.platform === 'win32' ? filePath.replace(/^\//, 'C:/') : filePath;
+
 function mockHashContents(contents: string | Buffer) {
   return crypto.createHash('sha1').update(contents).digest('hex');
 }
@@ -140,8 +145,11 @@ jest.mock('../crawlers/watchman', () => ({
 }));
 
 class MockWatcher extends AbstractWatcher {
-  constructor(root: string, opts: WatcherBackendOptions) {
+  readonly opts: WatcherOptions;
+
+  constructor(root: string, opts: WatcherOptions) {
     super(root, opts);
+    this.opts = opts;
     mockEmitters[root] = this;
   }
 
@@ -1769,6 +1777,56 @@ describe('FileMap', () => {
       size: 55,
     };
 
+    fm_it(
+      'passes the clocks from the crawl to the watcher, keyed by watch root',
+      async () => {
+        for (const root of [p('/project/fruits'), p('/project/vegetables')]) {
+          expect(mockEmitters[root].opts.clocks).toEqual(
+            new Map([
+              [wp('/project/fruits'), 'c:fake-clock:1'],
+              [wp('/project/vegetables'), 'c:fake-clock:2'],
+              [wp('/project/video'), 'c:fake-clock:3'],
+            ]),
+          );
+        }
+      },
+    );
+
+    test('does not miss a change made while the file map is being built', async () => {
+      mockNodeCrawler.mockImplementationOnce(async () => {
+        // The watcher is already running at this point, so this reports a
+        // change that the crawl itself cannot see.
+        mockFs[p('/project/fruits/Tomato.js')] = '// Tomato!';
+        mockEmitters[p('/project/fruits')]?.emitFileEvent({
+          event: 'touch',
+          relativePath: 'Tomato.js',
+          metadata: MOCK_CHANGE_FILE,
+        });
+        return {changedFiles: new Map(), removedFiles: new Set()};
+      });
+
+      let fileMap: ?FileMapT = null;
+      let fileSystem: ?FileSystem = null;
+      try {
+        ({fileMap, fileSystem} = await buildNewFileMap({
+          useWatchman: false,
+          watch: true,
+        }));
+        const changeEvent = await Promise.race([
+          waitForItToChange(fileMap),
+          new Promise<null>(resolve => setTimeout(() => resolve(null), 1000)),
+        ]);
+        expect(fileSystem?.exists(p('/project/fruits/Tomato.js'))).toBe(true);
+        expect(
+          [...(changeEvent?.changes.addedFiles ?? [])].map(
+            ([filePath]) => filePath,
+          ),
+        ).toContain(path.join('fruits', 'Tomato.js'));
+      } finally {
+        await fileMap?.end();
+      }
+    });
+
     fm_it(
       'handles several change events at once',
       async ({fileMap, hasteMap}) => {

packages/metro-file-map/src/index.js

@@ -1090,7 +1090,7 @@ export default class FileMap extends EventEmitter {
       this.#watcher != null,
       'Expected #watcher to have been initialised by build()',
     );
-    await this.#watcher.watch(onChange);
+    await this.#watcher.watch(onChange, clocks);
 
     if (this.#options.healthCheck.enabled) {
       const performHealthCheck = () => {