Simplify symlink handling in metro-file-map processFile (#1398)

Summary:

Reading symlink targets with `readLink` is currently delegated to `metro-file-map`'s worker abstraction. However, due to the way the abstraction is used for symlinks, we never actually use the worker processes/threads, and always process in band.

The key here is that `readLink` is always mutually exclusive with other worker tasks, like `computeSha1` and `computeDependencies` - as it must be, because those operations don't make sense for symlink files.

This lifts the call to `readLink` out of the abstraction and explicitly into the main process, simplifying the worker and improving readability.

Changelog: Internal

Differential Revision: D66899343

packages/metro-file-map/src/__tests__/index-test.js

@@ -469,6 +469,16 @@ describe('FileMap', () => {
       }),
     );
 
+    // We should not have read files inside node_modules during a build
+    // if retainAllFiles == false.
+    expect(require('fs').readFileSync.mock.calls).toEqual([
+      ['/project/fruits/Banana.js'],
+      ['/project/fruits/Pear.js'],
+      ['/project/fruits/Strawberry.js'],
+      ['/project/fruits/__mocks__/Pear.js'],
+      ['/project/vegetables/Melon.js'],
+    ]);
+
     expect(hasteMap.getModule('Banana')).toEqual(
       path.join(defaultConfig.rootDir, 'fruits', 'Banana.js'),
     );
@@ -1341,7 +1351,6 @@ describe('FileMap', () => {
           enableHastePackages: true,
           filePath: path.join('/', 'project', 'fruits', 'Banana.js'),
           hasteImplModulePath: undefined,
-          readLink: false,
           rootDir: path.join('/', 'project'),
         },
       ],
@@ -1353,7 +1362,6 @@ describe('FileMap', () => {
           enableHastePackages: true,
           filePath: path.join('/', 'project', 'fruits', 'Pear.js'),
           hasteImplModulePath: undefined,
-          readLink: false,
           rootDir: path.join('/', 'project'),
         },
       ],
@@ -1365,7 +1373,6 @@ describe('FileMap', () => {
           enableHastePackages: true,
           filePath: path.join('/', 'project', 'fruits', 'Strawberry.js'),
           hasteImplModulePath: undefined,
-          readLink: false,
           rootDir: path.join('/', 'project'),
         },
       ],
@@ -1377,7 +1384,6 @@ describe('FileMap', () => {
           enableHastePackages: true,
           filePath: path.join('/', 'project', 'fruits', '__mocks__', 'Pear.js'),
           hasteImplModulePath: undefined,
-          readLink: false,
           rootDir: path.join('/', 'project'),
         },
       ],
@@ -1389,7 +1395,6 @@ describe('FileMap', () => {
           enableHastePackages: true,
           filePath: path.join('/', 'project', 'vegetables', 'Melon.js'),
           hasteImplModulePath: undefined,
-          readLink: false,
           rootDir: path.join('/', 'project'),
         },
       ],

packages/metro-file-map/src/__tests__/worker-test.js

@@ -54,19 +54,6 @@ jest.mock('fs', () => {
       }
       throw new Error(`Cannot read path '${path}'.`);
     }),
-    promises: {
-      readlink: jest.fn(async path => {
-        const entry = mockFs[path];
-        if (entry) {
-          if (typeof entry.link === 'string') {
-            return entry.link;
-          } else {
-            throw new Error('Tried to call readlink on a non-symlink');
-          }
-        }
-        throw new Error(`Cannot read path '${path}'.`);
-      }),
-    },
   };
 });
 
@@ -240,26 +227,6 @@ describe('worker', () => {
     expect(fs.readFile).not.toHaveBeenCalled();
   });
 
-  test('calls readLink and returns symlink target when readLink=true', async () => {
-    expect(
-      await worker({
-        computeDependencies: false,
-        filePath: path.join('/project', 'fruits', 'LinkToStrawberry.js'),
-        readLink: true,
-        rootDir,
-      }),
-    ).toEqual({
-      dependencies: undefined,
-      id: undefined,
-      module: undefined,
-      sha1: undefined,
-      symlinkTarget: path.join('.', 'Strawberry.js'),
-    });
-
-    expect(fs.readFileSync).not.toHaveBeenCalled();
-    expect(fs.promises.readlink).toHaveBeenCalled();
-  });
-
   test('can be loaded directly without transpilation', async () => {
     const code = await jest
       .requireActual('fs')

packages/metro-file-map/src/flow-types.js

@@ -326,7 +326,6 @@ export type WorkerMessage = $ReadOnly<{
   computeSha1: boolean,
   dependencyExtractor?: ?string,
   enableHastePackages: boolean,
-  readLink: boolean,
   rootDir: string,
   filePath: string,
   hasteImplModulePath?: ?string,
@@ -337,5 +336,4 @@ export type WorkerMetadata = $ReadOnly<{
   id?: ?string,
   module?: ?HasteMapItemMetaData,
   sha1?: ?string,
-  symlinkTarget?: ?string,
 }>;

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

@@ -51,6 +51,7 @@ import TreeFS from './lib/TreeFS';
 import {Watcher} from './Watcher';
 import {worker} from './worker';
 import EventEmitter from 'events';
+import {promises as fsPromises} from 'fs';
 import invariant from 'invariant';
 import {Worker} from 'jest-worker';
 import {AbortController} from 'node-abort-controller';
@@ -525,18 +526,26 @@ export default class FileMap extends EventEmitter {
     fileMetadata: FileMetaData,
     workerOptions?: {forceInBand?: ?boolean, perfLogger?: ?PerfLogger},
   ): ?Promise<void> {
+    // Symlink Haste modules, Haste packages or mocks are not supported - read
+    // the target if requested and return early.
+    if (fileMetadata[H.SYMLINK] !== 0) {
+      // If we only need to read a link, it's more efficient to do it in-band
+      // (with async file IO) than to have the overhead of worker IO.
+      if (fileMetadata[H.SYMLINK] === 1) {
+        return fsPromises.readlink(filePath).then(symlinkTarget => {
+          fileMetadata[H.VISITED] = 1;
+          fileMetadata[H.SYMLINK] = symlinkTarget;
+        });
+      }
+      return null;
+    }
+
     const rootDir = this._options.rootDir;
 
     const relativeFilePath = this._pathUtils.absoluteToNormal(filePath);
-    const isSymlink = fileMetadata[H.SYMLINK] !== 0;
 
     const computeSha1 =
-      this._options.computeSha1 && !isSymlink && fileMetadata[H.SHA1] == null;
-
-    const readLink =
-      this._options.enableSymlinks &&
-      isSymlink &&
-      typeof fileMetadata[H.SYMLINK] !== 'string';
+      this._options.computeSha1 && fileMetadata[H.SHA1] == null;
 
     // Callback called when the response from the worker is successful.
     const workerReply = (metadata: WorkerMetadata) => {
@@ -557,10 +566,6 @@ export default class FileMap extends EventEmitter {
       if (computeSha1) {
         fileMetadata[H.SHA1] = metadata.sha1;
       }
-
-      if (metadata.symlinkTarget != null) {
-        fileMetadata[H.SYMLINK] = metadata.symlinkTarget;
-      }
     };
 
     // Callback called when the response from the worker is an error.
@@ -579,41 +584,22 @@ export default class FileMap extends EventEmitter {
       throw error;
     };
 
-    // If we retain all files in the virtual HasteFS representation, we avoid
-    // reading them if they aren't important (node_modules).
+    // If we're tracking node_modules (retainAllFiles), use a cheaper worker
+    // configuration for those files, because we never care about extracting
+    // dependencies, and they may never be Haste modules or packages.
+    //
+    // Note that if retainAllFiles==false, no node_modules file should get this
+    // far - it will have been ignored by the crawler.
     if (this._options.retainAllFiles && filePath.includes(NODE_MODULES)) {
-      if (computeSha1 || readLink) {
+      if (computeSha1) {
         return this._getWorker(workerOptions)
           .worker({
             computeDependencies: false,
-            computeSha1,
-            dependencyExtractor: null,
-            enableHastePackages: false,
-            filePath,
-            hasteImplModulePath: null,
-            readLink,
-            rootDir,
-          })
-          .then(workerReply, workerError);
-      }
-      return null;
-    }
-
-    // Symlink Haste modules, Haste packages or mocks are not supported - read
-    // the target if requested and return early.
-    if (isSymlink) {
-      if (readLink) {
-        // If we only need to read a link, it's more efficient to do it in-band
-        // (with async file IO) than to have the overhead of worker IO.
-        return this._getWorker({forceInBand: true})
-          .worker({
-            computeDependencies: false,
-            computeSha1: false,
+            computeSha1: true,
             dependencyExtractor: null,
             enableHastePackages: false,
             filePath,
             hasteImplModulePath: null,
-            readLink,
             rootDir,
           })
           .then(workerReply, workerError);
@@ -662,7 +648,6 @@ export default class FileMap extends EventEmitter {
         enableHastePackages: this._options.enableHastePackages,
         filePath,
         hasteImplModulePath: this._options.hasteImplModulePath,
-        readLink: false,
         rootDir,
       })
       .then(workerReply, workerError);

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

@@ -18,7 +18,6 @@ const H = require('./constants');
 const dependencyExtractor = require('./lib/dependencyExtractor');
 const excludedExtensions = require('./workerExclusionList');
 const {createHash} = require('crypto');
-const {promises: fsPromises} = require('fs');
 const fs = require('graceful-fs');
 const path = require('path');
 
@@ -54,13 +53,11 @@ async function worker(
   let id /*: WorkerMetadata['id'] */;
   let module /*: WorkerMetadata['module'] */;
   let sha1 /*: WorkerMetadata['sha1'] */;
-  let symlinkTarget /*: WorkerMetadata['symlinkTarget'] */;
 
   const {
     computeDependencies,
     computeSha1,
     enableHastePackages,
-    readLink,
     rootDir,
     filePath,
   } = data;
@@ -118,11 +115,7 @@ async function worker(
     sha1 = sha1hex(getContent());
   }
 
-  if (readLink) {
-    symlinkTarget = await fsPromises.readlink(filePath);
-  }
-
-  return {dependencies, id, module, sha1, symlinkTarget};
+  return {dependencies, id, module, sha1};
 }
 
 module.exports = {

packages/metro-file-map/types/flow-types.d.ts

@@ -304,7 +304,6 @@ export type WorkerMessage = Readonly<{
   computeSha1: boolean;
   dependencyExtractor?: string | null;
   enableHastePackages: boolean;
-  readLink: boolean;
   rootDir: string;
   filePath: string;
   hasteImplModulePath?: string | null;
@@ -315,5 +314,4 @@ export type WorkerMetadata = Readonly<{
   id?: string | null;
   module?: HasteMapItemMetaData | null;
   sha1?: string | null;
-  symlinkTarget?: string | null;
 }>;