Skip to content

Commit

Permalink
Watcher backends: Don't apply glob filter to symlink/directory change…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
robhogan committed Mar 21, 2023
1 parent a54288a commit ab86982
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 73 deletions.
22 changes: 6 additions & 16 deletions packages/metro-file-map/src/watchers/FSEventsWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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);
Expand All @@ -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(),
Expand Down
78 changes: 37 additions & 41 deletions packages/metro-file-map/src/watchers/NodeWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down
20 changes: 15 additions & 5 deletions packages/metro-file-map/src/watchers/WatchmanWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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),
Expand Down
17 changes: 10 additions & 7 deletions packages/metro-file-map/src/watchers/__tests__/integration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')),
]);

Expand All @@ -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
Expand Down Expand Up @@ -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,
});
Expand All @@ -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),
});
Expand Down
12 changes: 8 additions & 4 deletions packages/metro-file-map/src/watchers/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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});
}

/**
Expand Down

0 comments on commit ab86982

Please sign in to comment.