diff --git a/change/change-21b37dad-e52f-40ef-937e-eb775c98f1e9.json b/change/change-21b37dad-e52f-40ef-937e-eb775c98f1e9.json new file mode 100644 index 00000000..5aae2feb --- /dev/null +++ b/change/change-21b37dad-e52f-40ef-937e-eb775c98f1e9.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "packageName": "just-scripts", + "type": "minor", + "dependentChangeType": "patch", + "comment": "Replace `run-parallel-limit` with `p-limit`, and use native promises in `cleanTask`, `copyTask`, and `sassTask`", + "email": "198982749+Copilot@users.noreply.github.com" + } + ] +} diff --git a/packages/just-scripts/package.json b/packages/just-scripts/package.json index af07fa69..c43a90a4 100644 --- a/packages/just-scripts/package.json +++ b/packages/just-scripts/package.json @@ -31,7 +31,7 @@ "glob": "^13.0.6", "just-task": "workspace:^", "nano-spawn": "catalog:prod", - "run-parallel-limit": "^1.0.6", + "p-limit": "^7.3.1", "supports-color": "^8.1.0", "webpack-merge": "^6.0.1" }, @@ -181,7 +181,6 @@ "devDependencies": { "@microsoft/api-extractor": "catalog:dev", "@microsoft/just-internal-scripts": "workspace:^", - "@types/run-parallel-limit": "^1.0.0", "@types/supports-color": "^8.1.1", "async-done": "^2.0.0", "autoprefixer": "catalog:dev", diff --git a/packages/just-scripts/src/tasks/__tests__/cleanTask.spec.ts b/packages/just-scripts/src/tasks/__tests__/cleanTask.spec.ts index 6b9fd803..1abeb831 100644 --- a/packages/just-scripts/src/tasks/__tests__/cleanTask.spec.ts +++ b/packages/just-scripts/src/tasks/__tests__/cleanTask.spec.ts @@ -7,10 +7,7 @@ jest.mock('just-task/lib/logger'); // Mock fse.remove to track calls without actual filesystem operations jest.mock('fs-extra', () => ({ - remove: jest.fn((_path, cb: (err: null) => void) => { - cb(null); - return Promise.resolve(); - }), + remove: jest.fn(() => Promise.resolve()), })); const removeSpy = fse.remove as jest.MockedFunction; diff --git a/packages/just-scripts/src/tasks/__tests__/copyTask.spec.ts b/packages/just-scripts/src/tasks/__tests__/copyTask.spec.ts index 42c8680c..e32af294 100644 --- a/packages/just-scripts/src/tasks/__tests__/copyTask.spec.ts +++ b/packages/just-scripts/src/tasks/__tests__/copyTask.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, jest, afterEach } from '@jest/globals'; +import { beforeAll, describe, expect, it, jest, afterEach } from '@jest/globals'; import mockfs from 'mock-fs'; import fse from 'fs-extra'; import { Readable } from 'stream'; @@ -13,6 +13,12 @@ jest.mock('just-task/lib/logger'); const itWindows = process.platform === 'win32' ? it : it.skip; describe('copyTask', () => { + beforeAll(async () => { + // Load the ESM-only `p-limit` used by copyTask up front, since it can't be loaded from disk + // while mock-fs is active. + await import('p-limit'); + }); + afterEach(() => { mockfs.restore(); // a couple tests mock additional functions diff --git a/packages/just-scripts/src/tasks/cleanTask.ts b/packages/just-scripts/src/tasks/cleanTask.ts index 154360e8..d56d3232 100644 --- a/packages/just-scripts/src/tasks/cleanTask.ts +++ b/packages/just-scripts/src/tasks/cleanTask.ts @@ -2,7 +2,6 @@ import fse from 'fs-extra'; import path from 'path'; import type { TaskFunction } from 'just-task'; import { logger } from 'just-task'; -import parallelLimit from 'run-parallel-limit'; export interface CleanTaskOptions { /** @@ -24,16 +23,13 @@ export function defaultCleanPaths(): string[] { export function cleanTask(options?: CleanTaskOptions): TaskFunction { const { paths = defaultCleanPaths(), limit = 5 } = options || {}; - return function clean(done: (err: Error | null) => void) { + return async function clean() { logger.info(`Removing [${paths.map(p => path.relative(process.cwd(), p)).join(', ')}]`); - const cleanTasks = paths.map( - cleanPath => - function (cb: (error: Error | null) => void) { - fse.remove(cleanPath, cb); - }, - ); + // p-limit is ESM and must be async imported from CJS + const pLimit = (await import('p-limit')).default; + const limiter = pLimit(limit); - parallelLimit(cleanTasks, limit, done); + await Promise.all(paths.map(cleanPath => limiter(() => fse.remove(cleanPath)))); }; } diff --git a/packages/just-scripts/src/tasks/copyTask.ts b/packages/just-scripts/src/tasks/copyTask.ts index ee0bd0d8..0ca1aaad 100644 --- a/packages/just-scripts/src/tasks/copyTask.ts +++ b/packages/just-scripts/src/tasks/copyTask.ts @@ -1,10 +1,9 @@ import { globSync, hasMagic } from 'glob'; import fse from 'fs-extra'; import path from 'path'; -import { pipeline } from 'stream'; +import { pipeline } from 'stream/promises'; import type { TaskFunction } from 'just-task'; import { logger } from 'just-task'; -import parallelLimit from 'run-parallel-limit'; export interface CopyTaskOptions { /** @@ -38,9 +37,8 @@ export interface CopyTaskOptions { export function copyTask(options: CopyTaskOptions): TaskFunction { const { paths, dest, limit = 15 } = options; - return function copy(done) { + return async function copy() { if (!paths?.length) { - done(); return; } @@ -54,13 +52,30 @@ export function copyTask(options: CopyTaskOptions): TaskFunction { fse.mkdirpSync(dest); - const copyTasks: parallelLimit.Task[] = []; + // Source/destination pairs for all files to copy, collected before starting any copies + const filesToCopy: { src: string; dest: string }[] = []; for (const copyPath of normalizedPaths) { helper(copyPath, getBasePath(copyPath)); } - parallelLimit(copyTasks, limit, done); + // p-limit is ESM and must be async imported from CJS + const pLimit = (await import('p-limit')).default; + const limiter = pLimit(limit); + + await Promise.all( + filesToCopy.map(({ src, dest: destPath }) => + limiter(async () => { + fse.mkdirpSync(path.dirname(destPath)); + + // Use `pipeline` rather than wiring up `pipe`/`end`/`error` manually: it resolves only + // after the destination has been fully flushed and closed (not merely when the source + // finishes reading), and destroys both streams on error so no file descriptors or + // partial destination files are leaked. + await pipeline(fse.createReadStream(src), fse.createWriteStream(destPath)); + }), + ), + ); function helper(srcGlob: string, basePath: string) { // Return absolute paths to ensure path.relative(basePath, matchedPath) works @@ -86,17 +101,7 @@ export function copyTask(options: CopyTaskOptions): TaskFunction { const relativePath = path.relative(basePath, matchedPath); - copyTasks.push(cb => { - const destPath = path.join(dest, relativePath); - - fse.mkdirpSync(path.dirname(destPath)); - - // Use `pipeline` rather than wiring up `pipe`/`end`/`error` manually: it invokes the - // callback exactly once, only after the destination has been fully flushed and closed - // (not merely when the source finishes reading), and destroys both streams on error so - // no file descriptors or partial destination files are leaked. - pipeline(fse.createReadStream(matchedPath), fse.createWriteStream(destPath), err => cb(err || null)); - }); + filesToCopy.push({ src: matchedPath, dest: path.join(dest, relativePath) }); } } }; diff --git a/packages/just-scripts/src/tasks/sassTask.ts b/packages/just-scripts/src/tasks/sassTask.ts index 0544a6c8..6eacc4a0 100644 --- a/packages/just-scripts/src/tasks/sassTask.ts +++ b/packages/just-scripts/src/tasks/sassTask.ts @@ -3,7 +3,6 @@ import { globSync } from 'glob'; import { logger, resolveCwd, type TaskFunction } from 'just-task'; import path from 'path'; import type { AcceptedPlugin } from 'postcss'; -import parallelLimit from 'run-parallel-limit'; import { pathToFileURL } from 'url'; import { tryRequire } from '../tryRequire'; @@ -25,7 +24,7 @@ export interface SassTaskOptions { export function sassTask(options: SassTaskOptions): TaskFunction { const { createSourceModule, postcssPlugins = [] } = options; - return function sass(done) { + return async function sass() { const sassModule = tryRequire('sass') || tryRequire('node-sass'); const postcss = tryRequire('postcss'); const autoprefixer = tryRequire('autoprefixer'); @@ -42,64 +41,64 @@ export function sassTask(options: SassTaskOptions): TaskFunction { .filter(Boolean) .join(', '); logger.warn(`Required dependencies not found (${missing}), so this task has no effect.`); - done(); return; } const autoprefixerFn = autoprefixer({ overrideBrowserslist: ['> 1%', 'last 2 versions', 'ie >= 11'] }); const files = globSync('src/**/*.scss', { absolute: true, cwd: process.cwd() }); - const tasks: parallelLimit.Task[] = files.map(fileName => cb => { - fileName = path.resolve(fileName); - - // The modern `compile()` API is available in `sass` but not in `node-sass` - if (typeof sassModule.compile === 'function') { - try { - const { css } = sassModule.compile(fileName, { - importers: [{ findFileUrl: patchSassFileUrl }], - loadPaths: [path.resolve(process.cwd(), 'node_modules')], - }); - processCss(css); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - cb(new Error(`${path.relative(process.cwd(), fileName)}: ${message}`)); - } - } else { - sassModule.render( - { - file: fileName, - importer: patchSassUrl, - includePaths: [path.resolve(process.cwd(), 'node_modules')], - }, - (err, result) => { - if (err || !result) { - cb(new Error(`${path.relative(process.cwd(), fileName)}: ${err || 'no result returned'}`)); - return; + // p-limit is ESM and must be async imported from CJS + const pLimit = (await import('p-limit')).default; + const limiter = pLimit(5); + + await Promise.all( + files.map(file => + limiter(async () => { + const fileName = path.resolve(file); + + let css: string; + // The modern `compile()` API is available in `sass` but not in `node-sass` + if (typeof sassModule.compile === 'function') { + try { + css = sassModule.compile(fileName, { + importers: [{ findFileUrl: patchSassFileUrl }], + loadPaths: [path.resolve(process.cwd(), 'node_modules')], + }).css; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`${path.relative(process.cwd(), fileName)}: ${message}`, { cause: err }); } - - processCss(result.css.toString()); - }, - ); - } - - function processCss(css: string) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- always defined per above - postcss!([ - autoprefixerFn, - ...(postcssRtl ? [postcssRtl({})] : []), - ...postcssPlugins, - ...(clean ? [clean()] : []), - ]) - .process(css, { from: fileName }) - .then(res => { - fs.writeFileSync(fileName + '.ts', createSourceModule(fileName, res.css)); - cb(null); - }) - .catch(e => cb(e instanceof Error ? e : new Error(String(e)))); - } - }); - - parallelLimit(tasks, 5, done); + } else { + // The legacy `render()` API is callback-based, so it must be promisified + css = await new Promise((resolve, reject) => { + sassModule.render( + { + file: fileName, + importer: patchSassUrl, + includePaths: [path.resolve(process.cwd(), 'node_modules')], + }, + (err, result) => { + if (err || !result) { + reject(new Error(`${path.relative(process.cwd(), fileName)}: ${err || 'no result returned'}`)); + } else { + resolve(result.css.toString()); + } + }, + ); + }); + } + + const res = await postcss([ + autoprefixerFn, + ...(postcssRtl ? [postcssRtl({})] : []), + ...postcssPlugins, + ...(clean ? [clean()] : []), + ]).process(css, { from: fileName }); + + fs.writeFileSync(fileName + '.ts', createSourceModule(fileName, res.css)); + }), + ), + ); }; } diff --git a/yarn.lock b/yarn.lock index f1759995..d7cf0743 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2217,13 +2217,6 @@ __metadata: languageName: node linkType: hard -"@types/run-parallel-limit@npm:^1.0.0": - version: 1.0.3 - resolution: "@types/run-parallel-limit@npm:1.0.3" - checksum: 10c0/499bf6640cf0c9ed6a20922117a247bcdaff8427ebde7f8237333f9966a82affe8504ae9c846193d4d19127be6aa291a10a8221b389a2bc47de6295c49146538 - languageName: node - linkType: hard - "@types/send@npm:*": version: 1.2.1 resolution: "@types/send@npm:1.2.1" @@ -6444,7 +6437,6 @@ __metadata: dependencies: "@microsoft/api-extractor": "catalog:dev" "@microsoft/just-internal-scripts": "workspace:^" - "@types/run-parallel-limit": "npm:^1.0.0" "@types/supports-color": "npm:^8.1.1" "@types/tar-fs": "npm:^2.0.4" async-done: "npm:^2.0.0" @@ -6461,8 +6453,8 @@ __metadata: just-task: "workspace:^" mock-fs: "catalog:dev" nano-spawn: "catalog:prod" + p-limit: "npm:^7.3.1" postcss: "catalog:dev" - run-parallel-limit: "npm:^1.0.6" sass: "catalog:dev" supports-color: "npm:^8.1.0" tar-fs: "catalog:dev" @@ -7367,6 +7359,15 @@ __metadata: languageName: node linkType: hard +"p-limit@npm:^7.3.1": + version: 7.3.1 + resolution: "p-limit@npm:7.3.1" + dependencies: + yocto-queue: "npm:^1.2.1" + checksum: 10c0/49b7d7fcc244d4659fa3b260c7b4e632bc72333c60e76e4cd24c7befdeb9b8e22da644e5064e83df9e2ded75245e3e3b9df0e9cf8500b08db8ebf58ac18c6764 + languageName: node + linkType: hard + "p-locate@npm:^4.1.0": version: 4.1.0 resolution: "p-locate@npm:4.1.0" @@ -8053,15 +8054,6 @@ __metadata: languageName: node linkType: hard -"run-parallel-limit@npm:^1.0.6": - version: 1.1.0 - resolution: "run-parallel-limit@npm:1.1.0" - dependencies: - queue-microtask: "npm:^1.2.2" - checksum: 10c0/9c78eb77e788d0ed803a7e80921412f6f6accfb2006de8c21699d9ebf7696df9cefaa313fe14d6169a3fc9f564b34fe91bfd9948cc3a58e2d24136a2390523ae - languageName: node - linkType: hard - "run-parallel@npm:^1.1.9": version: 1.2.0 resolution: "run-parallel@npm:1.2.0" @@ -9753,3 +9745,10 @@ __metadata: checksum: 10c0/dceb44c28578b31641e13695d200d34ec4ab3966a5729814d5445b194933c096b7ced71494ce53a0e8820685d1d010df8b2422e5bf2cdea7e469d97ffbea306f languageName: node linkType: hard + +"yocto-queue@npm:^1.2.1": + version: 1.2.2 + resolution: "yocto-queue@npm:1.2.2" + checksum: 10c0/36d4793e9cf7060f9da543baf67c55e354f4862c8d3d34de1a1b1d7c382d44171315cc54abf84d8900b8113d742b830108a1434f4898fb244f9b7e8426d4b8f5 + languageName: node + linkType: hard