From d1343fc369b4484d9fe60d13ab026a058b887e97 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 11 Dec 2024 05:20:50 -0800 Subject: [PATCH 1/2] 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 | 5 -- .../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, 25 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..867ac66994 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -1341,7 +1341,6 @@ describe('FileMap', () => { enableHastePackages: true, filePath: path.join('/', 'project', 'fruits', 'Banana.js'), hasteImplModulePath: undefined, - readLink: false, rootDir: path.join('/', 'project'), }, ], @@ -1353,7 +1352,6 @@ describe('FileMap', () => { enableHastePackages: true, filePath: path.join('/', 'project', 'fruits', 'Pear.js'), hasteImplModulePath: undefined, - readLink: false, rootDir: path.join('/', 'project'), }, ], @@ -1365,7 +1363,6 @@ describe('FileMap', () => { enableHastePackages: true, filePath: path.join('/', 'project', 'fruits', 'Strawberry.js'), hasteImplModulePath: undefined, - readLink: false, rootDir: path.join('/', 'project'), }, ], @@ -1377,7 +1374,6 @@ describe('FileMap', () => { enableHastePackages: true, filePath: path.join('/', 'project', 'fruits', '__mocks__', 'Pear.js'), hasteImplModulePath: undefined, - readLink: false, rootDir: path.join('/', 'project'), }, ], @@ -1389,7 +1385,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; }>; From 664c5d659980afae2641b90082e6ebb74bbd663e Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Wed, 11 Dec 2024 05:20:50 -0800 Subject: [PATCH 2/2] metro-file-map: Extract mocks handling into nullable MockMap (#1402) Summary: Extract almost all mock handling logic out of `FileMap` and into `MockMap`. Mocks are obviously a Jest concept inherited from `jest-haste-map`, but we're keeping them around in `metro-file-map` for planned re-use of `metro-file-map` as a custom `hasteMapModulePath` in Jest, at least at Meta. This is a step towards making `MockMap` into a *plugin* or subscriber to `FileMap`, so that we don't need to keep it in `metro-file-map`'s codebase and can inject it where needed. Changelog: Internal (No Metro-user-facing change, `metro-file-map`'s API is [explicitly experimental](https://github.com/facebook/metro/tree/main/packages/metro-file-map#experimental-metro-file-map)) Test Plan: - New unit tests - `mocksPattern` is always `null` in Metro, so the new code isn't reached right now, except by tests. - We'll exercise this code next half when we use it in a Jest context. Differential Revision: D67056483 Pulled By: robhogan --- .../src/__tests__/index-test.js | 30 +++++++ packages/metro-file-map/src/flow-types.js | 2 +- packages/metro-file-map/src/index.js | 87 ++++++------------- packages/metro-file-map/src/lib/MockMap.js | 71 ++++++++++++++- 4 files changed, 127 insertions(+), 63 deletions(-) diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index 867ac66994..8652d2e19e 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -742,6 +742,36 @@ describe('FileMap', () => { expect(fs.readFileSync.mock.calls.length).toBe(5); }); + test('builds a mock map if mocksPattern is non-null', async () => { + const pathToMock = path.join( + '/', + 'project', + 'fruits1', + '__mocks__', + 'Blueberry.js', + ); + mockFs[pathToMock] = '/* empty */'; + + const {mockMap} = await new FileMap({ + mocksPattern: '__mocks__', + throwOnModuleCollision: true, + ...defaultConfig, + }).build(); + + expect(mockMap).not.toBeNull(); + expect(mockMap.getMockModule('Blueberry')).toEqual(pathToMock); + }); + + test('returns null mockMap if mocksPattern is empty', async () => { + const {mockMap} = await new FileMap({ + mocksPattern: '', + throwOnModuleCollision: true, + ...defaultConfig, + }).build(); + + expect(mockMap).toBeNull(); + }); + test('warns on duplicate mock files', async () => { expect.assertions(1); diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js index 34d66b2133..97cd5a27c1 100644 --- a/packages/metro-file-map/src/flow-types.js +++ b/packages/metro-file-map/src/flow-types.js @@ -43,7 +43,7 @@ export type BuildParameters = $ReadOnly<{ export type BuildResult = { fileSystem: FileSystem, hasteMap: HasteMap, - mockMap: MockMap, + mockMap: ?MockMap, }; export type CacheData = $ReadOnly<{ diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index daf7b98447..47e2e4c9f2 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -31,8 +31,6 @@ import type { Path, PerfLogger, PerfLoggerFactory, - RawMockMap, - ReadOnlyRawMockMap, WatchmanClocks, WorkerMetadata, } from './flow-types'; @@ -40,9 +38,7 @@ import type {IJestWorker} from 'jest-worker'; import {DiskCacheManager} from './cache/DiskCacheManager'; import H from './constants'; -import getMockName from './getMockName'; import checkWatchmanCapabilities from './lib/checkWatchmanCapabilities'; -import {DuplicateError} from './lib/DuplicateError'; import MockMapImpl from './lib/MockMap'; import MutableHasteMap from './lib/MutableHasteMap'; import normalizePathSeparatorsToSystem from './lib/normalizePathSeparatorsToSystem'; @@ -367,7 +363,6 @@ export default class FileMap extends EventEmitter { }) : new TreeFS({rootDir}); this._startupPerfLogger?.point('constructFileSystem_end'); - const mocks = initialData?.mocks ?? new Map(); // Construct the Haste map from the cached file system state while // crawling to build a diff of current state vs cached. `fileSystem` @@ -380,13 +375,24 @@ export default class FileMap extends EventEmitter { this._constructHasteMap(fileSystem), ]); + const mockMap = + this._options.mocksPattern != null + ? new MockMapImpl({ + console: this._console, + mocksPattern: this._options.mocksPattern, + rawMockMap: initialData?.mocks ?? new Map(), + rootDir, + throwOnModuleCollision: this._options.throwOnModuleCollision, + }) + : null; + // Update `fileSystem`, `hasteMap` and `mocks` based on the file delta. - await this._applyFileDelta(fileSystem, hasteMap, mocks, fileDelta); + await this._applyFileDelta(fileSystem, hasteMap, mockMap, fileDelta); await this._takeSnapshotAndPersist( fileSystem, fileDelta.clocks ?? new Map(), - mocks, + mockMap, fileDelta.changedFiles, fileDelta.removedFiles, ); @@ -396,11 +402,11 @@ export default class FileMap extends EventEmitter { fileDelta.removedFiles.size, ); - await this._watch(fileSystem, hasteMap, mocks); + await this._watch(fileSystem, hasteMap, mockMap); return { fileSystem, hasteMap, - mockMap: new MockMapImpl({rootDir, rawMockMap: mocks}), + mockMap, }; })(); } @@ -521,7 +527,7 @@ export default class FileMap extends EventEmitter { */ _processFile( hasteMap: MutableHasteMap, - mockMap: RawMockMap, + mockMap: ?MockMapImpl, filePath: Path, fileMetadata: FileMetaData, workerOptions?: {forceInBand?: ?boolean, perfLogger?: ?PerfLogger}, @@ -542,8 +548,6 @@ export default class FileMap extends EventEmitter { const rootDir = this._options.rootDir; - const relativeFilePath = this._pathUtils.absoluteToNormal(filePath); - const computeSha1 = this._options.computeSha1 && fileMetadata[H.SHA1] == null; @@ -607,38 +611,7 @@ export default class FileMap extends EventEmitter { return null; } - if ( - this._options.mocksPattern && - this._options.mocksPattern.test(filePath) - ) { - const mockPath = getMockName(filePath); - const existingMockPath = mockMap.get(mockPath); - - if (existingMockPath != null) { - const secondMockPath = this._pathUtils.absoluteToNormal(filePath); - if (existingMockPath !== secondMockPath) { - const method = this._options.throwOnModuleCollision - ? 'error' - : 'warn'; - - this._console[method]( - [ - 'metro-file-map: duplicate manual mock found: ' + mockPath, - ' The following files share their name; please delete one of them:', - ' * ' + path.sep + existingMockPath, - ' * ' + path.sep + secondMockPath, - '', - ].join('\n'), - ); - - if (this._options.throwOnModuleCollision) { - throw new DuplicateError(existingMockPath, secondMockPath); - } - } - } - - mockMap.set(mockPath, relativeFilePath); - } + mockMap?.onNewOrModifiedFile(filePath); return this._getWorker(workerOptions) .worker({ @@ -656,7 +629,7 @@ export default class FileMap extends EventEmitter { async _applyFileDelta( fileSystem: MutableFileSystem, hasteMap: MutableHasteMap, - mockMap: RawMockMap, + mockMap: ?MockMapImpl, delta: $ReadOnly<{ changedFiles: FileData, removedFiles: $ReadOnlySet, @@ -758,7 +731,7 @@ export default class FileMap extends EventEmitter { async _takeSnapshotAndPersist( fileSystem: FileSystem, clocks: WatchmanClocks, - mockMap: ReadOnlyRawMockMap, + mockMap: ?MockMapImpl, changed: FileData, removed: Set, ) { @@ -767,7 +740,7 @@ export default class FileMap extends EventEmitter { { fileSystemData: fileSystem.getSerializableSnapshot(), clocks: new Map(clocks), - mocks: new Map(mockMap), + mocks: mockMap ? mockMap.getSerializableSnapshot() : new Map(), }, {changed, removed}, ); @@ -809,7 +782,7 @@ export default class FileMap extends EventEmitter { _removeIfExists( fileSystem: MutableFileSystem, hasteMap: MutableHasteMap, - mockMap: RawMockMap, + mockMap: ?MockMapImpl, relativeFilePath: Path, ) { const fileMetadata = fileSystem.remove(relativeFilePath); @@ -823,18 +796,10 @@ export default class FileMap extends EventEmitter { hasteMap.removeModule(moduleName, relativeFilePath); - if (this._options.mocksPattern) { - const absoluteFilePath = path.join( - this._options.rootDir, - normalizePathSeparatorsToSystem(relativeFilePath), + if (mockMap) { + mockMap?.onRemovedFile( + this._pathUtils.normalToAbsolute(relativeFilePath), ); - if ( - this._options.mocksPattern && - this._options.mocksPattern.test(absoluteFilePath) - ) { - const mockName = getMockName(absoluteFilePath); - mockMap.delete(mockName); - } } } @@ -844,7 +809,7 @@ export default class FileMap extends EventEmitter { async _watch( fileSystem: MutableFileSystem, hasteMap: MutableHasteMap, - mockMap: RawMockMap, + mockMap: ?MockMapImpl, ): Promise { this._startupPerfLogger?.point('watch_start'); if (!this._options.watch) { @@ -854,8 +819,8 @@ export default class FileMap extends EventEmitter { // In watch mode, we'll only warn about module collisions and we'll retain // all files, even changes to node_modules. - this._options.throwOnModuleCollision = false; hasteMap.setThrowOnModuleCollision(false); + mockMap?.setThrowOnModuleCollision(false); this._options.retainAllFiles = true; const hasWatchedExtension = (filePath: string) => diff --git a/packages/metro-file-map/src/lib/MockMap.js b/packages/metro-file-map/src/lib/MockMap.js index 395ffa85b8..455a0873be 100644 --- a/packages/metro-file-map/src/lib/MockMap.js +++ b/packages/metro-file-map/src/lib/MockMap.js @@ -11,21 +11,90 @@ import type {MockMap as IMockMap, Path, RawMockMap} from '../flow-types'; +import getMockName from '../getMockName'; +import {DuplicateError} from './DuplicateError'; import {RootPathUtils} from './RootPathUtils'; +import path from 'path'; export default class MockMap implements IMockMap { + +#mocksPattern: RegExp; +#raw: RawMockMap; +#rootDir: Path; +#pathUtils: RootPathUtils; + +#console: typeof console; + #throwOnModuleCollision: boolean; - constructor({rawMockMap, rootDir}: {rawMockMap: RawMockMap, rootDir: Path}) { + constructor({ + console, + mocksPattern, + rawMockMap, + rootDir, + throwOnModuleCollision, + }: { + console: typeof console, + mocksPattern: RegExp, + rawMockMap: RawMockMap, + rootDir: Path, + throwOnModuleCollision: boolean, + }) { + this.#mocksPattern = mocksPattern; this.#raw = rawMockMap; this.#rootDir = rootDir; + this.#console = console; this.#pathUtils = new RootPathUtils(rootDir); + this.#throwOnModuleCollision = throwOnModuleCollision; } getMockModule(name: string): ?Path { const mockPath = this.#raw.get(name) || this.#raw.get(name + '/index'); return mockPath != null ? this.#pathUtils.normalToAbsolute(mockPath) : null; } + + onNewOrModifiedFile(absoluteFilePath: Path): void { + if (!this.#mocksPattern.test(absoluteFilePath)) { + return; + } + + const mockName = getMockName(absoluteFilePath); + const existingMockPath = this.#raw.get(mockName); + const newMockPath = this.#pathUtils.absoluteToNormal(absoluteFilePath); + + if (existingMockPath != null) { + if (existingMockPath !== newMockPath) { + const method = this.#throwOnModuleCollision ? 'error' : 'warn'; + + this.#console[method]( + [ + 'metro-file-map: duplicate manual mock found: ' + mockName, + ' The following files share their name; please delete one of them:', + ' * ' + path.sep + existingMockPath, + ' * ' + path.sep + newMockPath, + '', + ].join('\n'), + ); + + if (this.#throwOnModuleCollision) { + throw new DuplicateError(existingMockPath, newMockPath); + } + } + } + + this.#raw.set(mockName, newMockPath); + } + + onRemovedFile(absoluteFilePath: Path): void { + if (!this.#mocksPattern.test(absoluteFilePath)) { + return; + } + const mockName = getMockName(absoluteFilePath); + this.#raw.delete(mockName); + } + + setThrowOnModuleCollision(throwOnModuleCollision: boolean): void { + this.#throwOnModuleCollision = throwOnModuleCollision; + } + + getSerializableSnapshot(): RawMockMap { + return new Map(this.#raw); + } }