Fix watcher path normalisation with Windows and Watchman

Summary:
Fixes an issue with the Watchman watcher backend on Windows where emitted posix-style paths were not properly normalised, leading to inconsistencies with other watchers and broken tests.

Adds the necessary normalisation and reenables tests.

Changelog: Internal

(This is luckily corrected downstream by the use of `path.join` on `relativePath`, which performs normalisation, so there was no observable bug here, it was just fragile.)

Reviewed By: huntie

Differential Revision: D67700720

jest.config.js

@@ -28,9 +28,7 @@ if (process.platform === 'win32') {
       'packages/metro-config/src/__tests__/loadConfig-test.js',
       'packages/metro-symbolicate/src/__tests__/symbolicate-test.js',
       'packages/metro-file-map/src/__tests__/index-test.js',
-      'packages/metro-file-map/src/watchers/__tests__/WatchmanWatcher-test.js',
       'packages/metro-file-map/src/crawlers/__tests__/node-test.js',
-      'packages/metro-file-map/src/watchers/__tests__/integration-test.js',
 
       // resolveModulePath failed
       'packages/metro-cache/src/stores/__tests__/FileStore-test.js',

packages/metro-file-map/src/watchers/WatchmanWatcher.js

@@ -20,6 +20,7 @@ import type {
   WatchmanWatchResponse,
 } from 'fb-watchman';
 
+import normalizePathSeparatorsToSystem from '../lib/normalizePathSeparatorsToSystem';
 import {AbstractWatcher} from './AbstractWatcher';
 import * as common from './common';
 import RecrawlWarning from './RecrawlWarning';
@@ -105,9 +106,13 @@ export default class WatchmanWatcher extends AbstractWatcher {
 
       handleWarning(resp);
 
+      // NB: Watchman outputs posix-separated paths even on Windows, convert
+      // them to system-native separators.
       self.watchProjectInfo = {
-        relativePath: resp.relative_path ? resp.relative_path : '',
-        root: resp.watch,
+        relativePath: resp.relative_path
+          ? normalizePathSeparatorsToSystem(resp.relative_path)
+          : '',
+        root: normalizePathSeparatorsToSystem(resp.watch),
       };
 
       self.client.command(['clock', getWatchRoot()], onClock);
@@ -231,14 +236,18 @@ export default class WatchmanWatcher extends AbstractWatcher {
     );
 
     const {
-      name: relativePath,
+      name: relativePosixPath,
       new: isNew = false,
       exists = false,
       type,
       mtime_ms,
       size,
     } = changeDescriptor;
 
+    // Watchman emits posix-separated paths on Windows, which is inconsistent
+    // with other watchers. Normalize to system-native separators.
+    const relativePath = normalizePathSeparatorsToSystem(relativePosixPath);
+
     debug(
       'Handling change to: %s (new: %s, exists: %s, type: %s)',
       relativePath,

packages/metro-file-map/src/watchers/__tests__/WatchmanWatcher-test.js

@@ -28,13 +28,27 @@ const cmdCallback = <T>(err: ?Error, result: Partial<T>): void => {
   mockClient.command.mock.lastCall[1](err, result);
 };
 
+// Convenience function to write paths with posix separators but convert them
+// to system separators, and prepend a mock drive letter to absolute paths on
+// Windows.
+const p: string => string = filePath =>
+  process.platform === 'win32'
+    ? filePath.replaceAll('/', '\\').replace(/^\\/, 'C:\\')
+    : filePath;
+
+// 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.
+// This should be used for mocking Watchman *output*.
+const wp: string => string = filePath =>
+  process.platform === 'win32' ? filePath.replace(/^\//, 'C:/') : filePath;
+
 jest.mock('fb-watchman', () => ({
   Client: jest.fn().mockImplementation(() => mockClient),
 }));
 
 describe('WatchmanWatcher', () => {
   test('initializes with watch-project, clock, subscribe', () => {
-    const watchmanWatcher = new WatchmanWatcher('/project/subdir/js', {
+    const watchmanWatcher = new WatchmanWatcher(p('/project/subdir/js'), {
       dot: true,
       ignored: null,
       globs: ['**/*.js'],
@@ -46,16 +60,16 @@ describe('WatchmanWatcher', () => {
       .finally(() => (isSettled = true));
 
     expect(mockClient.command).toHaveBeenCalledWith(
-      ['watch-project', '/project/subdir/js'],
+      ['watch-project', p('/project/subdir/js')],
       expect.any(Function),
     );
     cmdCallback<WatchmanWatchResponse>(null, {
-      watch: '/project',
-      relative_path: 'subdir/js',
+      watch: wp('/project'),
+      relative_path: wp('subdir/js'),
     });
 
     expect(mockClient.command).toHaveBeenCalledWith(
-      ['clock', '/project'],
+      ['clock', p('/project')],
       expect.any(Function),
     );
     cmdCallback<WatchmanClockResponse>(null, {
@@ -65,12 +79,12 @@ describe('WatchmanWatcher', () => {
     expect(mockClient.command).toHaveBeenCalledWith(
       [
         'subscribe',
-        '/project',
+        p('/project'),
         watchmanWatcher.subscriptionName,
         {
           defer: ['busy'],
           fields: ['name', 'exists', 'new', 'type', 'size', 'mtime_ms'],
-          relative_root: 'subdir/js',
+          relative_root: p('subdir/js'),
           since: 'c:1629095304.251049',
         },
       ],
@@ -91,15 +105,20 @@ describe('WatchmanWatcher', () => {
     let startPromise: Promise<void>;
 
     beforeEach(async () => {
-      watchmanWatcher = new WatchmanWatcher('/project/subdir/js', {
+      watchmanWatcher = new WatchmanWatcher(p('/project/subdir/js'), {
         dot: true,
         ignored: null,
         globs: ['**/*.js'],
         watchmanDeferStates: ['busy'],
       });
       startPromise = watchmanWatcher.startWatching();
-      cmdCallback<WatchmanWatchResponse>(null, {});
-      cmdCallback<WatchmanClockResponse>(null, {});
+      cmdCallback<WatchmanWatchResponse>(null, {
+        watch: wp('/project'),
+        relative_path: wp('subdir/js'),
+      });
+      cmdCallback<WatchmanClockResponse>(null, {
+        clock: 'c:123',
+      });
     });
 
     afterEach(() => {