refactor(metro-file-map): Drop "native find binary" crawler (and forceNodeFilesystemAPI flag) (#1693)

Summary:
This is a proposal to drop the native find binary crawler code path entirely.

We've recently dropped the native find binary crawler in Expo, which was accidentally activated in some cases. We discovered (which is particularly obvious on large projects/repos and `expo/expo` itself) that it's consistently much slower than the Node FS crawler.

To get into the "correct" preference and use the Node crawler, we're currently forcing `useWatchman: null`, which then defaults to use watchman, if it's installed, but falls back to `forceNodeFilesystemAPI: true`. This is outside of what the types specify as valid, but is the cleanest way to disable the native find binary crawling.

This crawler basically just adds overhead over the Node.js implementation and is consistently slower, and less efficient, while the Node crawler is pretty optimised already (and has very low overhead once #1677 is merged)

The timings of the native find binary crawler in `expo/expo` was also observed to scale with the size of this repo, compared to a minimal reproduction, so the overhead isn't just limited to the child process call and result parsing.

Changelog: [Internal] Drop native find binary crawler and remove `forceNodeFilesystemAPI` flag

Pull Request resolved: #1693

Test Plan:
- Existing tests should pass
- Quick generated script to play around with and assess the crawlers' comparative performance: kitten@74dffd4

Reviewed By: huntie

Differential Revision: D101985455

Pulled By: robhogan

fbshipit-source-id: efe2ab02fc5c0edac4d68f037db57c635fef8e50

Author: kitten

packages/metro-file-map/src/Watcher.js

@@ -49,7 +49,6 @@ type WatcherOptions = {
   console: Console,
   enableSymlinks: boolean,
   extensions: ReadonlyArray<string>,
-  forceNodeFilesystemAPI: boolean,
   healthCheckFilePrefix: string,
   ignoreForCrawl: (filePath: string) => boolean,
   ignorePatternForWatch: RegExp,
@@ -131,7 +130,6 @@ export class Watcher extends EventEmitter {
       console: options.console,
       includeSymlinks: options.enableSymlinks,
       extensions: options.extensions,
-      forceNodeFilesystemAPI: options.forceNodeFilesystemAPI,
       ignore: ignoreForCrawl,
       onStatus: status => {
         this.emit('status', status);

packages/metro-file-map/src/cache/__tests__/DiskCacheManager-test.js

@@ -42,7 +42,6 @@ const buildParameters: BuildParameters = {
   cacheBreaker: '',
   computeSha1: true,
   enableSymlinks: false,
-  forceNodeFilesystemAPI: true,
   ignorePattern: /ignored/,
   retainAllFiles: false,
   extensions: ['js', 'json'],

packages/metro-file-map/src/crawlers/__tests__/integration-test.js

@@ -34,18 +34,10 @@ const isWatchmanOnPath = () => {
   }
 };
 
-const mockUseNativeFind = jest.fn();
-jest.mock('../node/hasNativeFindSupport', () => () => mockUseNativeFind());
-
 type Crawler = typeof nodeCrawl | typeof watchmanCrawl;
 
 const CRAWLERS: {[key: string]: ?Crawler} = {
-  'node-find': opts => {
-    mockUseNativeFind.mockResolvedValue(true);
-    return nodeCrawl(opts);
-  },
   'node-recursive': opts => {
-    mockUseNativeFind.mockResolvedValue(false);
     return nodeCrawl(opts);
   },
   watchman: isWatchmanOnPath() ? watchmanCrawl : null,
@@ -140,7 +132,6 @@ describe.each(Object.keys(CRAWLERS))(
           rootDir: FIXTURES_DIR,
           abortSignal: null,
           computeSha1: false,
-          forceNodeFilesystemAPI: false,
           onStatus: () => {},
         });
 

packages/metro-file-map/src/crawlers/__tests__/node-test.js

@@ -12,32 +12,6 @@ import TreeFS from '../../lib/TreeFS';
 
 jest.useRealTimers();
 
-jest.mock('child_process', () => ({
-  spawn: jest.fn((cmd, args) => {
-    let closeCallback;
-    return {
-      on: jest.fn().mockImplementation((event, callback) => {
-        if (event === 'exit') {
-          callback(mockSpawnExit, null);
-        }
-      }),
-      stdout: {
-        on: jest.fn().mockImplementation((event, callback) => {
-          if (event === 'data') {
-            setTimeout(() => {
-              callback(mockResponse);
-              setTimeout(closeCallback, 0);
-            }, 0);
-          } else if (event === 'close') {
-            closeCallback = callback;
-          }
-        }),
-        setEncoding: jest.fn(),
-      },
-    };
-  }),
-}));
-
 jest.mock('graceful-fs', () => {
   const slash = require('slash');
   let mtime = 32;
@@ -126,96 +100,37 @@ const createMap = obj =>
 const rootDir = '/project';
 const emptyFS = new TreeFS({rootDir, files: new Map()});
 const getFS = (files: FileData) => new TreeFS({rootDir, files});
-let mockResponse;
-let mockSpawnExit;
 let nodeCrawl;
-let childProcess;
 
 describe('node crawler', () => {
   beforeEach(() => {
     jest.resetModules();
-
-    mockResponse = [
-      '/project/fruits/pear.js',
-      '/project/fruits/strawberry.js',
-      '/project/fruits/tomato.js',
-    ].join('\n');
-
-    mockSpawnExit = 0;
-  });
-
-  test('crawls for files based on patterns', async () => {
-    childProcess = require('child_process');
-    nodeCrawl = require('../node').default;
-
-    mockResponse = [
-      '/project/fruits/pear.js',
-      '/project/fruits/strawberry.js',
-      '/project/fruits/tomato.js',
-      '/project/vegetables/melon.json',
-    ].join('\n');
-
-    const {changedFiles, removedFiles} = await nodeCrawl({
-      previousState: {fileSystem: emptyFS},
-      extensions: ['js', 'json'],
-      ignore: pearMatcher,
-      rootDir,
-      roots: ['/project/fruits', '/project/vegtables'],
-    });
-
-    expect(childProcess.spawn).lastCalledWith('find', [
-      '/project/fruits',
-      '/project/vegtables',
-      '(',
-      '(',
-      '-type',
-      'f',
-      '(',
-      '-iname',
-      '*.js',
-      '-o',
-      '-iname',
-      '*.json',
-      ')',
-      ')',
-      ')',
-    ]);
-
-    expect(changedFiles).not.toBe(null);
-
-    expect(changedFiles).toEqual(
-      createMap({
-        'fruits/strawberry.js': [32, 42, 0, null, 0, null],
-        'fruits/tomato.js': [33, 42, 0, null, 0, null],
-        'vegetables/melon.json': [34, 42, 0, null, 0, null],
-      }),
-    );
-
-    expect(removedFiles).toEqual(new Set());
   });
 
   test('updates only changed files', async () => {
     nodeCrawl = require('../node').default;
 
-    // In this test sample, strawberry is changed and tomato is unchanged
-    const tomato = [33, 42, 1, null, 0, null];
+    // The readdir mock returns tomato.js (mtime=32) and
+    // directory/strawberry.js (mtime=33). In this test, tomato is unchanged
+    // and strawberry is changed.
     const files = createMap({
-      'fruits/strawberry.js': [30, 40, 1, null, 0, null],
-      'fruits/tomato.js': tomato,
+      'fruits/directory/strawberry.js': [30, 40, 1, null, 0, null],
+      'fruits/tomato.js': [32, 42, 1, null, 0, null],
     });
 
     const {changedFiles, removedFiles} = await nodeCrawl({
+      console: global.console,
       previousState: {fileSystem: getFS(files)},
       extensions: ['js'],
       ignore: pearMatcher,
       rootDir,
       roots: ['/project/fruits'],
     });
 
-    // Tomato is not included because it is unchanged
+    // Tomato is not included because its mtime is unchanged
     expect(changedFiles).toEqual(
       createMap({
-        'fruits/strawberry.js': [32, 42, 0, null, 0, null],
+        'fruits/directory/strawberry.js': [33, 42, 0, null, 0, null],
       }),
     );
 
@@ -225,117 +140,34 @@ describe('node crawler', () => {
   test('returns removed files', async () => {
     nodeCrawl = require('../node').default;
 
-    // In this test sample, previouslyExisted was present before and will not be
-    // when crawling this directory.
+    // In this test sample, previouslyExisted was present before and will not
+    // be found when crawling this directory.
     const files = createMap({
       'fruits/previouslyExisted.js': [30, 40, 1, null, 0, null],
-      'fruits/strawberry.js': [33, 42, 0, null, 0, null],
+      'fruits/directory/strawberry.js': [33, 42, 0, null, 0, null],
       'fruits/tomato.js': [32, 42, 0, null, 0, null],
     });
 
     const {changedFiles, removedFiles} = await nodeCrawl({
+      console: global.console,
       previousState: {fileSystem: getFS(files)},
       extensions: ['js'],
       ignore: pearMatcher,
       rootDir,
       roots: ['/project/fruits'],
     });
 
-    expect(changedFiles).toEqual(
-      createMap({
-        'fruits/strawberry.js': [32, 42, 0, null, 0, null],
-        'fruits/tomato.js': [33, 42, 0, null, 0, null],
-      }),
-    );
+    expect(changedFiles).toEqual(new Map());
     expect(removedFiles).toEqual(new Set(['fruits/previouslyExisted.js']));
   });
 
-  test('uses node fs APIs with incompatible find binary', async () => {
-    mockResponse = '';
-    mockSpawnExit = 1;
-    childProcess = require('child_process');
-
-    nodeCrawl = require('../node').default;
-
-    const {changedFiles, removedFiles} = await nodeCrawl({
-      previousState: {fileSystem: emptyFS},
-      extensions: ['js'],
-      ignore: pearMatcher,
-      rootDir,
-      roots: ['/project/fruits'],
-    });
-
-    expect(childProcess.spawn).lastCalledWith(
-      'find',
-      ['.', '-type', 'f', '(', '-iname', '*.ts', '-o', '-iname', '*.js', ')'],
-      {cwd: expect.any(String)},
-    );
-    expect(changedFiles).toEqual(
-      createMap({
-        'fruits/directory/strawberry.js': [33, 42, 0, null, 0, null],
-        'fruits/tomato.js': [32, 42, 0, null, 0, null],
-      }),
-    );
-    expect(removedFiles).toEqual(new Set());
-  });
-
-  test('uses node fs APIs without find binary', async () => {
-    childProcess = require('child_process');
-    childProcess.spawn.mockImplementationOnce(() => {
-      throw new Error();
-    });
-    nodeCrawl = require('../node').default;
-
-    const {changedFiles, removedFiles} = await nodeCrawl({
-      console: global.console,
-      previousState: {fileSystem: emptyFS},
-      extensions: ['js'],
-      ignore: pearMatcher,
-      rootDir,
-      roots: ['/project/fruits'],
-    });
-
-    expect(changedFiles).toEqual(
-      createMap({
-        'fruits/directory/strawberry.js': [33, 42, 0, null, 0, null],
-        'fruits/tomato.js': [32, 42, 0, null, 0, null],
-      }),
-    );
-    expect(removedFiles).toEqual(new Set());
-  });
-
-  test('uses node fs APIs if "forceNodeFilesystemAPI" is set to true, regardless of platform', async () => {
-    childProcess = require('child_process');
-    nodeCrawl = require('../node').default;
-
-    const {changedFiles, removedFiles} = await nodeCrawl({
-      console: global.console,
-      previousState: {fileSystem: emptyFS},
-      extensions: ['js'],
-      forceNodeFilesystemAPI: true,
-      ignore: pearMatcher,
-      rootDir,
-      roots: ['/project/fruits'],
-    });
-
-    expect(childProcess.spawn).toHaveBeenCalledTimes(0);
-    expect(changedFiles).toEqual(
-      createMap({
-        'fruits/directory/strawberry.js': [33, 42, 0, null, 0, null],
-        'fruits/tomato.js': [32, 42, 0, null, 0, null],
-      }),
-    );
-    expect(removedFiles).toEqual(new Set());
-  });
-
   test('completes with empty roots', async () => {
     nodeCrawl = require('../node').default;
 
     const {changedFiles, removedFiles} = await nodeCrawl({
       console: global.console,
       previousState: {fileSystem: emptyFS},
       extensions: ['js'],
-      forceNodeFilesystemAPI: true,
       ignore: pearMatcher,
       rootDir,
       roots: [],
@@ -357,7 +189,6 @@ describe('node crawler', () => {
       console: mockConsole,
       previousState: {fileSystem: emptyFS},
       extensions: ['js'],
-      forceNodeFilesystemAPI: true,
       ignore: pearMatcher,
       rootDir,
       roots: ['/error'],
@@ -378,7 +209,6 @@ describe('node crawler', () => {
       console: global.console,
       previousState: {fileSystem: emptyFS},
       extensions: ['js'],
-      forceNodeFilesystemAPI: true,
       ignore: pearMatcher,
       rootDir,
       roots: ['/project/fruits'],
@@ -440,7 +270,7 @@ describe('node crawler', () => {
         extensions: ['js', 'json'],
         ignore: pearMatcher,
         rootDir,
-        roots: ['/project/fruits', '/project/vegtables'],
+        roots: ['/project/fruits'],
       }),
     ).rejects.toThrow(err);
   });