From b7f07ccf3e2f17c043c0d9f6dac337b958aad803 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 11 Dec 2024 04:38:27 -0800 Subject: [PATCH] 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 --- .../src/__tests__/index-test.js | 15 +++-- .../src/__tests__/worker-test.js | 33 ---------- packages/metro-file-map/src/flow-types.js | 2 - packages/metro-file-map/src/index.js | 63 +++++++------------ packages/metro-file-map/src/worker.js | 9 +-- packages/metro-file-map/types/flow-types.d.ts | 2 - 6 files changed, 35 insertions(+), 89 deletions(-) diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index 549cf6d7b0..2b649a53d7 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/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'), }, ], diff --git a/packages/metro-file-map/src/__tests__/worker-test.js b/packages/metro-file-map/src/__tests__/worker-test.js index 81ec1dd3e1..47451ff029 100644 --- a/packages/metro-file-map/src/__tests__/worker-test.js +++ b/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') diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index 85647db497..34d66b2133 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/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, }>; diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index 722ffee00b..daf7b98447 100644 --- a/packages/metro-file-map/src/index.js +++ b/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 { + // 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); diff --git a/packages/metro-file-map/src/worker.js b/packages/metro-file-map/src/worker.js index 3021a022e6..81edc489e2 100644 --- a/packages/metro-file-map/src/worker.js +++ b/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 = { diff --git a/packages/metro-file-map/types/flow-types.d.ts b/packages/metro-file-map/types/flow-types.d.ts index 92fe42d227..902c738584 100644 --- a/packages/metro-file-map/types/flow-types.d.ts +++ b/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; }>;