Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 31 additions & 2 deletions packages/rstack/src/fmt/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ interface ParsedFmtCLIArgs {
mode: FmtMode;
patterns: string[];
parallel: boolean;
maxWorkers?: number;
help: boolean;
}

Expand All @@ -26,8 +27,26 @@ ${color.cyan('Options')}:
--check Check whether files are formatted
--list-different Print paths of unformatted files
--no-parallel Disable worker parallelism
--parallel-workers <count> Number of parallel workers
-h, --help Display this help message`;

const parseMaxWorkers = (
kebabValue: string | undefined,
camelValue: string | undefined,
): number | undefined => {
const value = kebabValue ?? camelValue;
if (value === undefined) {
return undefined;
}

const maxWorkers = Number(value);
if (!/^\d+$/.test(value) || !Number.isSafeInteger(maxWorkers) || maxWorkers < 1) {
throw new Error('The --parallel-workers option must be a positive integer.');
}

return maxWorkers;
};

const parseFmtCLIArgs = (args: string[]): ParsedFmtCLIArgs => {
const { values, positionals } = parseArgs({
args,
Expand All @@ -38,6 +57,8 @@ const parseFmtCLIArgs = (args: string[]): ParsedFmtCLIArgs => {
listDifferent: { type: 'boolean' },
'no-parallel': { type: 'boolean' },
noParallel: { type: 'boolean' },
'parallel-workers': { type: 'string' },
parallelWorkers: { type: 'string' },
help: { type: 'boolean', short: 'h' },
},
allowPositionals: true,
Expand All @@ -51,11 +72,18 @@ const parseFmtCLIArgs = (args: string[]): ParsedFmtCLIArgs => {
}

const mode = values.check ? 'check' : listDifferent ? 'list-different' : 'write';
const noParallel = values['no-parallel'] || values.noParallel;
const maxWorkers = parseMaxWorkers(values['parallel-workers'], values.parallelWorkers);

if (noParallel && maxWorkers !== undefined) {
throw new Error('The --parallel-workers and --no-parallel options cannot be used together.');
}

return {
mode,
patterns: positionals,
parallel: !(values['no-parallel'] || values.noParallel),
parallel: !noParallel,
maxWorkers,
help: values.help ?? false,
};
};
Expand Down Expand Up @@ -98,7 +126,7 @@ const logFmtResult = (result: FmtRunResult, mode: FmtMode, cwd: string): void =>
};

const runFmtCLI = async (args: string[]): Promise<void> => {
const { help, mode, parallel, patterns } = parseFmtCLIArgs(args);
const { help, maxWorkers, mode, parallel, patterns } = parseFmtCLIArgs(args);
if (help) {
console.log(fmtHelpMessage);
return;
Expand All @@ -124,6 +152,7 @@ const runFmtCLI = async (args: string[]): Promise<void> => {
mode,
cache: false,
parallel,
maxWorkers,
});

logFmtResult(result, mode, cwd);
Expand Down
10 changes: 5 additions & 5 deletions packages/rstack/src/fmt/parallel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ interface FmtWorker {
terminate: () => void;
}

const getFmtWorkerCount = (fileCount: number): number =>
Math.min(fileCount, Math.max(1, availableParallelism() - 1));
const getFmtWorkerCount = (fileCount: number, maxWorkers?: number): number =>
Math.min(fileCount, maxWorkers ?? Math.max(1, availableParallelism() - 1));

const getFmtWorkerUrl = (): URL => {
// Source tests run after build and exercise the same worker artifact as the CLI.
Expand All @@ -22,8 +22,8 @@ const getFmtWorkerUrl = (): URL => {
};

/** Creates and starts every worker before formatting can begin. */
const createFmtWorker = async (fileCount: number): Promise<FmtWorker> => {
const workerCount = getFmtWorkerCount(fileCount);
const createFmtWorker = async (fileCount: number, maxWorkers?: number): Promise<FmtWorker> => {
const workerCount = getFmtWorkerCount(fileCount, maxWorkers);
const pool = new WorkTank<FmtWorkerMethods>({
pool: {
name: 'rstack-fmt',
Expand Down Expand Up @@ -51,4 +51,4 @@ const createFmtWorker = async (fileCount: number): Promise<FmtWorker> => {
};
};

export { createFmtWorker };
export { createFmtWorker, getFmtWorkerCount };
6 changes: 4 additions & 2 deletions packages/rstack/src/fmt/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ const runFmtFilesSerial = async (
const runFmtFilesParallel = async (
files: FmtFileRequest[],
shouldWrite: boolean,
maxWorkers?: number,
): Promise<FmtFileResult[]> => {
const { createFmtWorker } = await import('./parallel.ts');
const worker = await createFmtWorker(files.length);
const worker = await createFmtWorker(files.length, maxWorkers);

try {
return await Promise.all(files.map((file) => runFmtFile(file, shouldWrite, worker.formatFile)));
Expand Down Expand Up @@ -96,12 +97,13 @@ const runFmtFiles = async ({
files,
mode,
parallel,
maxWorkers,
}: RunFmtFilesOptions): Promise<FmtRunResult> => {
const startTime = performance.now();
const shouldWrite = mode === 'write';
const results =
parallel && files.length > 1 && canRunFmtFilesParallel(files)
? await runFmtFilesParallel(files, shouldWrite)
? await runFmtFilesParallel(files, shouldWrite, maxWorkers)
: await runFmtFilesSerial(files, shouldWrite);

return {
Expand Down
2 changes: 2 additions & 0 deletions packages/rstack/src/fmt/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ interface RunFmtFilesOptions {
cache: false;
/** Whether cloneable file requests should run in worker threads. */
parallel: boolean;
/** Maximum worker count when parallel execution is enabled. */
maxWorkers?: number;
}

interface SuccessfulFmtFileResult {
Expand Down
7 changes: 5 additions & 2 deletions packages/rstack/tests/cli/fmt/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,14 @@ test('formats the current directory with Prettier defaults', () => {
expect(readProjectFile('index.ts')).toBe('const message = "hello";\n');
});

test('supports disabling parallel execution', () => {
test.each([
['disabling parallel execution', ['--no-parallel']],
['configuring parallel worker count', ['--parallel-workers', '1']],
] as const)('supports %s', (_, options) => {
writeProjectFile('first.ts', 'const first="first"');
writeProjectFile('second.ts', 'const second="second"');

const result = runFmt(['--no-parallel', 'first.ts', 'second.ts']);
const result = runFmt([...options, 'first.ts', 'second.ts']);

expect(result.status).toBe(0);
expect(result.stdout).toBe('first.ts\nsecond.ts\n');
Expand Down
50 changes: 44 additions & 6 deletions packages/rstack/tests/fmt/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ test('uses write mode by default', () => {
mode: 'write',
patterns: [],
parallel: true,
maxWorkers: undefined,
help: false,
});
});
Expand All @@ -20,6 +21,7 @@ test.each([
mode,
patterns: [],
parallel: true,
maxWorkers: undefined,
help: false,
});
});
Expand All @@ -29,17 +31,54 @@ test.each(['--no-parallel', '--noParallel'])('disables parallel execution with %
mode: 'write',
patterns: [],
parallel: false,
maxWorkers: undefined,
help: false,
});
});

test.each(['--parallel-workers', '--parallelWorkers'])(
'configures parallel worker count with %s',
(option) => {
expect(parseFmtCLIArgs([option, '3'])).toEqual({
mode: 'write',
patterns: [],
parallel: true,
maxWorkers: 3,
help: false,
});
},
);

test.each(['0', '-1', '1.5', 'invalid', '9007199254740992'])(
'rejects invalid parallel worker count %s',
(count) => {
expect(() => parseFmtCLIArgs([`--parallel-workers=${count}`])).toThrow(
'The --parallel-workers option must be a positive integer.',
);
},
);

test('prefers the kebab-case parallel worker option', () => {
expect(parseFmtCLIArgs(['--parallel-workers', '2', '--parallelWorkers', '3']).maxWorkers).toBe(2);
});

test.each([
['--no-parallel', '--parallel-workers'],
['--noParallel', '--parallelWorkers'],
])('rejects conflicting parallel options: %s and %s', (noParallel, maxWorkersOption) => {
expect(() => parseFmtCLIArgs([noParallel, maxWorkersOption, '2'])).toThrow(
'The --parallel-workers and --no-parallel options cannot be used together.',
);
});

test('preserves file paths and globs', () => {
const patterns = ['src/file with spaces.ts', 'src/**/*.{js,ts}', '!src/generated/**'];

expect(parseFmtCLIArgs([patterns[0], '--check', ...patterns.slice(1)])).toEqual({
mode: 'check',
patterns,
parallel: true,
maxWorkers: undefined,
help: false,
});
});
Expand All @@ -49,6 +88,7 @@ test('treats arguments after the terminator as paths', () => {
mode: 'check',
patterns: ['--write', '--help'],
parallel: true,
maxWorkers: undefined,
help: false,
});
});
Expand All @@ -63,6 +103,7 @@ test('provides command help', () => {
expect(fmtHelpMessage).toContain('--check');
expect(fmtHelpMessage).toContain('--list-different');
expect(fmtHelpMessage).toContain('--no-parallel');
expect(fmtHelpMessage).toContain('--parallel-workers <count>');
expect(fmtHelpMessage).toContain('-h, --help');
});

Expand All @@ -78,9 +119,6 @@ test.each([
);
});

test.each(['--unknown', '--no-cache', '--parallel-workers'])(
'rejects unsupported option %s',
(option) => {
expect(() => parseFmtCLIArgs([option])).toThrow();
},
);
test.each(['--unknown', '--no-cache'])('rejects unsupported option %s', (option) => {
expect(() => parseFmtCLIArgs([option])).toThrow();
});
17 changes: 17 additions & 0 deletions packages/rstack/tests/fmt/parallel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { availableParallelism } from 'node:os';
import { expect, test } from 'rstack/test';
import { getFmtWorkerCount } from '../../src/fmt/parallel.ts';

test('uses one fewer worker than the available parallelism by default', () => {
const defaultWorkerCount = Math.max(1, availableParallelism() - 1);

expect(getFmtWorkerCount(defaultWorkerCount + 1)).toBe(defaultWorkerCount);
});

test.each([
[4, 1, 1],
[4, 2, 2],
[2, 4, 2],
])('uses %s files and %s configured workers as %s workers', (files, workers, expected) => {
expect(getFmtWorkerCount(files, workers)).toBe(expected);
});
18 changes: 16 additions & 2 deletions packages/rstack/tests/fmt/runnerParallelPreflight.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
import { readFileSync } from 'node:fs';
import { expect, rs, test } from 'rstack/test';
import { beforeEach, expect, rs, test } from 'rstack/test';
import { runFmtFiles } from '../../src/fmt/runner.ts';
import type { FmtFileRequest } from '../../src/fmt/types.ts';
import { withTempProject, writeProjectFile } from './helpers.ts';

const mocks = rs.hoisted(() => ({
createFmtWorkerCalls: [] as [number, number | undefined][],
}));

rs.mock('../../src/fmt/parallel.ts', () => ({
createFmtWorker: () => Promise.reject(new Error('worker startup failed')),
createFmtWorker: (fileCount: number, maxWorkers?: number) => {
mocks.createFmtWorkerCalls.push([fileCount, maxWorkers]);
return Promise.reject(new Error('worker startup failed'));
},
}));

beforeEach(() => {
mocks.createFmtWorkerCalls.length = 0;
});

const createRequest = (
filePath: string,
plugins?: FmtFileRequest['options']['plugins'],
Expand All @@ -32,9 +43,12 @@ test('does not write files when worker startup fails', async () => {
mode: 'write',
cache: false,
parallel: true,
maxWorkers: 3,
}),
).rejects.toThrow('worker startup failed');

expect(mocks.createFmtWorkerCalls).toEqual([[2, 3]]);

for (const filePath of filePaths) {
expect(readFileSync(filePath, 'utf8')).toBe('const value=1');
}
Expand Down