diff --git a/packages/rstack/src/fmt/cli.ts b/packages/rstack/src/fmt/cli.ts index 86514b15..663c73b5 100644 --- a/packages/rstack/src/fmt/cli.ts +++ b/packages/rstack/src/fmt/cli.ts @@ -10,7 +10,6 @@ import type { FmtMode, FmtRunResult } from './types.ts'; interface ParsedFmtCLIArgs { mode: FmtMode; patterns: string[]; - parallel: boolean; maxWorkers?: number; help: boolean; } @@ -26,7 +25,6 @@ ${color.cyan('Options')}: --write Write formatted files in place (default) --check Check whether files are formatted --list-different Print paths of unformatted files - --no-parallel Disable worker parallelism --parallel-workers Number of parallel workers -h, --help Display this help message`; @@ -55,8 +53,6 @@ const parseFmtCLIArgs = (args: string[]): ParsedFmtCLIArgs => { check: { type: 'boolean' }, 'list-different': { type: 'boolean' }, listDifferent: { type: 'boolean' }, - 'no-parallel': { type: 'boolean' }, - noParallel: { type: 'boolean' }, 'parallel-workers': { type: 'string' }, parallelWorkers: { type: 'string' }, help: { type: 'boolean', short: 'h' }, @@ -72,17 +68,11 @@ 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: !noParallel, maxWorkers, help: values.help ?? false, }; @@ -126,7 +116,7 @@ const logFmtResult = (result: FmtRunResult, mode: FmtMode, cwd: string): void => }; const runFmtCLI = async (args: string[]): Promise => { - const { help, maxWorkers, mode, parallel, patterns } = parseFmtCLIArgs(args); + const { help, maxWorkers, mode, patterns } = parseFmtCLIArgs(args); if (help) { console.log(fmtHelpMessage); return; @@ -151,7 +141,6 @@ const runFmtCLI = async (args: string[]): Promise => { files, mode, cache: false, - parallel, maxWorkers, }); diff --git a/packages/rstack/src/fmt/parallel.ts b/packages/rstack/src/fmt/parallel.ts index b15ae9a7..886a0dda 100644 --- a/packages/rstack/src/fmt/parallel.ts +++ b/packages/rstack/src/fmt/parallel.ts @@ -6,7 +6,7 @@ import WorkTank from 'worktank'; type FmtWorkerMethods = typeof import('./worker.ts'); interface FmtWorker { - formatFile: FmtWorkerMethods['formatFileSerial']; + formatFile: FmtWorkerMethods['formatFile']; terminate: () => void; } @@ -46,7 +46,7 @@ const createFmtWorker = async (fileCount: number, maxWorkers?: number): Promise< } return { - formatFile: (file, shouldWrite) => pool.exec('formatFileSerial', [file, shouldWrite]), + formatFile: (file, shouldWrite) => pool.exec('formatFile', [file, shouldWrite]), terminate: pool.terminate, }; }; diff --git a/packages/rstack/src/fmt/runner.ts b/packages/rstack/src/fmt/runner.ts index f1d08049..4b251e07 100644 --- a/packages/rstack/src/fmt/runner.ts +++ b/packages/rstack/src/fmt/runner.ts @@ -5,7 +5,6 @@ import type { FmtRunResult, RunFmtFilesOptions, } from './types.ts'; -import { formatFileSerial } from './serial.ts'; /** Formats one file and reports whether its contents differ. */ type FormatFile = (file: FmtFileRequest, shouldWrite: boolean) => Promise; @@ -36,22 +35,8 @@ const runFmtFile = async ( } }; -/** Processes files sequentially while preserving input order. */ -const runFmtFilesSerial = async ( - files: FmtFileRequest[], - shouldWrite: boolean, -): Promise => { - const results: FmtFileResult[] = []; - - for (const file of files) { - results.push(await runFmtFile(file, shouldWrite, formatFileSerial)); - } - - return results; -}; - -/** Processes files concurrently while preserving input order. */ -const runFmtFilesParallel = async ( +/** Processes files in workers while preserving input order. */ +const runFmtFilesWithWorkers = async ( files: FmtFileRequest[], shouldWrite: boolean, maxWorkers?: number, @@ -66,16 +51,6 @@ const runFmtFilesParallel = async ( } }; -/** Checks whether every request can be cloned for a worker. */ -const canRunFmtFilesParallel = (files: FmtFileRequest[]): boolean => { - try { - structuredClone(files); - return true; - } catch { - return false; - } -}; - /** Maps file results to the Prettier-compatible CLI exit code. */ const getFmtExitCode = (files: FmtFileResult[]): FmtExitCode => { let exitCode: FmtExitCode = 0; @@ -96,15 +71,12 @@ const getFmtExitCode = (files: FmtFileResult[]): FmtExitCode => { const runFmtFiles = async ({ files, mode, - parallel, maxWorkers, }: RunFmtFilesOptions): Promise => { const startTime = performance.now(); const shouldWrite = mode === 'write'; const results = - parallel && files.length > 1 && canRunFmtFilesParallel(files) - ? await runFmtFilesParallel(files, shouldWrite, maxWorkers) - : await runFmtFilesSerial(files, shouldWrite); + files.length === 0 ? [] : await runFmtFilesWithWorkers(files, shouldWrite, maxWorkers); return { files: results, diff --git a/packages/rstack/src/fmt/serial.ts b/packages/rstack/src/fmt/serial.ts deleted file mode 100644 index 1d1804d3..00000000 --- a/packages/rstack/src/fmt/serial.ts +++ /dev/null @@ -1,29 +0,0 @@ -// Derived from @prettier/cli, see THIRD_PARTY_NOTICES.md - -import { readFile, writeFile } from 'atomically'; -import { format } from 'prettier'; -import { getPrettierPlugins } from './prettierPlugins.ts'; -import type { FmtFileRequest } from './types.ts'; - -const formatFileSerial = async ( - { path, options }: FmtFileRequest, - shouldWrite: boolean, -): Promise => { - const source = await readFile(path, 'utf8'); - const formatted = await format(source, { - ...options, - plugins: await getPrettierPlugins(options), - }); - - if (source === formatted) { - return false; - } - - if (shouldWrite) { - await writeFile(path, formatted, 'utf8'); - } - - return true; -}; - -export { formatFileSerial }; diff --git a/packages/rstack/src/fmt/types.ts b/packages/rstack/src/fmt/types.ts index 52865a60..b02fbc3e 100644 --- a/packages/rstack/src/fmt/types.ts +++ b/packages/rstack/src/fmt/types.ts @@ -80,9 +80,7 @@ interface RunFmtFilesOptions { mode: FmtMode; /** Persistent cache support is added in a later implementation step. */ cache: false; - /** Whether cloneable file requests should run in worker threads. */ - parallel: boolean; - /** Maximum worker count when parallel execution is enabled. */ + /** Maximum number of formatting workers. */ maxWorkers?: number; } diff --git a/packages/rstack/src/fmt/worker.ts b/packages/rstack/src/fmt/worker.ts index 8739ff7d..e3ccf483 100644 --- a/packages/rstack/src/fmt/worker.ts +++ b/packages/rstack/src/fmt/worker.ts @@ -1,6 +1,32 @@ -import { formatFileSerial } from './serial.ts'; +// Derived from @prettier/cli, see THIRD_PARTY_NOTICES.md + +import { readFile, writeFile } from 'atomically'; +import { format } from 'prettier'; +import { getPrettierPlugins } from './prettierPlugins.ts'; +import type { FmtFileRequest } from './types.ts'; + +const formatFile = async ( + { path, options }: FmtFileRequest, + shouldWrite: boolean, +): Promise => { + const source = await readFile(path, 'utf8'); + const formatted = await format(source, { + ...options, + plugins: await getPrettierPlugins(options), + }); + + if (source === formatted) { + return false; + } + + if (shouldWrite) { + await writeFile(path, formatted, 'utf8'); + } + + return true; +}; /** Confirms that the worker module and its runtime dependencies are ready. */ const initializeFmtWorker = (): true => true; -export { formatFileSerial, initializeFmtWorker }; +export { formatFile, initializeFmtWorker }; diff --git a/packages/rstack/tests/cli/fmt/index.test.ts b/packages/rstack/tests/cli/fmt/index.test.ts index 0ecea399..37d2fed2 100644 --- a/packages/rstack/tests/cli/fmt/index.test.ts +++ b/packages/rstack/tests/cli/fmt/index.test.ts @@ -113,10 +113,7 @@ test('does not sort package.json by default', () => { ); }); -test.each([ - ['parallel execution', []], - ['serial execution', ['--no-parallel']], -] as const)('sorts package.json with %s', (_, options) => { +test('sorts package.json with workers', () => { writeProjectFile( 'rstack.config.ts', `import { define } from 'rstack'; @@ -127,7 +124,7 @@ define.fmt({ sortPackageJson: true }); writeProjectFile('package.json', packageJsonSource); writeProjectFile('packages/example/package.json', packageJsonSource); - const result = runFmt([...options, 'package.json', 'packages/example/package.json']); + const result = runFmt(['package.json', 'packages/example/package.json']); expect(result.status).toBe(0); expect(result.stderr).toBe(''); @@ -135,14 +132,11 @@ define.fmt({ sortPackageJson: true }); expect(readProjectFile('packages/example/package.json')).toBe(sortedPackageJson); }); -test.each([ - ['disabling parallel execution', ['--no-parallel']], - ['configuring parallel worker count', ['--parallel-workers', '1']], -] as const)('supports %s', (_, options) => { +test('supports configuring the worker count', () => { writeProjectFile('first.ts', 'const first="first"'); writeProjectFile('second.ts', 'const second="second"'); - const result = runFmt([...options, 'first.ts', 'second.ts']); + const result = runFmt(['--parallel-workers', '1', 'first.ts', 'second.ts']); expect(result.status).toBe(0); expect(result.stdout).toBe('first.ts\nsecond.ts\n'); @@ -267,10 +261,7 @@ test('returns exit code 2 for config errors', () => { expect(result.stderr).toContain('invalid fmt config'); }); -test.each([ - ['parallel execution', []], - ['serial execution', ['--no-parallel']], -] as const)('formats with a project-local plugin using %s', (_, options) => { +test('formats with a project-local plugin in workers', () => { writeProjectFile( 'rstack.config.ts', `import { define } from 'rstack'; @@ -284,7 +275,7 @@ define.fmt({ writeProjectFile('first.fixture', '{"first":true}'); writeProjectFile('second.fixture', '{"second":true}'); - const result = runFmt([...options, '*.fixture']); + const result = runFmt(['*.fixture']); expect(result.status).toBe(0); expect(result.stdout).toBe('first.fixture\nsecond.fixture\n'); @@ -293,7 +284,7 @@ define.fmt({ expect(readProjectFile('second.fixture')).toBe('{ "second": true }\n'); }); -test('formats mixed plugin overrides in parallel', () => { +test('formats mixed plugin overrides in workers', () => { writeProjectFile( 'rstack.config.ts', `import { define } from 'rstack'; diff --git a/packages/rstack/tests/fmt/cli.test.ts b/packages/rstack/tests/fmt/cli.test.ts index 8eb45194..3b9cfe40 100644 --- a/packages/rstack/tests/fmt/cli.test.ts +++ b/packages/rstack/tests/fmt/cli.test.ts @@ -5,7 +5,6 @@ test('uses write mode by default', () => { expect(parseFmtCLIArgs([])).toEqual({ mode: 'write', patterns: [], - parallel: true, maxWorkers: undefined, help: false, }); @@ -20,17 +19,6 @@ test.each([ expect(parseFmtCLIArgs([option])).toEqual({ mode, patterns: [], - parallel: true, - maxWorkers: undefined, - help: false, - }); -}); - -test.each(['--no-parallel', '--noParallel'])('disables parallel execution with %s', (option) => { - expect(parseFmtCLIArgs([option])).toEqual({ - mode: 'write', - patterns: [], - parallel: false, maxWorkers: undefined, help: false, }); @@ -42,7 +30,6 @@ test.each(['--parallel-workers', '--parallelWorkers'])( expect(parseFmtCLIArgs([option, '3'])).toEqual({ mode: 'write', patterns: [], - parallel: true, maxWorkers: 3, help: false, }); @@ -62,22 +49,12 @@ 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, }); @@ -87,7 +64,6 @@ test('treats arguments after the terminator as paths', () => { expect(parseFmtCLIArgs(['--check', '--', '--write', '--help'])).toEqual({ mode: 'check', patterns: ['--write', '--help'], - parallel: true, maxWorkers: undefined, help: false, }); @@ -102,7 +78,6 @@ test('provides command help', () => { expect(fmtHelpMessage).toContain('--write'); expect(fmtHelpMessage).toContain('--check'); expect(fmtHelpMessage).toContain('--list-different'); - expect(fmtHelpMessage).toContain('--no-parallel'); expect(fmtHelpMessage).toContain('--parallel-workers '); expect(fmtHelpMessage).toContain('-h, --help'); }); @@ -119,6 +94,9 @@ test.each([ ); }); -test.each(['--unknown', '--no-cache'])('rejects unsupported option %s', (option) => { - expect(() => parseFmtCLIArgs([option])).toThrow(); -}); +test.each(['--unknown', '--no-cache', '--no-parallel', '--noParallel'])( + 'rejects unsupported option %s', + (option) => { + expect(() => parseFmtCLIArgs([option])).toThrow(); + }, +); diff --git a/packages/rstack/tests/fmt/runner.test.ts b/packages/rstack/tests/fmt/runner.test.ts index 52173bcb..edd84ebf 100644 --- a/packages/rstack/tests/fmt/runner.test.ts +++ b/packages/rstack/tests/fmt/runner.test.ts @@ -3,7 +3,7 @@ import path from 'node:path'; import { expect, test } from 'rstack/test'; import { runFmtFiles } from '../../src/fmt/runner.ts'; import type { FmtFileRequest, FmtMode } from '../../src/fmt/types.ts'; -import { withTempProject, writeProjectFile } from './helpers.ts'; +import { withTempProject } from './helpers.ts'; const createRequest = (filePath: string): FmtFileRequest => ({ path: filePath, @@ -13,12 +13,11 @@ const createRequest = (filePath: string): FmtFileRequest => ({ }, }); -const run = (files: FmtFileRequest[], mode: FmtMode = 'write', parallel = false) => +const run = (files: FmtFileRequest[], mode: FmtMode = 'write') => runFmtFiles({ files, mode, cache: false, - parallel, }); test('does not rewrite unchanged files', async () => { @@ -105,37 +104,3 @@ test('continues after a file fails and gives errors exit-code precedence', async expect(readFileSync(validPath, 'utf8')).toBe('const value=1'); }); }); - -test('parallel execution matches serial results and preserves input order', async () => { - await withTempProject(async (rootPath) => { - const sources = [ - ['changed.ts', 'const changed=1'], - ['invalid.ts', 'const invalid = ;'], - ['unchanged.ts', 'const unchanged = 1;\n'], - ] as const; - - const createRequests = (directory: string) => - sources.map(([name, source]) => - createRequest(writeProjectFile(rootPath, path.join(directory, name), source)), - ); - - const serialResult = await run(createRequests('serial')); - const parallelResult = await run(createRequests('parallel'), 'write', true); - - const summarize = (result: typeof serialResult) => - result.files.map((file) => ({ - name: path.basename(file.path), - status: file.status, - error: file.status === 'error' ? String(file.error) : undefined, - })); - - expect(parallelResult.exitCode).toBe(serialResult.exitCode); - expect(summarize(parallelResult)).toEqual(summarize(serialResult)); - - for (const [name] of sources) { - expect(readFileSync(path.join(rootPath, 'parallel', name), 'utf8')).toBe( - readFileSync(path.join(rootPath, 'serial', name), 'utf8'), - ); - } - }); -}); diff --git a/packages/rstack/tests/fmt/runnerParallelPreflight.test.ts b/packages/rstack/tests/fmt/runnerParallelPreflight.test.ts index afcf28ce..18bf2b1f 100644 --- a/packages/rstack/tests/fmt/runnerParallelPreflight.test.ts +++ b/packages/rstack/tests/fmt/runnerParallelPreflight.test.ts @@ -6,7 +6,6 @@ import { withTempProject, writeProjectFile } from './helpers.ts'; const mocks = rs.hoisted(() => ({ createFmtWorkerCalls: [] as [number, number | undefined][], - formatFileSerialCalls: [] as FmtFileRequest[], })); rs.mock('../../src/fmt/parallel.ts', () => ({ @@ -16,79 +15,40 @@ rs.mock('../../src/fmt/parallel.ts', () => ({ }, })); -rs.mock('../../src/fmt/serial.ts', () => ({ - formatFileSerial: (file: FmtFileRequest) => { - mocks.formatFileSerialCalls.push(file); - return Promise.resolve(true); - }, -})); - beforeEach(() => { mocks.createFmtWorkerCalls.length = 0; - mocks.formatFileSerialCalls.length = 0; }); -const createRequest = ( - filePath: string, - plugins?: FmtFileRequest['options']['plugins'], -): FmtFileRequest => ({ +const createRequest = (filePath: string): FmtFileRequest => ({ path: filePath, options: { filepath: filePath, parser: 'typescript', - plugins, }, }); -test('sends plugin URL requests to workers before writing', async () => { +test('starts a worker before formatting a single file', async () => { await withTempProject(async (rootPath) => { - const filePaths = ['first.ts', 'second.ts'].map((name) => - writeProjectFile(rootPath, name, 'const value=1'), - ); + const filePath = writeProjectFile(rootPath, 'index.ts', 'const value=1'); await expect( runFmtFiles({ - files: filePaths.map((filePath) => - createRequest(filePath, ['file:///prettier-plugin-fixture.mjs']), - ), + files: [createRequest(filePath)], mode: 'write', cache: false, - parallel: true, - maxWorkers: 3, + maxWorkers: 1, }), ).rejects.toThrow('worker startup failed'); - expect(mocks.createFmtWorkerCalls).toEqual([[2, 3]]); - expect(mocks.formatFileSerialCalls).toEqual([]); - - for (const filePath of filePaths) { - expect(readFileSync(filePath, 'utf8')).toBe('const value=1'); - } + expect(mocks.createFmtWorkerCalls).toEqual([[1, 1]]); + expect(readFileSync(filePath, 'utf8')).toBe('const value=1'); }); }); -test('uses serial execution when options cannot be cloned', async () => { - await withTempProject(async (rootPath) => { - const filePaths = ['first.ts', 'second.ts'].map((name) => - writeProjectFile(rootPath, name, 'const value=1'), - ); - const files = filePaths.map((filePath) => createRequest(filePath)); - for (const file of files) { - Object.assign(file.options, { customOption() {} }); - } - - const result = await runFmtFiles({ - files, - mode: 'write', - cache: false, - parallel: true, - }); - - expect(result.files.map((file) => file.status)).toEqual(['written', 'written']); - expect(mocks.createFmtWorkerCalls).toEqual([]); - expect(mocks.formatFileSerialCalls).toEqual(files); - for (const filePath of filePaths) { - expect(readFileSync(filePath, 'utf8')).toBe('const value=1'); - } +test('does not start a worker when there are no files', async () => { + await expect(runFmtFiles({ files: [], mode: 'write', cache: false })).resolves.toMatchObject({ + files: [], + exitCode: 0, }); + expect(mocks.createFmtWorkerCalls).toEqual([]); }); diff --git a/packages/rstack/tests/fmt/runnerWriteFailure.test.ts b/packages/rstack/tests/fmt/runnerWriteFailure.test.ts index 88197fd2..35fe3926 100644 --- a/packages/rstack/tests/fmt/runnerWriteFailure.test.ts +++ b/packages/rstack/tests/fmt/runnerWriteFailure.test.ts @@ -1,9 +1,18 @@ import { expect, rs, test } from 'rstack/test'; import { runFmtFiles } from '../../src/fmt/runner.ts'; -rs.mock('atomically', () => ({ - readFile: () => Promise.resolve('const value=1'), - writeFile: () => Promise.reject(new Error('atomic write failed')), +const mocks = rs.hoisted(() => ({ + terminateCalls: 0, +})); + +rs.mock('../../src/fmt/parallel.ts', () => ({ + createFmtWorker: () => + Promise.resolve({ + formatFile: () => Promise.reject(new Error('atomic write failed')), + terminate: () => { + mocks.terminateCalls++; + }, + }), })); test('returns an error when the atomic write fails', async () => { @@ -21,7 +30,6 @@ test('returns an error when the atomic write fails', async () => { ], mode: 'write', cache: false, - parallel: false, }); expect(result).toMatchObject({ @@ -34,4 +42,5 @@ test('returns an error when the atomic write fails', async () => { }, ], }); + expect(mocks.terminateCalls).toBe(1); });