Skip to content

Commit

Permalink
Fix watcher path normalisation with Windows and Watchman (#1423)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #1423

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

fbshipit-source-id: 08ae84c0473e9db1b4e1844210ba111fe25fbc92
  • Loading branch information
robhogan authored and facebook-github-bot committed Dec 31, 2024
1 parent 9518c0e commit dd3e56c
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 16 deletions.
2 changes: 0 additions & 2 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
15 changes: 12 additions & 3 deletions packages/metro-file-map/src/watchers/WatchmanWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand All @@ -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, {
Expand All @@ -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',
},
],
Expand All @@ -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(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ describe.each(Object.keys(WATCHERS))(
// If all tests are skipped, Jest will not run before/after hooks either.
const maybeTest = WATCHERS[watcherName] ? test : test.skip;
const maybeTestOn = (...platforms: $ReadOnlyArray<string>) =>
platforms.includes(os.platform()) ? test : test.skip;
platforms.includes(os.platform()) && WATCHERS[watcherName]
? test
: test.skip;

beforeAll(async () => {
watchRoot = await createTempWatchRoot(watcherName);
Expand Down

0 comments on commit dd3e56c

Please sign in to comment.