Skip to content

Commit

Permalink
Simplify symlink handling
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Dec 9, 2024
1 parent 9512ae0 commit a672bef
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 76 deletions.
15 changes: 10 additions & 5 deletions packages/metro-file-map/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
);
Expand Down Expand Up @@ -1341,7 +1351,6 @@ describe('FileMap', () => {
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Banana.js'),
hasteImplModulePath: undefined,
readLink: false,
rootDir: path.join('/', 'project'),
},
],
Expand All @@ -1353,7 +1362,6 @@ describe('FileMap', () => {
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Pear.js'),
hasteImplModulePath: undefined,
readLink: false,
rootDir: path.join('/', 'project'),
},
],
Expand All @@ -1365,7 +1373,6 @@ describe('FileMap', () => {
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Strawberry.js'),
hasteImplModulePath: undefined,
readLink: false,
rootDir: path.join('/', 'project'),
},
],
Expand All @@ -1377,7 +1384,6 @@ describe('FileMap', () => {
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', '__mocks__', 'Pear.js'),
hasteImplModulePath: undefined,
readLink: false,
rootDir: path.join('/', 'project'),
},
],
Expand All @@ -1389,7 +1395,6 @@ describe('FileMap', () => {
enableHastePackages: true,
filePath: path.join('/', 'project', 'vegetables', 'Melon.js'),
hasteImplModulePath: undefined,
readLink: false,
rootDir: path.join('/', 'project'),
},
],
Expand Down
20 changes: 0 additions & 20 deletions packages/metro-file-map/src/__tests__/worker-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,26 +240,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')
Expand Down
2 changes: 0 additions & 2 deletions packages/metro-file-map/src/flow-types.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,6 @@ export type WorkerMessage = $ReadOnly<{
computeSha1: boolean,
dependencyExtractor?: ?string,
enableHastePackages: boolean,
readLink: boolean,
rootDir: string,
filePath: string,
hasteImplModulePath?: ?string,
Expand All @@ -337,5 +336,4 @@ export type WorkerMetadata = $ReadOnly<{
id?: ?string,
module?: ?HasteMapItemMetaData,
sha1?: ?string,
symlinkTarget?: ?string,
}>;
63 changes: 24 additions & 39 deletions packages/metro-file-map/src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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) => {
Expand All @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 1 addition & 8 deletions packages/metro-file-map/src/worker.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 = {
Expand Down
2 changes: 0 additions & 2 deletions packages/metro-file-map/types/flow-types.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,6 @@ export type WorkerMessage = Readonly<{
computeSha1: boolean;
dependencyExtractor?: string | null;
enableHastePackages: boolean;
readLink: boolean;
rootDir: string;
filePath: string;
hasteImplModulePath?: string | null;
Expand All @@ -315,5 +314,4 @@ export type WorkerMetadata = Readonly<{
id?: string | null;
module?: HasteMapItemMetaData | null;
sha1?: string | null;
symlinkTarget?: string | null;
}>;

0 comments on commit a672bef

Please sign in to comment.