diff --git a/.changeset/lint-only-what-changed.md b/.changeset/lint-only-what-changed.md new file mode 100644 index 0000000..dee2eac --- /dev/null +++ b/.changeset/lint-only-what-changed.md @@ -0,0 +1,5 @@ +--- +"diagnostics-webpack-plugin": patch +--- + +Lint only the files webpack rebuilt, reporting the rest from the previous compilation, and start linting while the module graph is still being built. diff --git a/README.md b/README.md index a12c942..b46a898 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,9 @@ new DiagnosticsPlugin({ Run with `{ use: "eslint" }`. It lints the files webpack builds, so only the modules that end up in the bundle are checked. -Alongside the shared options you can pass any [ESLint Node.js API option](https://eslint.org/docs/latest/integrate/nodejs-api#-new-eslintoptions) — they are handed to the `ESLint` class as they are. +Alongside the shared options you can pass any [ESLint Node.js API option](https://eslint.org/docs/latest/integrate/nodejs-api#-new-eslintoptions) — they are handed to the `ESLint` class as they are. `concurrency` is worth knowing about: it spreads a lint across worker threads, and ESLint warns on the runs where doing so costs more than it saves, so measure your own project rather than turning it on by default. + +A rebuild lints only the files webpack rebuilt and reports the rest from the previous run, so `lintDirtyModulesOnly` is only worth setting to skip the first lint entirely. ### `configType` diff --git a/src/check.js b/src/check.js index 90df7b7..a73980b 100644 --- a/src/check.js +++ b/src/check.js @@ -2,6 +2,7 @@ import { isAbsolute, join } from "node:path"; import DiagnosticError from "./DiagnosticError.js"; import { reportedAs } from "./options.js"; +import { toPosixPath } from "./utils.js"; /** @typedef {import("webpack").Compilation} Compilation */ /** @typedef {import("./checks/index.js").CheckResult} CheckResult */ @@ -9,7 +10,37 @@ import { reportedAs } from "./options.js"; /** @typedef {import("./options.js").EnabledCheck} EnabledCheck */ /** @typedef {{ filePath: string, content: string }} OutputReportContent */ /** @typedef {{ errors?: DiagnosticError, warnings?: DiagnosticError, outputReport?: OutputReportContent }} Report */ -/** @typedef {{ lint: (files: string[]) => void, report: () => Promise }} Runner */ +/** @typedef {{ lint: (files: string[]) => void, keep: (files: string[]) => void, keepKnown: (removed: ReadonlySet) => void, report: () => Promise }} Runner */ +/** @typedef {Map} ResultStore */ + +/** @type {WeakMap>} */ +const resultStores = new WeakMap(); + +/** + * The results of the last compilation, so a rebuild lints what webpack rebuilt + * and reports the rest from here. + * @param {Compilation} compilation compilation + * @param {string} check a key unique to the check within the compiler + * @returns {ResultStore} what the check last found in every file it covered + */ +function getResultStore(compilation, check) { + const { compiler } = compilation; + let stores = resultStores.get(compiler); + + if (!stores) { + stores = new Map(); + resultStores.set(compiler, stores); + } + + let store = stores.get(check); + + if (!store) { + store = new Map(); + stores.set(check, store); + } + + return store; +} /** * @param {Promise[]} results results @@ -42,21 +73,125 @@ function createCheckRunner(key, { name, adapter, options }, compilation) { /** @type {Promise[]} */ const rawResults = []; + // Every path the store is keyed by goes through `toPosixPath`: webpack hands + // over a module's resource with the separators the platform uses, and a + // check answers with whatever its own tool wrote. + const store = getResultStore(compilation, `${key}:${name}`); + // A check that cannot say which file a result came from is linted whole. + const { resultPath } = adapter; + /** @type {Set} */ + const covered = new Set(); + /** @type {Set} */ + const linted = new Set(); + // A check that cannot lint fails the same way for every batch it is given. + let failed = false; /** * @param {string[]} files files */ function lint(files) { + for (const file of files) { + const known = toPosixPath(file); + + covered.add(known); + linted.add(known); + } + rawResults.push( pending .then((instance) => (instance ? instance.lintFiles(files) : [])) .catch((err) => { - compilation.errors.push(new DiagnosticError(name, err.message)); + if (!failed) { + failed = true; + compilation.errors.push(new DiagnosticError(name, err.message)); + } + return []; }), ); } + /** + * Reports a file from the last compilation rather than linting it again. + * A file the store does not hold is linted: webpack restores a module from + * its own cache without building it, and the first run of a compiler that + * does so has nothing to report it from. + * @param {string[]} files the files webpack did not rebuild + */ + function keep(files) { + if (!resultPath) { + lint(files); + return; + } + + /** @type {string[]} */ + const unknown = []; + + for (const file of files) { + const known = toPosixPath(file); + + if (store.has(known)) covered.add(known); + else unknown.push(file); + } + + if (unknown.length > 0) lint(unknown); + } + + /** + * Keeps every file the last compilation covered bar the ones webpack says + * are gone — what a check that walks the file system knows about the files + * it is not being told changed, without walking it again. + * @param {ReadonlySet} removed the files webpack no longer sees + */ + function keepKnown(removed) { + if (!resultPath) return; + + const gone = new Set([...removed].map((file) => toPosixPath(file))); + + for (const file of store.keys()) { + if (!gone.has(file)) covered.add(file); + } + } + + /** + * Puts what was just linted into the store, drops what webpack no longer + * builds, and answers with the results for every file this compilation + * covers — the fresh ones and the ones kept from the last. + * @param {CheckResult[]} results what the check produced this time + * @returns {CheckResult[]} the results to report + */ + function remember(results) { + if (!resultPath) return results; + + // A check reports nothing for a file it found nothing in, so what was + // linted is forgotten first and only what came back is put back. + for (const file of linted) store.delete(file); + + // A result the check cannot put a file to is reported as it is: it is + // this compilation's, and there is nothing to remember it under. + /** @type {CheckResult[]} */ + const loose = []; + + for (const result of results) { + const file = resultPath(result); + + if (file) store.set(toPosixPath(file), result); + else loose.push(result); + } + + // A file a check found nothing in is remembered as nothing found, which is + // what tells a rebuild it has been linted at all. + for (const file of linted) { + if (!store.has(file)) store.set(file, undefined); + } + + for (const file of store.keys()) { + if (!covered.has(file)) store.delete(file); + } + + return [...store.values(), ...loose].filter(Boolean); + } + /** * @returns {Promise} report */ @@ -70,7 +205,7 @@ function createCheckRunner(key, { name, adapter, options }, compilation) { await instance.cleanup(); - const results = await instance.getResults(raw); + const results = remember(await instance.getResults(raw)); // Do not analyze when the check reported nothing. if (!results || results.length === 0) { @@ -111,7 +246,7 @@ function createCheckRunner(key, { name, adapter, options }, compilation) { return report; } - return { lint, report }; + return { keep, keepKnown, lint, report }; } export default createCheckRunner; diff --git a/src/checks/eslint.js b/src/checks/eslint.js index 675a8d4..77da6f6 100644 --- a/src/checks/eslint.js +++ b/src/checks/eslint.js @@ -267,6 +267,8 @@ export default { extensions: "js", }, defaultExclude: () => "**/node_modules/**", + resultPath: (/** @type {EXPECTED_ANY} */ result) => + /** @type {LintResult} */ (result).filePath, create, getESLintOptions, }; diff --git a/src/checks/index.js b/src/checks/index.js index 2f1263c..048daa5 100644 --- a/src/checks/index.js +++ b/src/checks/index.js @@ -31,6 +31,7 @@ import stylelint from "./stylelint.js"; * @property {(files: string[]) => Promise} lintFiles lints the given files * @property {(results: CheckResult[]) => Promise} getResults turns the raw results of every `lintFiles` call into the results to report * @property {(results: CheckResult[]) => { errors: CheckResult[], warnings: CheckResult[] }} splitResults splits the results by their own severity, leaving `reportAs` to the plugin + * @property {((result: CheckResult) => string | undefined)=} resultPath the file a result came from, without which a rebuild re-lints everything * @property {(formatter?: FormatterOption) => Promise} getFormatter loads a formatter, falling back to the tool's default one * @property {() => Promise} cleanup releases whatever the tool holds after a run */ @@ -57,6 +58,7 @@ import stylelint from "./stylelint.js"; * @property {{ [key: string]: EXPECTED_ANY }} defaults default options for this check * @property {(compiler: Compiler) => string | string[]} defaultExclude the globs excluded when the user specifies none * @property {(context: CheckContext) => Promise} create creates a check for one compilation + * @property {((result: CheckResult) => string | undefined)=} resultPath the file a result came from, without which a rebuild re-lints everything */ /** @type {Map} */ diff --git a/src/checks/stylelint.js b/src/checks/stylelint.js index 7794b05..ad5676e 100644 --- a/src/checks/stylelint.js +++ b/src/checks/stylelint.js @@ -65,26 +65,6 @@ const KEPT_OPTIONS = ["cache", "cacheLocation", "files", "fix", "formatter"]; /** @type {{ [key: string]: Loaded }} */ const cache = {}; -/** @type {WeakMap} */ -const resultStorage = new WeakMap(); - -/** - * Stylelint only lints the files webpack reports as modified, so results of - * files left untouched by a watch rebuild are carried over from the last run. - * @param {Compiler} compiler compiler - * @returns {LintResultMap} lint result map - */ -function getResultStorage(compiler) { - let storage = resultStorage.get(compiler); - - if (!storage) { - storage = {}; - resultStorage.set(compiler, storage); - } - - return storage; -} - /** * @param {Options} options options * @returns {Partial} stylelint options @@ -228,9 +208,8 @@ function getLoadedStylelint(key, options) { * @param {CheckContext} context check context * @returns {Promise} stylelint check */ -async function create({ key, options, compilation }) { +async function create({ key, options }) { const loaded = getLoadedStylelint(key, options); - const storage = getResultStorage(compilation.compiler); /** @type {LintResult[]} */ let lastResults = []; @@ -239,10 +218,6 @@ async function create({ key, options, compilation }) { async lintFiles(files) { const resolved = parseFiles(files, String(options.context)); - for (const file of resolved) { - delete storage[file]; - } - // One task per file keeps every worker of the pool busy. if (loaded.threads > 1) { const results = await Promise.all( @@ -255,13 +230,9 @@ async function create({ key, options, compilation }) { return loaded.lintFiles(resolved); }, async getResults(results) { - for (const result of /** @type {LintResult[]} */ (results)) { - if (result.ignored) continue; - - storage[String(result.source)] = result; - } - - lastResults = Object.values(storage); + lastResults = /** @type {LintResult[]} */ (results).filter( + (result) => !result.ignored, + ); return lastResults; }, @@ -332,6 +303,8 @@ export default { }, getLoadedStylelint, getStylelintOptions, + resultPath: (/** @type {EXPECTED_ANY} */ result) => + /** @type {LintResult} */ (result).source || undefined, /** * @param {Compiler} compiler compiler * @returns {string[]} default excluded globs diff --git a/src/index.js b/src/index.js index 2ce10c5..33df824 100644 --- a/src/index.js +++ b/src/index.js @@ -35,6 +35,10 @@ import { const LINT_PLUGIN = "DiagnosticsWebpackPlugin"; +// How many files webpack has to have built before a check is handed any of +// them, rather than all of them once the graph is done. +const EARLY_BATCH = 64; + let compilerId = 0; /** @@ -187,12 +191,44 @@ class DiagnosticsWebpackPlugin { if (enabled.length === 0) return; - const runners = enabled.map((check) => ({ - ...check, + const runners = enabled.map((check) => { + const runner = this.createRunner(check, compilation); + /** @type {string[]} */ + const pending = []; /** @type {string[]} */ - files: [], - runner: this.createRunner(check, compilation), - })); + const kept = []; + let scheduled = false; + + // Linting starts while webpack is still building rather than after + // it. A batch below the threshold waits for the end of the graph: a + // check that parallelises its own work, as ESLint does under + // `concurrency`, has nothing to spread across workers before then. + const flush = (atEnd = false) => { + if (!atEnd) { + if (scheduled || pending.length < EARLY_BATCH) return; + + scheduled = true; + setImmediate(() => flush(true)); + + return; + } + + scheduled = false; + + if (pending.length > 0) runner.lint(pending.splice(0)); + if (kept.length > 0) runner.keep(kept.splice(0)); + }; + + return { + ...check, + /** @type {string[]} */ + files: [], + pending, + kept, + flush, + runner, + }; + }); const fromModules = runners.filter( ({ adapter }) => adapter.filesSource === "modules", @@ -201,8 +237,9 @@ class DiagnosticsWebpackPlugin { if (fromModules.length > 0) { /** * @param {Module} module module + * @param {boolean} rebuilt whether webpack built the module this time */ - const addFile = (module) => { + const addFile = (module, rebuilt) => { const { resource } = /** @type {NormalModule} */ (module); if (!resource) return; @@ -222,29 +259,41 @@ class DiagnosticsWebpackPlugin { if (isFileNotListed && isFileWanted && isQueryNotExclude) { files.push(file); + (rebuilt ? check.pending : check.kept).push(file); + check.flush(); } } }; - // Add the file to be linted - compilation.hooks.succeedModule.tap(this.key, addFile); + compilation.hooks.succeedModule.tap(this.key, (module) => + addFile(module, true), + ); + // A module webpack did not rebuild is reported from the last run. if (!this.options.lintDirtyModulesOnly) { - compilation.hooks.stillValidModule.tap(this.key, addFile); + compilation.hooks.stillValidModule.tap(this.key, (module) => + addFile(module, false), + ); } } - // Lint all files added - compilation.hooks.finishModules.tap(this.key, () => { - for (const check of runners) { - const { adapter, files, runner } = check; - const filesToLint = - adapter.filesSource === "modules" - ? files - : collectFromFileSystem(compiler, check); - - if (filesToLint.length > 0) runner.lint(filesToLint); + // Nothing globbed from the file system waits on the module graph. + for (const check of runners) { + if (check.adapter.filesSource === "modules") continue; + + const files = collectFromFileSystem(compiler, check); + + if (files.length > 0) check.runner.lint(files); + + // A rebuild is told what changed rather than what exists, so the rest + // of what the walk found last time is reported from there. + if (compiler.modifiedFiles) { + check.runner.keepKnown(compiler.removedFiles || new Set()); } + } + + compilation.hooks.finishModules.tap(this.key, () => { + for (const check of fromModules) check.flush(true); }); // await and interpret results diff --git a/test/incremental.test.js b/test/incremental.test.js new file mode 100644 index 0000000..cf2aa32 --- /dev/null +++ b/test/incremental.test.js @@ -0,0 +1,87 @@ +import assert from "node:assert/strict"; +import { rmSync, writeFileSync } from "node:fs"; +import { createRequire } from "node:module"; +import { join } from "node:path"; +import { afterEach, describe, it } from "node:test"; + +import pack from "./utils/pack.js"; + +const require = createRequire(import.meta.url); +const eslintPath = join(import.meta.dirname, "mock/eslint-recorder"); +const entry = join(import.meta.dirname, "fixtures", "watch-entry.js"); +const leaf = join(import.meta.dirname, "fixtures", "watch-leaf.js"); +const linted = () => require(eslintPath)._calls.flat(); + +describe("incremental", () => { + let watch; + + afterEach(() => { + if (watch) watch.close(); + rmSync(entry, { force: true }); + rmSync(leaf, { force: true }); + }); + + it("should lint only what webpack rebuilt", (t, done) => { + writeFileSync(leaf, "const leaf = 1;\n"); + writeFileSync(entry, "require('./watch-leaf');\nconst entry = 1;\n"); + require(eslintPath)._reset(); + + // eslint-disable-next-line no-use-before-define + let next = firstPass; + const compiler = pack("watch", { eslintPath }); + + watch = compiler.watch({}, (err, stats) => next(err, stats)); + + function secondPass(err) { + assert.strictEqual(err, null); + + const files = linted(); + + assert.strictEqual(files.length, 1); + assert.match(files[0], /watch-leaf\.js/u); + done(); + } + + function firstPass(err) { + assert.strictEqual(err, null); + + const files = linted(); + + assert.strictEqual(files.length, 2); + require(eslintPath)._reset(); + next = secondPass; + writeFileSync(leaf, "const leaf = 2;\n"); + } + }); + + it("should stop reporting a file that leaves the graph", (t, done) => { + writeFileSync(leaf, "const leaf = 1;\n"); + writeFileSync(entry, "require('./watch-leaf');\nconst entry = 1;\n"); + + // eslint-disable-next-line no-use-before-define + let next = firstPass; + const compiler = pack("watch"); + + watch = compiler.watch({}, (err, stats) => next(err, stats)); + + function secondPass(err, stats) { + assert.strictEqual(err, null); + + const [{ message }] = stats.compilation.errors; + + assert.match(message, /watch-entry\.js/u); + assert.doesNotMatch(message, /watch-leaf\.js/u); + done(); + } + + function firstPass(err, stats) { + assert.strictEqual(err, null); + + const [{ message }] = stats.compilation.errors; + + assert.match(message, /watch-leaf\.js/u); + next = secondPass; + writeFileSync(entry, "const entry = 1;\n"); + } + }); +}); diff --git a/test/store.test.js b/test/store.test.js new file mode 100644 index 0000000..7f229a8 --- /dev/null +++ b/test/store.test.js @@ -0,0 +1,88 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; + +import createCheckRunner from "../src/check.js"; + +const win32 = (/** @type {string} */ file) => `C:\\project\\${file}`; + +/** + * @param {string[]} dirty the files the check finds something in + * @returns {EXPECTED_ANY} an adapter answering with the paths it was given + */ +function adapterFinding(dirty) { + return { + name: "fake", + resultPath: (/** @type {EXPECTED_ANY} */ result) => result.filePath, + create: async () => ({ + cleanup: async () => {}, + getFormatter: async () => (/** @type {EXPECTED_ANY[]} */ results) => + results.map((result) => result.filePath).join(","), + getResults: async (/** @type {EXPECTED_ANY[]} */ raw) => raw, + lintFiles: async (/** @type {string[]} */ files) => + files + .filter((file) => dirty.includes(file)) + .map((file) => ({ filePath: file })), + splitResults: (/** @type {EXPECTED_ANY[]} */ results) => ({ + errors: results, + warnings: [], + }), + }), + }; +} + +/** + * @param {EXPECTED_ANY} adapter the check to run + * @param {EXPECTED_ANY} compiler the compiler the store hangs off + * @returns {EXPECTED_ANY} a runner over a fresh compilation of that compiler + */ +function runnerFor(adapter, compiler) { + const compilation = { compiler, errors: [], warnings: [] }; + + return createCheckRunner( + "test", + { adapter, name: "fake", options: {} }, + compilation, + ); +} + +const reported = async (/** @type {EXPECTED_ANY} */ runner) => { + const { errors } = await runner.report(); + + return errors ? errors.message.replace("[fake] ", "").split(",") : []; +}; + +describe("store", () => { + it("should keep a file webpack spells with backslashes", async () => { + const compiler = { outputPath: "/out" }; + const adapter = adapterFinding([win32("a.css")]); + const first = runnerFor(adapter, compiler); + + first.lint([win32("a.css"), win32("b.css")]); + + assert.deepStrictEqual(await reported(first), [win32("a.css")]); + + const second = runnerFor(adapterFinding([]), compiler); + + second.keep([win32("a.css"), win32("b.css")]); + + assert.deepStrictEqual(await reported(second), [win32("a.css")]); + }); + + it("should drop a removed file whatever webpack spells it with", async () => { + // A glob check walks the file system for forward slashes and is told what + // changed in the separators the platform uses. + const compiler = { outputPath: "/out" }; + const walked = "C:/project/a.css"; + const first = runnerFor(adapterFinding([walked]), compiler); + + first.lint([walked]); + + assert.deepStrictEqual(await reported(first), [walked]); + + const second = runnerFor(adapterFinding([]), compiler); + + second.keepKnown(new Set([win32("a.css")])); + + assert.deepStrictEqual(await reported(second), []); + }); +}); diff --git a/types/check.d.ts b/types/check.d.ts index e069298..37960cf 100644 --- a/types/check.d.ts +++ b/types/check.d.ts @@ -14,8 +14,11 @@ export type Report = { }; export type Runner = { lint: (files: string[]) => void; + keep: (files: string[]) => void; + keepKnown: (removed: ReadonlySet) => void; report: () => Promise; }; +export type ResultStore = Map; /** * Creates the check synchronously so that the compilation hooks are tapped * before webpack starts building modules, whatever the tool takes to load. diff --git a/types/checks/eslint.d.ts b/types/checks/eslint.d.ts index e8fb2e1..904de7b 100644 --- a/types/checks/eslint.d.ts +++ b/types/checks/eslint.d.ts @@ -10,6 +10,7 @@ declare namespace _default { let extensions: string; } export function defaultExclude(): string; + export function resultPath(result: EXPECTED_ANY): string; export { create }; export { getESLintOptions }; } diff --git a/types/checks/index.d.ts b/types/checks/index.d.ts index 58682ae..00b0d93 100644 --- a/types/checks/index.d.ts +++ b/types/checks/index.d.ts @@ -44,6 +44,10 @@ export type CheckInstance = { errors: CheckResult[]; warnings: CheckResult[]; }; + /** + * the file a result came from, without which a rebuild re-lints everything + */ + resultPath?: ((result: CheckResult) => string | undefined) | undefined; /** * loads a formatter, falling back to the tool's default one */ @@ -132,6 +136,10 @@ export type CheckAdapter = { * creates a check for one compilation */ create: (context: CheckContext) => Promise; + /** + * the file a result came from, without which a rebuild re-lints everything + */ + resultPath?: ((result: CheckResult) => string | undefined) | undefined; }; /** @typedef {import("webpack").Compilation} Compilation */ /** @typedef {import("webpack").Compiler} Compiler */ @@ -155,6 +163,7 @@ export type CheckAdapter = { * @property {(files: string[]) => Promise} lintFiles lints the given files * @property {(results: CheckResult[]) => Promise} getResults turns the raw results of every `lintFiles` call into the results to report * @property {(results: CheckResult[]) => { errors: CheckResult[], warnings: CheckResult[] }} splitResults splits the results by their own severity, leaving `reportAs` to the plugin + * @property {((result: CheckResult) => string | undefined)=} resultPath the file a result came from, without which a rebuild re-lints everything * @property {(formatter?: FormatterOption) => Promise} getFormatter loads a formatter, falling back to the tool's default one * @property {() => Promise} cleanup releases whatever the tool holds after a run */ @@ -179,6 +188,7 @@ export type CheckAdapter = { * @property {{ [key: string]: EXPECTED_ANY }} defaults default options for this check * @property {(compiler: Compiler) => string | string[]} defaultExclude the globs excluded when the user specifies none * @property {(context: CheckContext) => Promise} create creates a check for one compilation + * @property {((result: CheckResult) => string | undefined)=} resultPath the file a result came from, without which a rebuild re-lints everything */ /** @type {Map} */ declare const adapters: Map; diff --git a/types/checks/stylelint.d.ts b/types/checks/stylelint.d.ts index 6821f82..618d43f 100644 --- a/types/checks/stylelint.d.ts +++ b/types/checks/stylelint.d.ts @@ -10,6 +10,7 @@ declare namespace _default { } export { getLoadedStylelint }; export { getStylelintOptions }; + export function resultPath(result: EXPECTED_ANY): string | undefined; export function defaultExclude(compiler: Compiler): string[]; export { create }; } @@ -67,9 +68,5 @@ export function getStylelintOptions( * @param {CheckContext} context check context * @returns {Promise} stylelint check */ -declare function create({ - key, - options, - compilation, -}: CheckContext): Promise; +declare function create({ key, options }: CheckContext): Promise; import { Worker as JestWorker } from "jest-worker";