From 6894a32cbf32c576b58561b1491e129f8b6c2a1f Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 19 Feb 2018 16:12:20 +0000 Subject: [PATCH 1/2] fix(watchman): Overhauls how Watchman crawler works fixing Windows issues **Summary** Watchman crawler was ignoring the `relative_path` field in the response of a `watch-project` call, requiring it to match watch roots with the actual project roots afterwards. Not only this was inefficient, it was also faulty due to the naive `.startsWith()` check in `isDescendant()`. This was causing issues both with Windows file paths (#5553) and after that with case-insensitive file systems where the names from Watchman did not match the casing of the passed roots. This patch replaces all that logic by taking the `relative_path` field into account and does some consolidation along with using `async`/`await` instead of promises. **Test plan** Run the updated test suite on all platforms and make sure it passes. I've also verified this on some internal Windows repos by manually patching the built module and making sure there are no warnings regarding duplicated haste names due to incorrect crawling of project root siblings. --- .../src/crawlers/__tests__/watchman.test.js | 368 ++++++++++-------- .../jest-haste-map/src/crawlers/watchman.js | 221 +++++------ 2 files changed, 326 insertions(+), 263 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/__tests__/watchman.test.js b/packages/jest-haste-map/src/crawlers/__tests__/watchman.test.js index e4bddd166a01..9b52c70e21d9 100644 --- a/packages/jest-haste-map/src/crawlers/__tests__/watchman.test.js +++ b/packages/jest-haste-map/src/crawlers/__tests__/watchman.test.js @@ -13,74 +13,82 @@ const path = require('path'); jest.mock('fb-watchman', () => { const normalizePathSep = require('../../lib/normalize_path_sep').default; const Client = jest.fn(); - Client.prototype.command = jest.fn((args, callback) => { - if (args[0] === 'watch-project') { - setImmediate(() => callback(null, {watch: args[1].replace(/\\/g, '/')})); - } else if (args[0] === 'query') { - setImmediate(() => - callback(null, mockResponse[normalizePathSep(args[1])]), - ); - } - }); + Client.prototype.command = jest.fn((args, callback) => + setImmediate(() => { + const response = mockResponse[args[0]][normalizePathSep(args[1])]; + callback(null, response.next ? response.next().value : response); + }), + ); Client.prototype.on = jest.fn(); Client.prototype.end = jest.fn(); return {Client}; }); +const forcePOSIXPaths = path => path.replace(/\\/g, '/'); const pearMatcher = path => /pear/.test(path); +let watchman; let watchmanCrawl; let mockResponse; let mockFiles; -const FRUITS = path.sep + 'fruits'; -const VEGETABLES = path.sep + 'vegetables'; +const ROOT_MOCK = path.sep === '/' ? '/root-mock' : 'M:\\root-mock'; +const FRUITS = `${ROOT_MOCK}${path.sep}fruits`; +const VEGETABLES = `${ROOT_MOCK}${path.sep}vegetables`; const ROOTS = [FRUITS, VEGETABLES]; const BANANA = path.join(FRUITS, 'banana.js'); const STRAWBERRY = path.join(FRUITS, 'strawberry.js'); const KIWI = path.join(FRUITS, 'kiwi.js'); const TOMATO = path.join(FRUITS, 'tomato.js'); const MELON = path.join(VEGETABLES, 'melon.json'); +const WATCH_PROJECT_MOCK = { + [FRUITS]: { + relative_path: 'fruits', + watch: forcePOSIXPaths(ROOT_MOCK), + }, + [VEGETABLES]: { + relative_path: 'vegetables', + watch: forcePOSIXPaths(ROOT_MOCK), + }, +}; describe('watchman watch', () => { beforeEach(() => { watchmanCrawl = require('../watchman'); + watchman = require('fb-watchman'); + mockResponse = { - [FRUITS]: { - clock: 'c:fake-clock:1', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 30}, - name: 'strawberry.js', - }, - { - exists: true, - mtime_ms: {toNumber: () => 31}, - name: 'tomato.js', - }, - { - exists: true, - mtime_ms: {toNumber: () => 32}, - name: 'pear.js', - }, - ], - is_fresh_instance: true, - version: '4.5.0', - }, - [VEGETABLES]: { - clock: 'c:fake-clock:2', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 33}, - name: 'melon.json', - }, - ], - is_fresh_instance: true, - version: '4.5.0', + query: { + [ROOT_MOCK]: { + clock: 'c:fake-clock:1', + files: [ + { + exists: true, + mtime_ms: {toNumber: () => 30}, + name: 'fruits/strawberry.js', + }, + { + exists: true, + mtime_ms: {toNumber: () => 31}, + name: 'fruits/tomato.js', + }, + { + exists: true, + mtime_ms: {toNumber: () => 32}, + name: 'fruits/pear.js', + }, + { + exists: true, + mtime_ms: {toNumber: () => 33}, + name: 'vegetables/melon.json', + }, + ], + is_fresh_instance: true, + version: '4.5.0', + }, }, + 'watch-project': WATCH_PROJECT_MOCK, }; mockFiles = Object.assign(Object.create(null), { @@ -90,14 +98,11 @@ describe('watchman watch', () => { }); }); - it('returns a list of all files when there are no clocks', () => { - const watchman = require('fb-watchman'); - const normalizePathSep = require('../../lib/normalize_path_sep').default; - - const originalPathRelative = path.relative; - const ROOT_MOCK = path.sep === '/' ? '/root-mock' : 'M:\\root-mock'; - path.relative = jest.fn(from => normalizePathSep(ROOT_MOCK + from)); + afterEach(() => { + watchman.Client.mock.instances[0].command.mockClear(); + }); + test('returns a list of all files when there are no clocks', () => { return watchmanCrawl({ data: { clocks: Object.create(null), @@ -117,73 +122,57 @@ describe('watchman watch', () => { expect(calls[0][0][0]).toEqual('watch-project'); expect(calls[1][0][0]).toEqual('watch-project'); - // Calls 2 and 3 are queries - const query1 = calls[2][0]; - const query2 = calls[3][0]; - expect(query1[0]).toEqual('query'); - expect(query2[0]).toEqual('query'); + // Call 2 is the query + const query = calls[2][0]; + expect(query[0]).toEqual('query'); - expect(query1[2].expression).toEqual([ - 'allof', - ['type', 'f'], - ['anyof', ['suffix', 'js'], ['suffix', 'json']], - ['anyof', ['dirname', ROOT_MOCK + FRUITS]], - ]); - expect(query2[2].expression).toEqual([ + expect(query[2].expression).toEqual([ 'allof', ['type', 'f'], ['anyof', ['suffix', 'js'], ['suffix', 'json']], - ['anyof', ['dirname', ROOT_MOCK + VEGETABLES]], + ['anyof', ['dirname', 'fruits'], ['dirname', 'vegetables']], ]); - expect(query1[2].fields).toEqual(['name', 'exists', 'mtime_ms']); - expect(query2[2].fields).toEqual(['name', 'exists', 'mtime_ms']); + expect(query[2].fields).toEqual(['name', 'exists', 'mtime_ms']); - expect(query1[2].suffix).toEqual(['js', 'json']); - expect(query2[2].suffix).toEqual(['js', 'json']); + expect(query[2].suffix).toEqual(['js', 'json']); expect(data.clocks).toEqual({ - [FRUITS]: 'c:fake-clock:1', - [VEGETABLES]: 'c:fake-clock:2', + [ROOT_MOCK]: 'c:fake-clock:1', }); expect(data.files).toEqual(mockFiles); - path.relative = originalPathRelative; - expect(client.end).toBeCalled(); }); }); - it('updates the file object when the clock is given', () => { + test('updates the file object when the clock is given', () => { mockResponse = { - [FRUITS]: { - clock: 'c:fake-clock:3', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 42}, - name: 'kiwi.js', - }, - { - exists: false, - mtime_ms: null, - name: 'tomato.js', - }, - ], - is_fresh_instance: false, - version: '4.5.0', - }, - [VEGETABLES]: { - clock: 'c:fake-clock:4', - files: [], - version: '4.5.0', + query: { + [ROOT_MOCK]: { + clock: 'c:fake-clock:2', + files: [ + { + exists: true, + mtime_ms: {toNumber: () => 42}, + name: 'fruits/kiwi.js', + }, + { + exists: false, + mtime_ms: null, + name: 'fruits/tomato.js', + }, + ], + is_fresh_instance: false, + version: '4.5.0', + }, }, + 'watch-project': WATCH_PROJECT_MOCK, }; const clocks = Object.assign(Object.create(null), { - [FRUITS]: 'c:fake-clock:1', - [VEGETABLES]: 'c:fake-clock:2', + [ROOT_MOCK]: 'c:fake-clock:1', }); return watchmanCrawl({ @@ -199,8 +188,7 @@ describe('watchman watch', () => { expect(data.files).toBe(mockFiles); expect(data.clocks).toEqual({ - [FRUITS]: 'c:fake-clock:3', - [VEGETABLES]: 'c:fake-clock:4', + [ROOT_MOCK]: 'c:fake-clock:2', }); expect(data.files).toEqual({ @@ -211,44 +199,40 @@ describe('watchman watch', () => { }); }); - it('resets the file object when watchman is restarted', () => { + test('resets the file object when watchman is restarted', () => { mockResponse = { - [FRUITS]: { - clock: 'c:fake-clock:5', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 42}, - name: 'kiwi.js', - }, - { - exists: true, - mtime_ms: {toNumber: () => 41}, - name: 'banana.js', - }, - { - exists: true, - mtime_ms: {toNumber: () => 31}, - name: 'tomato.js', - }, - ], - is_fresh_instance: true, - version: '4.5.0', - }, - [VEGETABLES]: { - clock: 'c:fake-clock:6', - files: [], - is_fresh_instance: true, - version: '4.5.0', + query: { + [ROOT_MOCK]: { + clock: 'c:fake-clock:3', + files: [ + { + exists: true, + mtime_ms: {toNumber: () => 42}, + name: 'fruits/kiwi.js', + }, + { + exists: true, + mtime_ms: {toNumber: () => 41}, + name: 'fruits/banana.js', + }, + { + exists: true, + mtime_ms: {toNumber: () => 31}, + name: 'fruits/tomato.js', + }, + ], + is_fresh_instance: true, + version: '4.5.0', + }, }, + 'watch-project': WATCH_PROJECT_MOCK, }; const mockMetadata = ['Banana', 41, 1, ['Raspberry']]; mockFiles[BANANA] = mockMetadata; const clocks = Object.assign(Object.create(null), { - [FRUITS]: 'c:fake-clock:1', - [VEGETABLES]: 'c:fake-clock:2', + [ROOT_MOCK]: 'c:fake-clock:1', }); return watchmanCrawl({ @@ -264,8 +248,7 @@ describe('watchman watch', () => { expect(data.files).not.toBe(mockFiles); expect(data.clocks).toEqual({ - [FRUITS]: 'c:fake-clock:5', - [VEGETABLES]: 'c:fake-clock:6', + [ROOT_MOCK]: 'c:fake-clock:3', }); // /fruits/strawberry.js was removed from the file list. @@ -283,31 +266,41 @@ describe('watchman watch', () => { }); }); - it('properly resets the file map when only one watcher is reset', () => { + test('properly resets the file map when only one watcher is reset', () => { mockResponse = { - [FRUITS]: { - clock: 'c:fake-clock:3', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 42}, - name: 'kiwi.js', - }, - ], - is_fresh_instance: false, - version: '4.5.0', + query: { + [FRUITS]: { + clock: 'c:fake-clock:3', + files: [ + { + exists: true, + mtime_ms: {toNumber: () => 42}, + name: 'kiwi.js', + }, + ], + is_fresh_instance: false, + version: '4.5.0', + }, + [VEGETABLES]: { + clock: 'c:fake-clock:4', + files: [ + { + exists: true, + mtime_ms: {toNumber: () => 33}, + name: 'melon.json', + }, + ], + is_fresh_instance: true, + version: '4.5.0', + }, }, - [VEGETABLES]: { - clock: 'c:fake-clock:4', - files: [ - { - exists: true, - mtime_ms: {toNumber: () => 33}, - name: 'melon.json', - }, - ], - is_fresh_instance: true, - version: '4.5.0', + 'watch-project': { + [FRUITS]: { + watch: forcePOSIXPaths(FRUITS), + }, + [VEGETABLES]: { + watch: forcePOSIXPaths(VEGETABLES), + }, }, }; @@ -336,4 +329,73 @@ describe('watchman watch', () => { }); }); }); + + test('does not add directory filters to query when watching a ROOT', () => { + mockResponse = { + query: { + [ROOT_MOCK]: { + clock: 'c:fake-clock:1', + files: [], + is_fresh_instance: false, + version: '4.5.0', + }, + }, + 'watch-project': { + [FRUITS]: { + relative_path: 'fruits', + watch: forcePOSIXPaths(ROOT_MOCK), + }, + [ROOT_MOCK]: { + watch: forcePOSIXPaths(ROOT_MOCK), + }, + [VEGETABLES]: { + relative_path: 'vegetables', + watch: forcePOSIXPaths(ROOT_MOCK), + }, + }, + }; + + return watchmanCrawl({ + data: { + clocks: Object.create(null), + files: Object.create(null), + }, + extensions: ['js', 'json'], + ignore: pearMatcher, + roots: [...ROOTS, ROOT_MOCK], + }).then(data => { + const client = watchman.Client.mock.instances[0]; + const calls = client.command.mock.calls; + + expect(client.on).toBeCalled(); + expect(client.on).toBeCalledWith('error', expect.any(Function)); + + // First 3 calls are for ['watch-project'] + expect(calls[0][0][0]).toEqual('watch-project'); + expect(calls[1][0][0]).toEqual('watch-project'); + expect(calls[2][0][0]).toEqual('watch-project'); + + // Call 4 is the query + const query = calls[3][0]; + expect(query[0]).toEqual('query'); + + expect(query[2].expression).toEqual([ + 'allof', + ['type', 'f'], + ['anyof', ['suffix', 'js'], ['suffix', 'json']], + ]); + + expect(query[2].fields).toEqual(['name', 'exists', 'mtime_ms']); + + expect(query[2].suffix).toEqual(['js', 'json']); + + expect(data.clocks).toEqual({ + [ROOT_MOCK]: 'c:fake-clock:1', + }); + + expect(data.files).toEqual({}); + + expect(client.end).toBeCalled(); + }); + }); }); diff --git a/packages/jest-haste-map/src/crawlers/watchman.js b/packages/jest-haste-map/src/crawlers/watchman.js index 2944695b7415..3e69981c766c 100644 --- a/packages/jest-haste-map/src/crawlers/watchman.js +++ b/packages/jest-haste-map/src/crawlers/watchman.js @@ -18,126 +18,127 @@ import H from '../constants'; const watchmanURL = 'https://facebook.github.io/watchman/docs/troubleshooting.html'; -function isDescendant(root: string, child: string): boolean { - return child.startsWith(root); +class WatchmanError extends Error { + constructor(error) { + super( + `Watchman error: ${error.message.trim()}. Make sure watchman ` + + `is running for this project. See ${watchmanURL}.`, + ); + const stack = error.stack; + if (stack) { + this.stack = stack; + } + } } -function WatchmanError(error: Error): Error { - return new Error( - `Watchman error: ${error.message.trim()}. Make sure watchman ` + - `is running for this project. See ${watchmanURL}.`, - ); -} - -module.exports = function watchmanCrawl( +module.exports = async function watchmanCrawl( options: CrawlerOptions, ): Promise { const {data, extensions, ignore, roots} = options; - // Watchman always returns POSIX style paths so use posixRoots - // instead of roots to avoid on-the-fly checks inside the loop. - const posixRoots = - path.sep === '/' - ? Array.from(roots) - : roots.map(root => root.replace(/\\/g, '/')); + const defaultWatchExpression = [ + 'allof', + ['type', 'f'], + ['anyof'].concat(extensions.map(extension => ['suffix', extension])), + ]; + const client = new watchman.Client(); + let clientError; + client.on('error', error => (clientError = error)); - return new Promise((resolve, reject) => { - const client = new watchman.Client(); - client.on('error', error => reject(error)); + const cmd = (...args) => + new Promise((resolve, reject) => + client.command( + args, + (error, result) => (error ? reject(error) : resolve(result)), + ), + ); - const cmd = (...args) => - new Promise((resolve, reject) => { - client.command(args, (error, result) => { - if (error) { - reject(error); - } else { - resolve(result); - } - }); - }); + const clocks = data.clocks; + let files = data.files; - const clocks = data.clocks; - let files = data.files; + try { + const watchmanRoots = new Map(); + for (const root of roots) { + const response = await cmd('watch-project', root); + const existing = watchmanRoots.get(response.watch); + // A root can only be filtered if it was never seen with a relative_path before + const canBeFiltered = !existing || existing.length > 0; - return Promise.all(roots.map(root => cmd('watch-project', root))) - .then(responses => - Promise.all( - Array.from(new Set(responses.map(response => response.watch))).map( - root => { - // Build an expression to filter the output by the relevant roots. - const dirExpr = (['anyof']: Array>); - posixRoots.forEach(subRoot => { - if (isDescendant(root, subRoot)) { - dirExpr.push(['dirname', path.relative(root, subRoot)]); - } - }); - const expression = [ - 'allof', - ['type', 'f'], - ['anyof'].concat( - extensions.map(extension => ['suffix', extension]), - ), - ]; - if (dirExpr.length > 1) { - expression.push(dirExpr); - } - const fields = ['name', 'exists', 'mtime_ms']; + if (canBeFiltered) { + if (response.relative_path) { + watchmanRoots.set( + response.watch, + (existing || []).concat(response.relative_path), + ); + } else { + // Make the filter directories an empty array to signal that this root + // was already seen and needs to be watched for all files/directories + watchmanRoots.set(response.watch, []); + } + } + } - const query = clocks[root] - ? // Use the `since` generator if we have a clock available - {expression, fields, since: clocks[root]} - : // Otherwise use the `suffix` generator - {expression, fields, suffix: extensions}; - return cmd('query', root, query).then(response => ({ - response, - root, - })); - }, - ), - ).then(pairs => { - // Reset the file map if watchman was restarted and sends us a list of - // files. - if (pairs.some(pair => pair.response.is_fresh_instance)) { - files = Object.create(null); - } + let shouldReset = false; + const watchmanFileResults = new Map(); + for (const [root, directoryFilters] of watchmanRoots) { + const expression = Array.from(defaultWatchExpression); + if (directoryFilters.length > 0) { + expression.push([ + 'anyof', + ...directoryFilters.map(dir => ['dirname', dir]), + ]); + } + const fields = ['name', 'exists', 'mtime_ms']; - pairs.forEach(pair => { - const root = normalizePathSep(pair.root); - const response = pair.response; - if ('warning' in response) { - console.warn('watchman warning: ', response.warning); - } + const query = clocks[root] + ? // Use the `since` generator if we have a clock available + {expression, fields, since: clocks[root]} + : // Otherwise use the `suffix` generator + {expression, fields, suffix: extensions}; + + const response = await cmd('query', root, query); + shouldReset = shouldReset || response.is_fresh_instance; + watchmanFileResults.set(root, response); + } + + // Reset the file map if watchman was restarted and sends us a list of files. + if (shouldReset) { + files = Object.create(null); + } + + for (const [watchRoot, response] of watchmanFileResults) { + const fsRoot = normalizePathSep(watchRoot); + if ('warning' in response) { + console.warn('watchman warning: ', response.warning); + } + clocks[fsRoot] = response.clock; + for (const fileData of response.files) { + const name = fsRoot + path.sep + normalizePathSep(fileData.name); + if (!fileData.exists) { + delete files[name]; + } else if (!ignore(name)) { + const mtime = + typeof fileData.mtime_ms === 'number' + ? fileData.mtime_ms + : fileData.mtime_ms.toNumber(); + const isOld = data.files[name] && data.files[name][H.MTIME] === mtime; + if (isOld) { + files[name] = data.files[name]; + } else { + // See ../constants.js + files[name] = ['', mtime, 0, []]; + } + } + } + } + } catch (error) { + throw new WatchmanError(error); + } finally { + client.end(); + } - clocks[root] = response.clock; - response.files.forEach(fileData => { - const name = root + path.sep + normalizePathSep(fileData.name); - if (!fileData.exists) { - delete files[name]; - } else if (!ignore(name)) { - const mtime = - typeof fileData.mtime_ms === 'number' - ? fileData.mtime_ms - : fileData.mtime_ms.toNumber(); - const isNew = - !data.files[name] || data.files[name][H.MTIME] !== mtime; - if (isNew) { - // See ../constants.js - files[name] = ['', mtime, 0, []]; - } else { - files[name] = data.files[name]; - } - } - }); - }); - }), - ) - .then(() => { - client.end(); - data.files = files; - resolve(data); - }) - .catch(error => { - client.end(); - reject(WatchmanError(error)); - }); - }); + if (clientError) { + throw new WatchmanError(clientError); + } + data.files = files; + return data; }; From 69607ddfcb38d148cef9c46352012cd481df0b1f Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 19 Feb 2018 17:02:35 +0000 Subject: [PATCH 2/2] Better error handling --- .../jest-haste-map/src/crawlers/watchman.js | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/jest-haste-map/src/crawlers/watchman.js b/packages/jest-haste-map/src/crawlers/watchman.js index 3e69981c766c..945f68edc9bc 100644 --- a/packages/jest-haste-map/src/crawlers/watchman.js +++ b/packages/jest-haste-map/src/crawlers/watchman.js @@ -18,17 +18,11 @@ import H from '../constants'; const watchmanURL = 'https://facebook.github.io/watchman/docs/troubleshooting.html'; -class WatchmanError extends Error { - constructor(error) { - super( - `Watchman error: ${error.message.trim()}. Make sure watchman ` + - `is running for this project. See ${watchmanURL}.`, - ); - const stack = error.stack; - if (stack) { - this.stack = stack; - } - } +function WatchmanError(error: Error): Error { + error.message = + `Watchman error: ${error.message.trim()}. Make sure watchman ` + + `is running for this project. See ${watchmanURL}.`; + return error; } module.exports = async function watchmanCrawl( @@ -131,13 +125,13 @@ module.exports = async function watchmanCrawl( } } } catch (error) { - throw new WatchmanError(error); + throw WatchmanError(error); } finally { client.end(); } if (clientError) { - throw new WatchmanError(clientError); + throw WatchmanError(clientError); } data.files = files; return data;