Skip to content

Commit

Permalink
Refactor metro-file-map worker
Browse files Browse the repository at this point in the history 
Summary:
Currently, `metro-file-map`'s worker exports a `worker` and a `getSha1` method, which have identical signatures but different behaviour. `worker` could be used for everything, except that it differs from `getSha1` in the following ways:

 - `worker` will check whether the file is a `package.json`, and if not, whether it passes a Haste / `computeDependencies` exclusion list *even if there's no work to be done*.
 - `worker` will re-use a memoised `hasteImpl` even if `hasteImplModulePath` is not given.
 - `worker` is not able to calculate the sha1 of binary files, because it reads contents in `utf8`.

This diff changes things around a bit so that we can consolidate on the one `worker` export - the motivation primarily being to bring in symlink support without adding new code paths in several places.

 - Move lazy loading of `hasteImpl` to where it's required, and use it only if `hasteImplModulePath` is non-null.
 - Have `getContent` return a `Buffer` and decode it where necessary.
 - Don't parse a `package.json` as a Haste package unless new option `enableHastePackages = true`.
 - Don't check `exclusionList` (now `excludedExtensions`) unless there's something to do if the check passes.

All this means we can delete `getSha1` and the `worker` can cover all of our needs - so we'll only have one place to add symlink target reads in the next diff.

Changelog: [Internal]

Reviewed By: jacdebug

Differential Revision: D42537300

fbshipit-source-id: 31faa076c3e7fc6ddf9c3e6a906b0f0796a1be51
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Jan 18, 2023
1 parent 20920b0 commit 51df674
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 74 deletions.
9 changes: 6 additions & 3 deletions packages/metro-file-map/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,9 +649,8 @@ describe('HasteMap', () => {

expect(data.map.get('IRequireAVideo')).toBeDefined();
expect(data.files.get(path.join('video', 'video.mp4'))).toBeDefined();
expect(fs.readFileSync).not.toBeCalledWith(
expect(fs.readFileSync.mock.calls.map(call => call[0])).not.toContain(
path.join('video', 'video.mp4'),
'utf8',
);
});

Expand Down Expand Up @@ -898,7 +897,6 @@ describe('HasteMap', () => {
expect(fs.readFileSync).toHaveBeenCalledTimes(1);
expect(fs.readFileSync).toBeCalledWith(
path.join('/', 'project', 'fruits', 'Banana.js'),
'utf8',
);

expect(deepNormalize(data.clocks)).toEqual(mockClocks);
Expand Down Expand Up @@ -1187,6 +1185,7 @@ describe('HasteMap', () => {
computeDependencies: true,
computeSha1: false,
dependencyExtractor,
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Banana.js'),
hasteImplModulePath: undefined,
rootDir: path.join('/', 'project'),
Expand All @@ -1197,6 +1196,7 @@ describe('HasteMap', () => {
computeDependencies: true,
computeSha1: false,
dependencyExtractor,
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Pear.js'),
hasteImplModulePath: undefined,
rootDir: path.join('/', 'project'),
Expand All @@ -1207,6 +1207,7 @@ describe('HasteMap', () => {
computeDependencies: true,
computeSha1: false,
dependencyExtractor,
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', 'Strawberry.js'),
hasteImplModulePath: undefined,
rootDir: path.join('/', 'project'),
Expand All @@ -1217,6 +1218,7 @@ describe('HasteMap', () => {
computeDependencies: true,
computeSha1: false,
dependencyExtractor,
enableHastePackages: true,
filePath: path.join('/', 'project', 'fruits', '__mocks__', 'Pear.js'),
hasteImplModulePath: undefined,
rootDir: path.join('/', 'project'),
Expand All @@ -1227,6 +1229,7 @@ describe('HasteMap', () => {
computeDependencies: true,
computeSha1: false,
dependencyExtractor,
enableHastePackages: true,
filePath: path.join('/', 'project', 'vegetables', 'Melon.js'),
hasteImplModulePath: undefined,
rootDir: path.join('/', 'project'),
Expand Down
41 changes: 33 additions & 8 deletions packages/metro-file-map/src/__tests__/worker-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@
*/

import H from '../constants';
import {getSha1, worker} from '../worker';
import {worker} from '../worker';
import * as fs from 'graceful-fs';
import * as path from 'path';
import * as vm from 'vm';

jest.mock('graceful-fs', () => {
const path = require('path');
Expand Down Expand Up @@ -110,6 +111,7 @@ describe('worker', () => {
await worker({
computeDependencies: true,
filePath: path.join('/project', 'fruits', 'Strawberry.js'),
hasteImplModulePath: require.resolve('./haste_impl.js'),
rootDir,
}),
).toEqual({
Expand All @@ -119,10 +121,11 @@ describe('worker', () => {
});
});

it('parses package.json files as haste packages', async () => {
it('parses package.json files as haste packages when enableHastePackages=true', async () => {
expect(
await worker({
computeDependencies: true,
enableHastePackages: true,
filePath: path.join('/project', 'package.json'),
rootDir,
}),
Expand All @@ -133,6 +136,21 @@ describe('worker', () => {
});
});

it('does not parse package.json files as haste packages when enableHastePackages=false', async () => {
expect(
await worker({
computeDependencies: true,
enableHastePackages: false,
filePath: path.join('/project', 'package.json'),
rootDir,
}),
).toEqual({
dependencies: undefined,
id: undefined,
module: undefined,
});
});

it('returns an error when a file cannot be accessed', async () => {
let error = null;

Expand All @@ -147,39 +165,39 @@ describe('worker', () => {

it('simply computes SHA-1s when requested (works well with binary data)', async () => {
expect(
await getSha1({
await worker({
computeSha1: true,
filePath: path.join('/project', 'fruits', 'apple.png'),
rootDir,
}),
).toEqual({sha1: '4caece539b039b16e16206ea2478f8c5ffb2ca05'});

expect(
await getSha1({
await worker({
computeSha1: false,
filePath: path.join('/project', 'fruits', 'Banana.js'),
rootDir,
}),
).toEqual({sha1: null});
).toEqual({sha1: undefined});

expect(
await getSha1({
await worker({
computeSha1: true,
filePath: path.join('/project', 'fruits', 'Banana.js'),
rootDir,
}),
).toEqual({sha1: '7772b628e422e8cf59c526be4bb9f44c0898e3d1'});

expect(
await getSha1({
await worker({
computeSha1: true,
filePath: path.join('/project', 'fruits', 'Pear.js'),
rootDir,
}),
).toEqual({sha1: 'c7a7a68a1c8aaf452669dd2ca52ac4a434d25552'});

await expect(
getSha1({computeSha1: true, filePath: '/i/dont/exist.js', rootDir}),
worker({computeSha1: true, filePath: '/i/dont/exist.js', rootDir}),
).rejects.toThrow();
});

Expand All @@ -202,4 +220,11 @@ describe('worker', () => {
expect(fs.readFileSync).not.toHaveBeenCalled();
expect(fs.readFile).not.toHaveBeenCalled();
});

it('can be loaded directly without transpilation', async () => {
const code = await jest
.requireActual('fs')
.promises.readFile(require.resolve('../worker.js'), 'utf8');
expect(() => new vm.Script(code)).not.toThrow();
});
});
1 change: 1 addition & 0 deletions packages/metro-file-map/src/flow-types.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ export type WorkerMessage = $ReadOnly<{
computeDependencies: boolean,
computeSha1: boolean,
dependencyExtractor?: ?string,
enableHastePackages: boolean,
rootDir: string,
filePath: string,
hasteImplModulePath?: ?string,
Expand Down
18 changes: 10 additions & 8 deletions packages/metro-file-map/src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import getPlatformExtension from './lib/getPlatformExtension';
import normalizePathSep from './lib/normalizePathSep';
import HasteModuleMap from './ModuleMap';
import {Watcher} from './Watcher';
import {getSha1, worker} from './worker';
import {worker} from './worker';
import EventEmitter from 'events';
import invariant from 'invariant';
// $FlowFixMe[untyped-import] - jest-regex-util
Expand Down Expand Up @@ -125,7 +125,7 @@ type InternalOptions = {
watchmanDeferStates: $ReadOnlyArray<string>,
};

type WorkerInterface = {worker: typeof worker, getSha1: typeof getSha1};
type WorkerInterface = {worker: typeof worker};

export {default as ModuleMap} from './ModuleMap';
export {DiskCacheManager} from './cache/DiskCacheManager';
Expand Down Expand Up @@ -617,12 +617,13 @@ export default class HasteMap extends EventEmitter {
if (this._options.retainAllFiles && filePath.includes(NODE_MODULES)) {
if (computeSha1) {
return this._getWorker(workerOptions)
.getSha1({
computeDependencies: this._options.computeDependencies,
.worker({
computeDependencies: false,
computeSha1,
dependencyExtractor: this._options.dependencyExtractor,
dependencyExtractor: null,
enableHastePackages: false,
filePath,
hasteImplModulePath: this._options.hasteImplModulePath,
hasteImplModulePath: null,
rootDir,
})
.then(workerReply, workerError);
Expand Down Expand Up @@ -669,6 +670,7 @@ export default class HasteMap extends EventEmitter {
computeDependencies: this._options.computeDependencies,
computeSha1,
dependencyExtractor: this._options.dependencyExtractor,
enableHastePackages: true,
filePath,
hasteImplModulePath: this._options.hasteImplModulePath,
rootDir,
Expand Down Expand Up @@ -782,10 +784,10 @@ export default class HasteMap extends EventEmitter {
_getWorker(options?: {forceInBand: boolean}): WorkerInterface {
if (!this._worker) {
if ((options && options.forceInBand) || this._options.maxWorkers <= 1) {
this._worker = {getSha1, worker};
this._worker = {worker};
} else {
this._worker = new Worker(require.resolve('./worker'), {
exposedMethods: ['getSha1', 'worker'],
exposedMethods: ['worker'],
maxRetries: 3,
numWorkers: this._options.maxWorkers,
});
Expand Down
Loading

0 comments on commit 51df674

Please sign in to comment.