From 3701039a05cbacd1a27fdaefbe93867f756ce0fd Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Tue, 8 Sep 2026 14:55:38 +0200 Subject: [PATCH] fix: correct savings-only handling in aggressive previews Ensure that `--savings-only` runs properly suppress aggressive mode previews when aggressive re-runs would save less than the current run. Also, update suggested commands to include the `--savings-only` flag where applicable. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert --- CHANGELOG.md | 6 ++++++ bin/css-dedup.js | 4 ++-- package-lock.json | 4 ++-- package.json | 2 +- src/cli/file-pass.js | 21 +++++++++++-------- src/cli/format.js | 18 +++++++++++------ src/cli/render.js | 21 ++++++++++--------- test/cli.test.js | 48 +++++++++++++++++++++++++++++++++++++++++++- test/format.test.js | 5 +++++ test/helpers.js | 11 ++++++++++ 10 files changed, 110 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccd4b69..893c7e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to CSS Dedup are documented in this file, which is (mostly) The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.13.1] - 2026-09-08 + +### Fixed + +* Corrected the aggressive-mode preview of `--fix` runs using `--savings-only`: It now names the gated command its figures come from (`--fix --aggressive --savings-only`), and is left out where that re-run would save less than the run just did + ## [1.13.0] - 2026-08-24 ### Fixed diff --git a/bin/css-dedup.js b/bin/css-dedup.js index 09b03d5..034a104 100644 --- a/bin/css-dedup.js +++ b/bin/css-dedup.js @@ -125,7 +125,7 @@ async function main() { // A blank line between per-file reports, so each file’s closing summary is // visually separated from the next file’s header if (multi && index > 0) console.log(''); - results.push(renderTarget(files[index], { multi, flags }, outcome)); + results.push(renderTarget(files[index], { multi, flags, savingsOnly: options.savingsOnly }, outcome)); }; // A run big enough to pay for a pool spreads across worker threads; anything @@ -157,7 +157,7 @@ async function main() { await runSequentially(files, options, flags, prefetched, render); } - if (multi) printOverallSummary(results, { fix: flags.fix }); + if (multi) printOverallSummary(results, { fix: flags.fix, savingsOnly: options.savingsOnly }); // `--exit-zero` never changes what got merged—only what a finding (skipped // as unsafe, or withheld by `--savings-only`) does to the exit code. A file diff --git a/package-lock.json b/package-lock.json index a7ce2fd..d3d65a6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "css-dedup", - "version": "1.13.0", + "version": "1.13.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "css-dedup", - "version": "1.13.0", + "version": "1.13.1", "license": "MIT", "dependencies": { "postcss": "^8.5.26" diff --git a/package.json b/package.json index a183de9..50a7513 100644 --- a/package.json +++ b/package.json @@ -60,5 +60,5 @@ }, "type": "module", "types": "src/index.d.ts", - "version": "1.13.0" + "version": "1.13.1" } diff --git a/src/cli/file-pass.js b/src/cli/file-pass.js index a9c5969..630f019 100644 --- a/src/cli/file-pass.js +++ b/src/cli/file-pass.js @@ -32,15 +32,20 @@ function skippedWithAggressive(potential) { } // What `--aggressive` would add on top of this run’s real outcome, measured -// against a discarded opposite-mode pass (`potential`)—shared by `--fix` -// and report mode, which differ only in which CSS string and `bytes` they -// compare it against (the written output vs. a discarded dry run) -function computeAggressivePreview(potential, resultCss, applied, bytes) { - const aggDiffers = Boolean(potential && potential.css !== resultCss); - if (!aggDiffers) return { aggExtra: 0, aggExtraSaved: 0, aggDiffers: false }; +// against a discarded opposite-mode pass (`potential`) run under the same +// settings—report mode has its own four passes instead +function computeAggressivePreview(potential, resultCss, applied, bytes, savingsOnly) { + const none = { aggExtra: 0, aggExtraSaved: 0, aggDiffers: false }; + if (!potential || potential.css === resultCss) return none; + + const aggExtraSaved = potential.bytes.saved - bytes.saved; + // Both passes went through the same gate, so an aggressive re-run that saves + // less is strictly worse—not a trade to offer a `savingsOnly` run + if (savingsOnly && aggExtraSaved < 0) return none; + return { aggExtra: potential.applied.length - applied.length, - aggExtraSaved: potential.bytes.saved - bytes.saved, + aggExtraSaved, aggDiffers: true, }; } @@ -111,7 +116,7 @@ async function computeFixPass(css, targetOptions, { isStdin, label }) { aggressiveDiffers, aggressiveOnly, wrote, - ...computeAggressivePreview(potential, output, applied, bytes), + ...computeAggressivePreview(potential, output, applied, bytes, targetOptions.savingsOnly), }; } diff --git a/src/cli/format.js b/src/cli/format.js index 93e308c..1aad419 100644 --- a/src/cli/format.js +++ b/src/cli/format.js @@ -148,11 +148,17 @@ export function formatOutcomeBullet({ countLabel, tense, filesShrinkLen, shrinkT return null; } -// The “in aggressive mode” preview bullet, shared by `--fix` and report mode— -// always still-hypothetical, so always present-tense even inside a `--fix` -// run. `baseSaved` only spells out the combined total in the trailing note; -// the main clause quotes `aggExtraSaved` on its own. -export function formatAggressivePreviewLine(aggExtra, aggExtraSaved, before, baseSaved) { +// The command an aggressive preview quotes its figures for: The gate rides +// along, since what was measured is a re-run under this run’s own settings +export function aggressiveFixCommand(savingsOnly) { + return `--fix --aggressive${savingsOnly ? ' --savings-only' : ''}`; +} + +// The “in aggressive mode” preview bullet—always still-hypothetical, so always +// present-tense even inside a `--fix` run. `baseSaved` only spells out the +// combined total in the trailing note; the main clause quotes `aggExtraSaved` +// on its own. +export function formatAggressivePreviewLine(aggExtra, aggExtraSaved, before, baseSaved, savingsOnly = false) { const label = aggExtra > 0 ? `${aggExtra} more declaration${plural(aggExtra)}` : 'Further consolidation'; - return `* ${label} in aggressive mode: ${formatReduceClause(aggExtraSaved, before, true)} with \`--fix --aggressive\`${formatAggregateTotalNote(baseSaved + aggExtraSaved, before)}`; + return `* ${label} in aggressive mode: ${formatReduceClause(aggExtraSaved, before, true)} with \`${aggressiveFixCommand(savingsOnly)}\`${formatAggregateTotalNote(baseSaved + aggExtraSaved, before)}`; } diff --git a/src/cli/render.js b/src/cli/render.js index 38b9428..9b981b6 100644 --- a/src/cli/render.js +++ b/src/cli/render.js @@ -5,6 +5,7 @@ import { styleText } from 'node:util'; import { aggressiveKeySpelling } from './file-pass.js'; import { + aggressiveFixCommand, formatAggregateTotalNote, formatAggressivePreviewLine, formatAppliedReduceClause, @@ -98,7 +99,7 @@ function logSkippedDetail(log, skipped, skippedAggressive) { log(''); } -function renderFixPass(payload, { isStdin, label, multi, flags }) { +function renderFixPass(payload, { isStdin, label, multi, flags, savingsOnly }) { const { applied, skipped, skippedAggressive, bytes, withheld, sourceMapStale, aggressiveDiffers, aggressiveOnly, aggExtra, aggExtraSaved, aggDiffers } = payload; const log = isStdin ? console.error : console.log; @@ -148,9 +149,9 @@ function renderFixPass(payload, { isStdin, label, multi, flags }) { if (skipped.length) { log(styleText('yellow', `* ${skipped.length} finding${plural(skipped.length)} skipped (considered unsafe to auto-merge)`)); } - // The opposite-mode pass this was measured against went through the same - // `savingsOnly` gate, so a result the re-run would withhold earns no hint - if (aggDiffers) log(formatAggressivePreviewLine(aggExtra, aggExtraSaved, bytes.before, bytes.saved)); + // The opposite-mode pass this was measured against ran under this run’s own + // `savingsOnly` setting, which is why the suggested command repeats the flag + if (aggDiffers) log(formatAggressivePreviewLine(aggExtra, aggExtraSaved, bytes.before, bytes.saved, savingsOnly)); return { exitFailure: skipped.length > 0 || Boolean(withheld), @@ -198,7 +199,7 @@ function renderReportPass(payload, { label }) { // Prints one target’s report and returns `{ exitFailure, errored, stats }`: // whether it counts against the exit code, whether it never produced stats // (read/parse failure), and the numbers the overall summary aggregates -export function renderTarget(file, { multi, flags }, outcome) { +export function renderTarget(file, { multi, flags, savingsOnly }, outcome) { const isStdin = file === '-'; const label = targetLabel(file); @@ -220,14 +221,14 @@ export function renderTarget(file, { multi, flags }, outcome) { return RESULT_ERRORED; } - if (outcome.payload.mode === 'fix') return renderFixPass(outcome.payload, { isStdin, label, multi, flags }); + if (outcome.payload.mode === 'fix') return renderFixPass(outcome.payload, { isStdin, label, multi, flags, savingsOnly }); return renderReportPass(outcome.payload, { label }); } // Rolls up every file’s `stats` into one closing report, so a terminal showing // only the last N lines of a multi-file run doesn’t leave the final file’s own // summary looking like it spoke for the whole run -export function printOverallSummary(results, { fix }) { +export function printOverallSummary(results, { fix, savingsOnly }) { const ok = results.filter(result => result.stats); const errored = results.length - ok.length; const erroredNote = errored ? ` (${errored} file${plural(errored)} could not be processed; see errors above)` : ''; @@ -239,7 +240,7 @@ export function printOverallSummary(results, { fix }) { printOverallReportTable(ok); return; } - printOverallFixSummary(ok); + printOverallFixSummary(ok, savingsOnly); } // Report mode’s all-files table is the per-file table again, one row per file @@ -261,7 +262,7 @@ function printOverallReportTable(ok) { console.log(REPORT_LEGEND); } -function printOverallFixSummary(ok) { +function printOverallFixSummary(ok, savingsOnly) { // Every percentage below is against this—the combined original size of every // successfully processed file—since there’s no single file left to relate a // byte count to once the run’s totals are combined @@ -327,7 +328,7 @@ function printOverallFixSummary(ok) { filesGrowLen: aggFilesGrow.length, growTotal: aggGrowTotal, totalBefore: totalBeforeAll, - flag: '--fix --aggressive', + flag: aggressiveFixCommand(savingsOnly), skipFlag: '--fix --aggressive --savings-only', more: true, aggregateNote: formatAggregateTotalNote(aggNetAll, totalBeforeAll), diff --git a/test/cli.test.js b/test/cli.test.js index b8db58d..91e0541 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -5,7 +5,7 @@ import path from 'node:path'; import { availableParallelism } from 'node:os'; import { poolSize, shouldParallelize } from '../src/cli/pool.js'; import { dedup } from '../src/index.js'; -import { BEST_CELL, RE_MERGED_AB, RE_MERGED_AC, RE_PAYOFF_FIX, RE_SYNTAX_ERROR, RE_SYNTAX_ERROR_UNCLOSED, RE_WITHHELD_ONE, cssGrowing, cssGrowingAggressive, cssMixed, cssShrinkingAggressive, dirTest, findingsRow, fixturesDir, makeTempDir, run, runColor, runTty } from './helpers.js'; +import { BEST_CELL, RE_MERGED_AB, RE_MERGED_AC, RE_PAYOFF_FIX, RE_SYNTAX_ERROR, RE_SYNTAX_ERROR_UNCLOSED, RE_WITHHELD_ONE, cssAggressiveCostlierMerge, cssGrowing, cssGrowingAggressive, cssMixed, cssShrinkingAggressive, dirTest, findingsRow, fixturesDir, makeTempDir, run, runColor, runTty } from './helpers.js'; describe('CLI', () => { test('Shows help with `--help`', () => { @@ -373,6 +373,39 @@ describe('CLI', () => { } }); + test('Suppresses the `--aggressive` preview under `--savings-only` when the gated re-run would save less', () => { + const dirTemp = makeTempDir('temp_aggressive_saves_less'); + const file = path.join(dirTemp, 'less.css'); + fs.writeFileSync(file, cssAggressiveCostlierMerge); + + try { + // Ungated, the trade is real and worth previewing… + assert.match(run(['--fix', file]).stdout, /in aggressive mode: Reduce duplication but grow by \d+ more bytes/); + + // …but both passes ran gated here, so the aggressive re-run just saves + // less than what was written—no trade a `--savings-only` run wants + fs.writeFileSync(file, cssAggressiveCostlierMerge); + assert.ok(!run(['--fix', '--savings-only', file]).stdout.includes('in aggressive mode')); + } finally { + fs.rmSync(dirTemp, { recursive: true, force: true }); + } + }); + + test('Repeats `--savings-only` in the command the `--aggressive` preview suggests', () => { + const dirTemp = makeTempDir('temp_aggressive_preview_gated'); + const file = path.join(dirTemp, 'shrink.css'); + fs.writeFileSync(file, cssShrinkingAggressive); + + try { + // The figures come from a gated re-run, so the gate belongs in the + // command: `--fix --aggressive` on its own can land somewhere else + const { stdout } = run(['--fix', '--savings-only', file]); + assert.match(stdout, /in aggressive mode: Reduce duplication and save \d+ more bytes \(-\d+\.\d%\) with `--fix --aggressive --savings-only`/); + } finally { + fs.rmSync(dirTemp, { recursive: true, force: true }); + } + }); + test('Processes multiple files in one invocation, with a header per file', () => { const dirTemp = makeTempDir('temp_multi'); const fileA = path.join(dirTemp, 'a.css'); @@ -641,6 +674,19 @@ describe('CLI', () => { } }); + test('Repeats `--savings-only` in the overall summary’s aggressive bullet, too', () => { + const dirTemp = makeTempDir('temp_multi_summary_aggressive_gated'); + fs.writeFileSync(path.join(dirTemp, 'one.css'), cssShrinkingAggressive); + fs.writeFileSync(path.join(dirTemp, 'two.css'), cssShrinkingAggressive); + + try { + const { stdout } = run(['--fix', '--savings-only', dirTemp]); + assert.match(stdout, /in aggressive mode: Reduce duplication and save \d+ more bytes \(-\d+\.\d%\) with `--fix --aggressive --savings-only`/); + } finally { + fs.rmSync(dirTemp, { recursive: true, force: true }); + } + }); + test('Recursively finds `.css` files under a directory, skipping `node_modules` and dotfolders', () => { const dirTemp = path.join(dirTest, 'temp_dir_scan'); fs.mkdirSync(path.join(dirTemp, 'sub', 'node_modules'), { recursive: true }); diff --git a/test/format.test.js b/test/format.test.js index 025f144..85c1165 100644 --- a/test/format.test.js +++ b/test/format.test.js @@ -125,6 +125,11 @@ describe('Summary clauses', () => { assert.match(formatAggressivePreviewLine(0, 100, 1000, 50), /^\* Further consolidation in aggressive mode: /); }); + test('Carries `--savings-only` into the command the aggressive preview suggests', () => { + assert.match(formatAggressivePreviewLine(1, 100, 1000, 50), /with `--fix --aggressive`/); + assert.match(formatAggressivePreviewLine(1, 100, 1000, 50, true), /with `--fix --aggressive --savings-only`/); + }); + test('Quotes the combined total, not just its own delta, in the aggressive preview’s trailing note', () => { // 50 already saved by `--fix`, 100 more from aggressive → 150 combined assert.match(formatAggressivePreviewLine(1, 100, 1000, 50), /\(total: -150 bytes \/ -15\.0%\)$/); diff --git a/test/helpers.js b/test/helpers.js index 785d48b..56c9468 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -111,6 +111,17 @@ export const cssTwoLeadingRemovals = [ // removes a whole rule instead of just adding to a selector list export const cssShrinkingAggressive = '.a { transform: rotate(90deg); }\n.b { transform: rotate(100grad); }\n'; +// Mergeable in both modes, but aggressive pulls a third rule—reachable only by +// canonicalizing the `` values—into the cluster, and its long selector +// costs more than the extra fold saves: Gated, the aggressive re-run comes out +// behind the gated default pass. +export const cssAggressiveCostlierMerge = [ + '.a { transform: rotate(90deg); }', + '.b { transform: rotate(90deg); }', + '.a-very-long-selector-name-here-indeed { transform: rotate(100grad); color: red; }', + '', +].join('\n'); + // Assertion patterns shared across several tests export const RE_WITHHELD_ONE = /1 withheld/; export const RE_MERGED_AB = /\.a,\s*\.b\s*{\s*color: red;\s*}/;