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

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(),

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) {

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),

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),
       });

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<string>,
   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});
 }
 
 /**