diff --git a/.changeset/nothing-sync-on-the-build-thread.md b/.changeset/nothing-sync-on-the-build-thread.md new file mode 100644 index 00000000..f22e6842 --- /dev/null +++ b/.changeset/nothing-sync-on-the-build-thread.md @@ -0,0 +1,5 @@ +--- +"diagnostics-webpack-plugin": patch +--- + +Walk the file system for a check's files without blocking the thread webpack builds on, and read the tree once where two compilers want the same walk. diff --git a/src/checks/typescript-program.js b/src/checks/typescript-program.js index 08f03d4f..feadc50f 100644 --- a/src/checks/typescript-program.js +++ b/src/checks/typescript-program.js @@ -63,6 +63,8 @@ function getHeld(compiler, id) { } /** + * Read synchronously because TypeScript's own host is: the compiler calls + * `getSourceFile` and waits for a source file, with nothing to await into. * @param {string} file the file to read the state of * @returns {string | undefined} what tells one write of it from the next */ diff --git a/src/index.js b/src/index.js index bae584b6..a51c0122 100644 --- a/src/index.js +++ b/src/index.js @@ -1,9 +1,10 @@ -import { readdirSync } from "node:fs"; +import { readdir } from "node:fs/promises"; import { isAbsolute, join, normalize, relative } from "node:path"; import picomatch from "picomatch"; -import { globSync } from "tinyglobby"; +import { glob } from "tinyglobby"; +import DiagnosticError from "./DiagnosticError.js"; import createCheckRunner from "./check.js"; import { getOptions, reportedAs, validateOptions } from "./options.js"; import { @@ -46,22 +47,45 @@ const EARLY_BATCH = 64; let compilerId = 0; +// A walk two compilers are making at the same moment over the same patterns, +// so that the tree is read once and both of them go on together — which is +// what leaves their checks a run to share. +/** @type {Map>} */ +const walks = new Map(); + +/** + * @param {string[]} wanted the globs of the files to walk for + * @param {string[]} exclude the globs of the files to leave out + * @returns {Promise} what the tree holds of them + */ +function walk(wanted, exclude) { + const key = JSON.stringify([wanted, exclude]); + const running = walks.get(key); + + if (running) return running; + + const started = glob(wanted, { absolute: true, dot: true, ignore: exclude }); + const forget = () => walks.delete(key); + + walks.set(key, started); + started.then(forget, forget); + + return started; +} + /** * Walks the file system for the files a check wants, and says which of them * webpack has just seen change. * @param {Compiler} compiler compiler * @param {ResolvedCheck} check the check to collect the files of - * @returns {{ lint: string[], keep: string[] }} the files to lint, and the ones to report from the last compilation + * @returns {Promise<{ lint: string[], keep: string[] }>} the files to lint, and the ones to report from the last compilation */ -function collectFromFileSystem(compiler, { adapter, wanted, exclude }) { +async function collectFromFileSystem(compiler, { adapter, wanted, exclude }) { + // Read before the walk, which is what the build goes on building through. + const { modifiedFiles } = compiler; // The walk is what says which files there are: one webpack never built is // one it cannot report as added, changed or gone either. - const found = globSync(wanted, { - absolute: true, - dot: true, - ignore: exclude, - }); - const { modifiedFiles } = compiler; + const found = await walk(wanted, exclude); // A check that cannot say which file a result came from has nothing to report // a file it was not given from, so it is given all of them every time. One @@ -132,15 +156,15 @@ function globRoots({ filesSource, wanted }) { * @param {Compiler} compiler compiler * @param {ResolvedCheck} check the check that reads it * @param {string[]} writes the paths the check itself writes - * @returns {boolean} whether the whole of it can be watched + * @returns {Promise} whether the whole of it can be watched */ -function canWatch(directory, compiler, check, writes) { +async function canWatch(directory, compiler, check, writes) { const written = [compiler.outputPath, ...writes]; if (written.some((path) => contains(directory, path))) return false; try { - for (const entry of readdirSync(directory, { withFileTypes: true })) { + for (const entry of await readdir(directory, { withFileTypes: true })) { if ( entry.isDirectory() && check.isExcluded(join(directory, entry.name)) @@ -199,7 +223,7 @@ class DiagnosticsWebpackPlugin { validateOptions(compiler, this.given, this.options.checks); }); - /** @type {ResolvedCheck[] | undefined} */ + /** @type {Promise | undefined} */ let checks; // Resolved on the first build rather than here, so that an option the @@ -208,8 +232,10 @@ class DiagnosticsWebpackPlugin { if (!checks) { const context = this.getContext(compiler); - checks = this.options.checks.map((check) => - this.resolveCheck(compiler, context, check), + checks = Promise.all( + this.options.checks.map((check) => + this.resolveCheck(compiler, context, check), + ), ); } @@ -218,22 +244,22 @@ class DiagnosticsWebpackPlugin { // A build is nothing but a first compilation, so `lintOnStart` cannot // silence one without silencing the plugin. - compiler.hooks.run.tapPromise(this.key, (compiler) => - this.run(compiler, getChecks()), + compiler.hooks.run.tapPromise(this.key, async (compiler) => + this.run(compiler, await getChecks()), ); // A lint integration whose bundler reaches a file only once something // requests it defaults this off; webpack's first build walks all of them. let skipping = !this.options.lintOnStart; - compiler.hooks.watchRun.tapPromise(this.key, (compiler) => { + compiler.hooks.watchRun.tapPromise(this.key, async (compiler) => { if (skipping) { skipping = false; - return Promise.resolve(); + return; } - return this.run(compiler, getChecks()); + return this.run(compiler, await getChecks()); }); } @@ -241,9 +267,9 @@ class DiagnosticsWebpackPlugin { * @param {Compiler} compiler compiler * @param {string} context context * @param {EnabledCheck} check the check to resolve the globs of - * @returns {ResolvedCheck} the check with its globs resolved + * @returns {Promise} the check with its globs resolved */ - resolveCheck(compiler, context, { id, name, adapter, options }) { + async resolveCheck(compiler, context, { id, name, adapter, options }) { const resourceQueries = arrify(options.resourceQueryExclude || []); /** @type {CheckOptions} */ @@ -265,13 +291,13 @@ class DiagnosticsWebpackPlugin { ), }; - const wanted = parseFoldersToGlobs( - /** @type {string[]} */ (resolved.files), - resolved.extensions, - ); - const exclude = parseFoldersToGlobs( - /** @type {string[]} */ (resolved.exclude), - ); + const [wanted, exclude] = await Promise.all([ + parseFoldersToGlobs( + /** @type {string[]} */ (resolved.files), + resolved.extensions, + ), + parseFoldersToGlobs(/** @type {string[]} */ (resolved.exclude)), + ]); return { id, @@ -413,6 +439,9 @@ class DiagnosticsWebpackPlugin { handOver, late, runner, + // The walk this check's files come from, for the ones that walk. + /** @type {Promise | undefined} */ + collecting: undefined, }; }); @@ -468,15 +497,23 @@ class DiagnosticsWebpackPlugin { } } - // Nothing globbed from the file system waits on the module graph. + // Nothing globbed from the file system waits on the module graph, and + // the walk itself is what webpack goes on building through. for (const check of runners) { if (check.filesSource === "modules") continue; - const collected = collectFromFileSystem(compiler, check); - - check.pending.push(...collected.lint); - check.kept.push(...collected.keep); - check.flush(true); + check.collecting = collectFromFileSystem(compiler, check).then( + (collected) => { + check.pending.push(...collected.lint); + check.kept.push(...collected.keep); + check.flush(true); + }, + (err) => { + compilation.errors.push( + new DiagnosticError(check.name, err.message), + ); + }, + ); } compilation.hooks.finishModules.tap(this.key, () => { @@ -487,6 +524,10 @@ class DiagnosticsWebpackPlugin { compilation.hooks.processAssets.tapAsync( this.key, async (_, callback) => { + // Every walk is done by here, so a check has the files it covers + // before it is asked what it found in them. + await Promise.all(runners.map((check) => check.collecting)); + /** @type {Map} */ const outputReports = new Map(); @@ -495,8 +536,12 @@ class DiagnosticsWebpackPlugin { * configured to, which is not always the same set of files. * @param {ResolvedCheck} check the check that read them * @param {Dependencies} dependencies what a run of it read + * @returns {Promise} when the watcher has been told */ - const watch = (check, { read, missing, directories, writes }) => { + const watch = async ( + check, + { read, missing, directories, writes }, + ) => { // Each one is spelled the way the platform does: a watcher looks a // change up under the path it joined, not the one it was given. for (const file of read) { @@ -513,9 +558,13 @@ class DiagnosticsWebpackPlugin { // A file that does not exist yet is under no watch of its own, so // the directory a check would find it in answers for it. - const roots = [...globRoots(check), ...directories].filter( - (directory) => canWatch(directory, compiler, check, writes), + const candidates = [...globRoots(check), ...directories]; + const watchable = await Promise.all( + candidates.map((directory) => + canWatch(directory, compiler, check, writes), + ), ); + const roots = candidates.filter((_, at) => watchable[at]); for (const directory of roots) { // Watching a directory covers what is under it, so one inside @@ -540,7 +589,10 @@ class DiagnosticsWebpackPlugin { // reads this time is not known until it is done, by which point // the watcher has been handed its list. if (check.late) { - watch(check, /** @type {Dependencies} */ (readLast.get(id))); + await watch( + check, + /** @type {Dependencies} */ (readLast.get(id)), + ); afterBuild.push(() => { check.handOver(); @@ -562,7 +614,7 @@ class DiagnosticsWebpackPlugin { } = report; readLast.set(id, { read, missing, directories, writes }); - watch(check, report); + await watch(check, report); // `reportAs` has already dropped whatever it reports as `false`, // so what is left only needs putting where it belongs. diff --git a/src/utils.js b/src/utils.js index a52a6021..47600b74 100644 --- a/src/utils.js +++ b/src/utils.js @@ -1,7 +1,7 @@ // eslint-disable-next-line jsdoc/reject-any-type /** @typedef {any} EXPECTED_ANY */ -import { statSync } from "node:fs"; +import { stat } from "node:fs/promises"; import { createRequire } from "node:module"; import { dirname, isAbsolute, resolve } from "node:path"; @@ -96,9 +96,9 @@ function parseFiles(files, context) { /** * @param {string | string[]} patterns patterns * @param {string | string[]} extensions extensions - * @returns {string[]} globs + * @returns {Promise} globs */ -function parseFoldersToGlobs(patterns, extensions = []) { +async function parseFoldersToGlobs(patterns, extensions = []) { const extensionsList = arrify(extensions); const [prefix, postfix] = extensionsList.length > 1 ? ["{", "}"] : ["", ""]; const extensionsGlob = extensionsList @@ -115,24 +115,29 @@ function parseFoldersToGlobs(patterns, extensions = []) { `/**${extensionsGlob ? `/*.${prefix + extensionsGlob + postfix}` : ""}`, ); - return arrify(patterns).flatMap((/** @type {string} */ pattern) => { - try { - // The patterns are absolute because they are prepended with the context. - // A folder is read as one whatever its name is made of, so this is asked - // before the pattern is: `[symbols]` is a directory and a character class. - if (statSync(pattern).isDirectory()) return asFolder(pattern); - - return pattern; - } catch { - // A glob already says what it covers, whether or not anything is there. - if (picomatch.scan(pattern).isGlob) return pattern; - - // A path that is not there yet is one the build may go on to write, and - // the globs are read once. Naming it both ways is what covers a folder - // that appears later without missing a file of the same name. - return [pattern, asFolder(pattern)]; - } - }); + const read = await Promise.all( + arrify(patterns).map(async (/** @type {string} */ pattern) => { + try { + // The patterns are absolute because they are prepended with the + // context. A folder is read as one whatever its name is made of, so + // this is asked before the pattern is: `[symbols]` is a directory and + // a character class. + if ((await stat(pattern)).isDirectory()) return asFolder(pattern); + + return pattern; + } catch { + // A glob already says what it covers, whether or not anything is there. + if (picomatch.scan(pattern).isGlob) return pattern; + + // A path that is not there yet is one the build may go on to write, and + // the globs are read once. Naming it both ways is what covers a folder + // that appears later without missing a file of the same name. + return [pattern, asFolder(pattern)]; + } + }), + ); + + return read.flat(); } /** diff --git a/test/utils.test.js b/test/utils.test.js index df909bba..dc6e570a 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -9,7 +9,7 @@ import { toPosixPath, } from "../src/utils.js"; -// `parseFoldersToGlobs` stats what it is given, so the fixtures have to exist. +// `parseFoldersToGlobs` reads what it is given, so the fixtures have to exist. const directory = join(import.meta.dirname, "fixtures"); const file = join(import.meta.dirname, "fixtures", "good.js"); @@ -51,16 +51,16 @@ describe("utils", () => { assert.ok(packageB.endsWith("main/package-b/src/**")); }); - it("parseFoldersToGlobs should return globs for folders", () => { - assert.deepStrictEqual(parseFoldersToGlobs(directory, "js"), [ + it("parseFoldersToGlobs should return globs for folders", async () => { + assert.deepStrictEqual(await parseFoldersToGlobs(directory, "js"), [ `${directory}/**/*.js`, ]); - assert.deepStrictEqual(parseFoldersToGlobs(`${directory}/`, "js"), [ + assert.deepStrictEqual(await parseFoldersToGlobs(`${directory}/`, "js"), [ `${directory}/**/*.js`, ]); assert.deepStrictEqual( - parseFoldersToGlobs( + await parseFoldersToGlobs( [directory, `${directory}/`, file], ["js", "cjs", "mjs"], ), @@ -71,26 +71,30 @@ describe("utils", () => { ], ); - assert.deepStrictEqual(parseFoldersToGlobs(directory), [`${directory}/**`]); - assert.deepStrictEqual(parseFoldersToGlobs(`${directory}/`), [ + assert.deepStrictEqual(await parseFoldersToGlobs(directory), [ + `${directory}/**`, + ]); + assert.deepStrictEqual(await parseFoldersToGlobs(`${directory}/`), [ `${directory}/**`, ]); }); - it("parseFoldersToGlobs should return unmodified globs for globs (ignoring extensions)", () => { - assert.deepStrictEqual(parseFoldersToGlobs("**.notjs", "js"), ["**.notjs"]); + it("parseFoldersToGlobs should return unmodified globs for globs (ignoring extensions)", async () => { + assert.deepStrictEqual(await parseFoldersToGlobs("**.notjs", "js"), [ + "**.notjs", + ]); }); - it("parseFoldersToGlobs should cover a path that is not there yet both ways", () => { + it("parseFoldersToGlobs should cover a path that is not there yet both ways", async () => { const absent = join(directory, "not-written-yet"); // Nothing says whether a path the build has still to write is a file or a // folder, and the globs are read once. - assert.deepStrictEqual(parseFoldersToGlobs(absent, "js"), [ + assert.deepStrictEqual(await parseFoldersToGlobs(absent, "js"), [ absent, `${absent}/**/*.js`, ]); - assert.deepStrictEqual(parseFoldersToGlobs(absent), [ + assert.deepStrictEqual(await parseFoldersToGlobs(absent), [ absent, `${absent}/**`, ]); diff --git a/types/index.d.ts b/types/index.d.ts index 2a269935..4bf69698 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -69,13 +69,13 @@ declare class DiagnosticsWebpackPlugin { * @param {Compiler} compiler compiler * @param {string} context context * @param {EnabledCheck} check the check to resolve the globs of - * @returns {ResolvedCheck} the check with its globs resolved + * @returns {Promise} the check with its globs resolved */ resolveCheck( compiler: Compiler, context: string, { id, name, adapter, options }: EnabledCheck, - ): ResolvedCheck; + ): Promise; /** * @param {ResolvedCheck} check the check to create a runner for * @param {Compilation} compilation compilation diff --git a/types/utils.d.ts b/types/utils.d.ts index ea6e284f..128428b0 100644 --- a/types/utils.d.ts +++ b/types/utils.d.ts @@ -73,12 +73,12 @@ export function parseFiles(files: string | string[], context: string): string[]; /** * @param {string | string[]} patterns patterns * @param {string | string[]} extensions extensions - * @returns {string[]} globs + * @returns {Promise} globs */ export function parseFoldersToGlobs( patterns: string | string[], extensions?: string | string[], -): string[]; +): Promise; /** * Globs only know the forward slash, so a path is compared and matched as one. * @param {string} file a path