From ab86982fad83da457d949f01a301c589fabcb12e Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 13 Feb 2023 14:30:39 -0800 Subject: [PATCH] Watcher backends: Don't apply glob filter to symlink/directory change events Summary: Metro uses the `glob` filter of the Watcher backends to include: - `package.json` - Files with a watched extension - Health check files All of these only make sense to apply against regular files - all directory events are ignored downstream, and symlinks must be included because they may (at some point) point to a directory. Separately, we have an `ignorePattern` to exclude source control patterns and the user-provided `blockList`. It continues to make sense to apply `ignorePattern` to all events, and for some backends it helps avoid walking ignored subtrees unnecessarily. Currently, backends apply both the glob filter and ignore pattern to all files. This diff: - Modifies tests to more closely resemble the way Metro uses the backends - providing a glob filter to allowlist file extensions. - Adds a `type` parameter to `common.isFileIncluded` (-> `isIncluded`) so that glob patterns are only applied to known regular files. D43214089 follows by making similar changes downstream on the `Watcher`. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D43234543 fbshipit-source-id: f9f2289c74e7e0d4c119eee05e0ede76fe7230d4 --- .../src/watchers/FSEventsWatcher.js | 22 ++---- .../src/watchers/NodeWatcher.js | 78 +++++++++---------- .../src/watchers/WatchmanWatcher.js | 20 +++-- .../watchers/__tests__/integration-test.js | 17 ++-- .../metro-file-map/src/watchers/common.js | 12 ++- 5 files changed, 76 insertions(+), 73 deletions(-) diff --git a/packages/metro-file-map/src/watchers/FSEventsWatcher.js b/packages/metro-file-map/src/watchers/FSEventsWatcher.js index d5eeb91ab5..7e8746a5b2 100644 --- a/packages/metro-file-map/src/watchers/FSEventsWatcher.js +++ b/packages/metro-file-map/src/watchers/FSEventsWatcher.js @@ -20,10 +20,7 @@ import {promises as fsPromises} from 'fs'; import * as path from 'path'; // $FlowFixMe[untyped-import] - walker import walker from 'walker'; -import {typeFromStat} from './common'; - -// $FlowFixMe[untyped-import] - micromatch -const micromatch = require('micromatch'); +import {isIncluded, typeFromStat} from './common'; const debug = require('debug')('Metro:FSEventsWatcher'); @@ -159,20 +156,8 @@ export default class FSEventsWatcher extends EventEmitter { } } - _isFileIncluded(relativePath: string): boolean { - if (this.doIgnore(relativePath)) { - return false; - } - return this.glob.length - ? micromatch([relativePath], this.glob, {dot: this.dot}).length > 0 - : this.dot || micromatch([relativePath], '**/*').length > 0; - } - async _handleEvent(filepath: string) { const relativePath = path.relative(this.root, filepath); - if (!this._isFileIncluded(relativePath)) { - return; - } try { const stat = await fsPromises.lstat(filepath); @@ -182,6 +167,11 @@ export default class FSEventsWatcher extends EventEmitter { if (!type) { return; } + + if (!isIncluded(type, this.glob, this.dot, this.doIgnore, relativePath)) { + return; + } + const metadata: ChangeEventMetadata = { type, modifiedTime: stat.mtime.getTime(), diff --git a/packages/metro-file-map/src/watchers/NodeWatcher.js b/packages/metro-file-map/src/watchers/NodeWatcher.js index eb3f588190..72e5975952 100644 --- a/packages/metro-file-map/src/watchers/NodeWatcher.js +++ b/packages/metro-file-map/src/watchers/NodeWatcher.js @@ -106,7 +106,7 @@ module.exports = class NodeWatcher extends EventEmitter { const relativePath = path.relative(this.root, filepath); if ( type === 'f' && - !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath) + !common.isIncluded('f', this.globs, this.dot, this.doIgnore, relativePath) ) { return false; } @@ -278,53 +278,49 @@ module.exports = class NodeWatcher extends EventEmitter { } if ( - stat && - common.isFileIncluded( + !common.isIncluded( + 'd', this.globs, this.dot, this.doIgnore, relativePath, ) ) { - common.recReaddir( - path.resolve(this.root, relativePath), - (dir, stats) => { - if (this._watchdir(dir)) { - this._emitEvent(ADD_EVENT, path.relative(this.root, dir), { - modifiedTime: stats.mtime.getTime(), - size: stats.size, - type: 'd', - }); - } - }, - (file, stats) => { - if (this._register(file, 'f')) { - this._emitEvent(ADD_EVENT, path.relative(this.root, file), { - modifiedTime: stats.mtime.getTime(), - size: stats.size, - type: 'f', - }); - } - }, - (symlink, stats) => { - if (this._register(symlink, 'l')) { - this._rawEmitEvent( - ADD_EVENT, - path.relative(this.root, symlink), - { - modifiedTime: stats.mtime.getTime(), - size: stats.size, - type: 'l', - }, - ); - } - }, - function endCallback() {}, - this._checkedEmitError, - this.ignored, - ); + return; } - return; + common.recReaddir( + path.resolve(this.root, relativePath), + (dir, stats) => { + if (this._watchdir(dir)) { + this._emitEvent(ADD_EVENT, path.relative(this.root, dir), { + modifiedTime: stats.mtime.getTime(), + size: stats.size, + type: 'd', + }); + } + }, + (file, stats) => { + if (this._register(file, 'f')) { + this._emitEvent(ADD_EVENT, path.relative(this.root, file), { + modifiedTime: stats.mtime.getTime(), + size: stats.size, + type: 'f', + }); + } + }, + (symlink, stats) => { + if (this._register(symlink, 'l')) { + this._rawEmitEvent(ADD_EVENT, path.relative(this.root, symlink), { + modifiedTime: stats.mtime.getTime(), + size: stats.size, + type: 'l', + }); + } + }, + function endCallback() {}, + this._checkedEmitError, + this.ignored, + ); } else { const type = common.typeFromStat(stat); if (type == null) { diff --git a/packages/metro-file-map/src/watchers/WatchmanWatcher.js b/packages/metro-file-map/src/watchers/WatchmanWatcher.js index 4cb7d7fd96..170de07a20 100644 --- a/packages/metro-file-map/src/watchers/WatchmanWatcher.js +++ b/packages/metro-file-map/src/watchers/WatchmanWatcher.js @@ -247,14 +247,26 @@ export default class WatchmanWatcher extends EventEmitter { } = changeDescriptor; debug( - 'Handling change to: %s (new: %s, exists: %s)', + 'Handling change to: %s (new: %s, exists: %s, type: %s)', relativePath, isNew, exists, + type, ); + // Ignore files of an unrecognized type + if (type != null && !(type === 'f' || type === 'd' || type === 'l')) { + return; + } + if ( - !common.isFileIncluded(this.globs, this.dot, this.doIgnore, relativePath) + !common.isIncluded( + type, + this.globs, + this.dot, + this.doIgnore, + relativePath, + ) ) { return; } @@ -274,10 +286,8 @@ export default class WatchmanWatcher extends EventEmitter { ); if ( - type === 'f' || - type === 'l' || // Change event on dirs are mostly useless. - (type === 'd' && eventType !== CHANGE_EVENT) + !(type === 'd' && eventType === CHANGE_EVENT) ) { self._emitEvent(eventType, relativePath, self.root, { modifiedTime: Number(mtime_ms), diff --git a/packages/metro-file-map/src/watchers/__tests__/integration-test.js b/packages/metro-file-map/src/watchers/__tests__/integration-test.js index ba4b03520b..534caeec81 100644 --- a/packages/metro-file-map/src/watchers/__tests__/integration-test.js +++ b/packages/metro-file-map/src/watchers/__tests__/integration-test.js @@ -47,8 +47,8 @@ describe.each(Object.keys(WATCHERS))( // these files to ensure that tests remain isolated. await mkdir(join(watchRoot, 'existing')); await Promise.all([ - writeFile(join(watchRoot, 'existing', 'file-to-delete'), ''), - writeFile(join(watchRoot, 'existing', 'file-to-modify'), ''), + writeFile(join(watchRoot, 'existing', 'file-to-delete.js'), ''), + writeFile(join(watchRoot, 'existing', 'file-to-modify.js'), ''), symlink('target', join(watchRoot, 'existing', 'symlink-to-delete')), ]); @@ -58,7 +58,7 @@ describe.each(Object.keys(WATCHERS))( const opts: WatcherOptions = { dot: true, - glob: [], + glob: ['**/package.json', '**/*.js', '**/cookie-*'], // We need to ignore `.watchmanconfig` to keep these tests stable. // Even though we write it before initialising watchers, OS-level // delays/debouncing(?) can mean the write is *sometimes* reported by @@ -171,10 +171,10 @@ describe.each(Object.keys(WATCHERS))( maybeTest('detects deletion of a pre-existing file', async () => { expect( await eventHelpers.nextEvent(() => - unlink(join(watchRoot, 'existing', 'file-to-delete')), + unlink(join(watchRoot, 'existing', 'file-to-delete.js')), ), ).toStrictEqual({ - path: join('existing', 'file-to-delete'), + path: join('existing', 'file-to-delete.js'), eventType: 'delete', metadata: undefined, }); @@ -195,10 +195,13 @@ describe.each(Object.keys(WATCHERS))( maybeTest('detects change to a pre-existing file as a change', async () => { expect( await eventHelpers.nextEvent(() => - writeFile(join(watchRoot, 'existing', 'file-to-modify'), 'changed'), + writeFile( + join(watchRoot, 'existing', 'file-to-modify.js'), + 'changed', + ), ), ).toStrictEqual({ - path: join('existing', 'file-to-modify'), + path: join('existing', 'file-to-modify.js'), eventType: 'change', metadata: expect.any(Object), }); diff --git a/packages/metro-file-map/src/watchers/common.js b/packages/metro-file-map/src/watchers/common.js index 9b9032b504..2dbdb6d99c 100644 --- a/packages/metro-file-map/src/watchers/common.js +++ b/packages/metro-file-map/src/watchers/common.js @@ -89,7 +89,8 @@ export const assignOptions = function ( /** * Checks a file relative path against the globs array. */ -export function isFileIncluded( +export function isIncluded( + type: ?('f' | 'l' | 'd'), globs: $ReadOnlyArray, dot: boolean, doIgnore: string => boolean, @@ -98,9 +99,12 @@ export function isFileIncluded( if (doIgnore(relativePath)) { return false; } - return globs.length - ? micromatch.some(relativePath, globs, {dot}) - : dot || micromatch.some(relativePath, '**/*'); + // For non-regular files or if there are no glob matchers, just respect the + // `dot` option to filter dotfiles if dot === false. + if (globs.length === 0 || type !== 'f') { + return dot || micromatch.some(relativePath, '**/*'); + } + return micromatch.some(relativePath, globs, {dot}); } /**