Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(jest-haste-map): clean up code and types #12795

Merged
merged 2 commits into from
May 3, 2022
Merged

refactor(jest-haste-map): clean up code and types #12795

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35cde42
refactor(jest-haste-map): clean up code and types
mrazauskas May 2, 2022
7e3e40b
revert !!
mrazauskas May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ module.exports = {
'packages/expect-utils/src/utils.ts',
'packages/jest-core/src/collectHandles.ts',
'packages/jest-core/src/plugins/UpdateSnapshotsInteractive.ts',
'packages/jest-haste-map/src/index.ts',
'packages/jest-haste-map/src/watchers/FSEventsWatcher.ts',
'packages/jest-jasmine2/src/jasmine/SpyStrategy.ts',
'packages/jest-jasmine2/src/jasmine/Suite.ts',
'packages/jest-leak-detector/src/index.ts',
Expand Down
51 changes: 21 additions & 30 deletions packages/jest-haste-map/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* LICENSE file in the root directory of this source tree.
*/

/* eslint-disable local/ban-types-eventually */

import {createHash} from 'crypto';
import {EventEmitter} from 'events';
import {tmpdir} from 'os';
Expand Down Expand Up @@ -40,6 +38,7 @@ import type {
HasteMap as InternalHasteMapObject,
MockData,
ModuleMapData,
ModuleMapItem,
ModuleMetaData,
SerializableModuleMap,
WorkerMetadata,
Expand Down Expand Up @@ -212,14 +211,14 @@ function invariant(condition: unknown, message?: string): asserts condition {
*
*/
export default class HasteMap extends EventEmitter {
private _buildPromise: Promise<InternalHasteMapObject> | null;
private _cachePath: string;
private _buildPromise: Promise<InternalHasteMapObject> | null = null;
private _cachePath = '';
private _changeInterval?: ReturnType<typeof setInterval>;
private _console: Console;
private _isWatchmanInstalledPromise: Promise<boolean> | null = null;
private _options: InternalOptions;
private _watchers: Array<Watcher>;
private _worker: JestWorkerFarm<HasteWorker> | HasteWorker | null;
private _watchers: Array<Watcher> = [];
private _worker: JestWorkerFarm<HasteWorker> | HasteWorker | null = null;

static getStatic(config: Config.ProjectConfig): HasteMapStatic {
if (config.haste.hasteMapModulePath) {
Expand All @@ -244,15 +243,12 @@ export default class HasteMap extends EventEmitter {
super();
this._options = {
cacheDirectory: options.cacheDirectory || tmpdir(),
computeDependencies:
options.computeDependencies === undefined
? true
: options.computeDependencies,
computeDependencies: options.computeDependencies ?? true,
computeSha1: options.computeSha1 || false,
dependencyExtractor: options.dependencyExtractor || null,
enableSymlinks: options.enableSymlinks || false,
extensions: options.extensions,
forceNodeFilesystemAPI: !!options.forceNodeFilesystemAPI,
forceNodeFilesystemAPI: options.forceNodeFilesystemAPI || false,
hasteImplModulePath: options.hasteImplModulePath,
id: options.id,
maxWorkers: options.maxWorkers,
Expand All @@ -264,10 +260,10 @@ export default class HasteMap extends EventEmitter {
retainAllFiles: options.retainAllFiles,
rootDir: options.rootDir,
roots: Array.from(new Set(options.roots)),
skipPackageJson: !!options.skipPackageJson,
throwOnModuleCollision: !!options.throwOnModuleCollision,
useWatchman: options.useWatchman == null ? true : options.useWatchman,
watch: !!options.watch,
skipPackageJson: options.skipPackageJson || false,
throwOnModuleCollision: options.throwOnModuleCollision || false,
useWatchman: options.useWatchman ?? true,
watch: options.watch || false,
mrazauskas marked this conversation as resolved.
Show resolved Hide resolved
};
this._console = options.console || globalThis.console;

Expand All @@ -293,11 +289,6 @@ export default class HasteMap extends EventEmitter {
'Set either `enableSymlinks` to false or `useWatchman` to false.',
);
}

this._cachePath = '';
this._buildPromise = null;
this._watchers = [];
this._worker = null;
}

private async setupCachePath(options: Options): Promise<void> {
Expand Down Expand Up @@ -444,7 +435,7 @@ export default class HasteMap extends EventEmitter {
let hasteMap: InternalHasteMap;
try {
const read = this._options.resetCache ? this._createEmptyMap : this.read;
hasteMap = await read.call(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await does nothing here.

hasteMap = read.call(this);
} catch {
hasteMap = this._createEmptyMap();
}
Expand All @@ -466,7 +457,7 @@ export default class HasteMap extends EventEmitter {
const setModule = (id: string, module: ModuleMetaData) => {
let moduleMap = map.get(id);
if (!moduleMap) {
moduleMap = Object.create(null) as {};
moduleMap = Object.create(null) as ModuleMapItem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to cast? isn't moduleMap already the correct type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not work without casting. It needs some type, because Object.create() returns any.

map.set(id, moduleMap);
}
const platform =
Expand Down Expand Up @@ -646,7 +637,7 @@ export default class HasteMap extends EventEmitter {
const moduleId = fileMetadata[H.ID];
let modulesByPlatform = map.get(moduleId);
if (!modulesByPlatform) {
modulesByPlatform = Object.create(null) as {};
modulesByPlatform = Object.create(null) as ModuleMapItem;
map.set(moduleId, modulesByPlatform);
}
modulesByPlatform[platform] = module;
Expand Down Expand Up @@ -729,7 +720,7 @@ export default class HasteMap extends EventEmitter {
private _cleanup() {
const worker = this._worker;

if (worker && 'end' in worker && typeof worker.end === 'function') {
if (worker && 'end' in worker) {
worker.end();
}

Expand All @@ -746,11 +737,11 @@ export default class HasteMap extends EventEmitter {
/**
* Creates workers or parses files and extracts metadata in-process.
*/
private _getWorker(options?: {
forceInBand: boolean;
}): JestWorkerFarm<HasteWorker> | HasteWorker {
private _getWorker(
options = {forceInBand: false},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false, not boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to have default value. options object always has some value, so there is no need for options && check later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, right, misread

): JestWorkerFarm<HasteWorker> | HasteWorker {
if (!this._worker) {
if ((options && options.forceInBand) || this._options.maxWorkers <= 1) {
if (options.forceInBand || this._options.maxWorkers <= 1) {
this._worker = {getSha1, worker};
} else {
this._worker = new Worker(require.resolve('./worker'), {
Expand Down Expand Up @@ -1070,8 +1061,8 @@ export default class HasteMap extends EventEmitter {

let dedupMap = hasteMap.map.get(moduleName);

if (dedupMap == null) {
dedupMap = Object.create(null) as {};
if (!dedupMap) {
dedupMap = Object.create(null) as ModuleMapItem;
Copy link
Contributor Author

@mrazauskas mrazauskas May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like similar cases above, here dedupMap does not have value yet, so {} is somewhat fine. But the value gets assigned just a couple of lines later. So it makes sense to type it with correct shape already here.

hasteMap.map.set(moduleName, dedupMap);
}
dedupMap[platform] = uniqueModule;
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-haste-map/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export type RawModuleMap = {
mocks: MockData;
};

type ModuleMapItem = {[platform: string]: ModuleMetaData};
export type ModuleMapItem = {[platform: string]: ModuleMetaData};
export type ModuleMetaData = [path: string, type: number];

export type HType = {
Expand Down
20 changes: 9 additions & 11 deletions packages/jest-haste-map/src/watchers/FSEventsWatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
*
*/

/* eslint-disable local/ban-types-eventually */

import {EventEmitter} from 'events';
import * as path from 'path';
import anymatch, {Matcher} from 'anymatch';
Expand Down Expand Up @@ -41,13 +39,13 @@ type FsEventsWatcherEvent =
* Watches `dir`.
*/
export class FSEventsWatcher extends EventEmitter {
public readonly root: string;
public readonly ignored?: Matcher;
public readonly glob: Array<string>;
public readonly dot: boolean;
public readonly hasIgnore: boolean;
public readonly doIgnore: (path: string) => boolean;
public readonly fsEventsWatchStopper: () => Promise<void>;
readonly root: string;
readonly ignored?: Matcher;
readonly glob: Array<string>;
readonly dot: boolean;
readonly hasIgnore: boolean;
readonly doIgnore: (path: string) => boolean;
readonly fsEventsWatchStopper: () => Promise<void>;
private _tracked: Set<string>;

static isSupported(): boolean {
Expand All @@ -65,8 +63,8 @@ export class FSEventsWatcher extends EventEmitter {
dir: string,
dirCallback: (normalizedPath: string, stats: fs.Stats) => void,
fileCallback: (normalizedPath: string, stats: fs.Stats) => void,
endCallback: Function,
errorCallback: Function,
endCallback: () => void,
errorCallback: () => void,
ignored?: Matcher,
) {
walker(dir)
Expand Down