feat: deprecation system and benchmark suite for Less 5.x prep - #4402
Merged
Conversation
…ess 5.x New deprecation infrastructure with automatic repetition limiting (max 5 per type): - deprecation.js: registry of deprecation IDs with descriptions - Parser warn() accepts deprecation IDs for categorized warnings - --quiet-deprecations: suppress only deprecation warnings (keeps other warnings) New deprecation warnings for features being removed in 5.x: - js-eval: inline JavaScript backtick expressions - at-plugin: @plugin directive Existing warnings now tagged with stable IDs: - mixin-call-no-parens, mixin-call-whitespace, dot-slash-operator - variable-in-unknown-value, property-in-unknown-value CLI deprecation notices for: --js, --line-numbers, --math=always
…t tracking
Results organized as:
results/latest/{system-id}.json - most recent per system
results/runs/{date}_{system-id}.json - historical archive (gitignored)
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a benchmarking suite (multiple Less benchmark files, a Node runner, historical-run script, and results), a structured deprecation registry with a handler, parser integration to emit controlled deprecation warnings, and CLI/context plumbing to silence deprecation warnings via a new flag. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/less/src/less/deprecation.js (1)
63-64: Consider exportingMAX_REPETITIONSin the default export for consistency.The named exports include
MAX_REPETITIONS, but the default export only includesdeprecationsandDeprecationHandler. If the default export is the primary API, consider includingMAX_REPETITIONSfor completeness.📝 Suggested fix
export { deprecations, DeprecationHandler, MAX_REPETITIONS }; -export default { deprecations, DeprecationHandler }; +export default { deprecations, DeprecationHandler, MAX_REPETITIONS };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/src/less/deprecation.js` around lines 63 - 64, Update the default export to include MAX_REPETITIONS alongside deprecations and DeprecationHandler so the default export mirrors the named exports; modify the export statement that currently returns { deprecations, DeprecationHandler } to add MAX_REPETITIONS (referencing the symbols deprecations, DeprecationHandler, and MAX_REPETITIONS) ensuring consistency between named and default exports.packages/less/benchmark/benchmark-v3.less (1)
1-2: Minor: Comment mentions@pluginbut it's not used in this file.The file header lists
@pluginas a v3.0+ feature being benchmarked, but the file doesn't contain any@pluginusage.📝 Suggested fix
-// Benchmark for Less v3.0+ features: if(), boolean(), $prop accessor, `@plugin` +// Benchmark for Less v3.0+ features: if(), boolean(), $prop accessor // This file is standalone and does NOT import the base benchmark.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/benchmark-v3.less` around lines 1 - 2, The header comment lists the Less v3 feature "@plugin" but the file doesn't use it; either remove "@plugin" from the header comment or add a minimal, valid `@plugin` usage example in this benchmark file (e.g., a short `@plugin` declaration and a small rule that demonstrates it) so the header accurately reflects the file contents; update the header comment or add the plugin snippet near the top of the file where other feature examples are located.packages/less/src/less/parser/parser.js (1)
63-63: CalldeprecationHandler.summarize()at the end of parsing to report suppressed warnings.The
DeprecationHandlerlimits deprecation warnings to 5 per type but never reports how many additional warnings were omitted. Thesummarize(logger)method exists to log these suppressed warnings, but it's never invoked. Users won't know that warnings were hidden from them.Consider calling
deprecationHandler.summarize(logger)in thefinish()callback (parser.js, line 257) before returning the result to the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/src/less/parser/parser.js` at line 63, The parser constructs a DeprecationHandler instance (deprecationHandler) but never calls its summarize method, so suppressed deprecation warnings are never reported; modify the parser's finish() callback to invoke deprecationHandler.summarize(logger) (using the same logger passed around in the parser) just before returning the final result so the summary of omitted warnings is logged; locate the finish function in parser.js and add a call to deprecationHandler.summarize(logger) immediately prior to the return/resolve path that yields the parse result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/benchmark/benchmark-import-target.less`:
- Around line 33-36: Stylelint is parsing Less as SCSS, causing the Less mixin
call `.imported-mixin()` inside `.imported-base` to throw CssSyntaxError; fix by
updating the Stylelint config (the .stylelintrc.json) to either set
"customSyntax": "postcss-less" globally or add an "overrides" entry targeting
Less files (e.g., "*.less" or the benchmark pattern) that sets "customSyntax":
"postcss-less", or alternatively add an "ignoreFiles" or exclude pattern for the
packages/less/benchmark/** directory to skip linting those files; apply the
change so .less files are parsed with postcss-less and the `.imported-mixin()`
call is accepted.
In `@packages/less/benchmark/benchmark-runner.js`:
- Around line 121-145: The returned field variance_pct is misnamed (it's
actually range over mean); update the stats computation to produce both a true
variability percent and a range percent: compute range_pct = ((max - min) / avg)
* 100 and compute variance_pct (coefficient of variation) = (stddev / avg) * 100
using the existing stddev and avg variables, then return range_pct and
variance_pct (rounded like the other fields) instead of the current incorrect
variance_pct value; refer to the variables avg, stddev, min, max and the
returned property variance_pct in the diff to locate where to change names and
calculations.
- Around line 22-35: The loop that requires candidate paths uses
require(path.resolve(p)) which incorrectly converts the package name 'less' into
a filesystem path; update the require logic in the for-loop (where tryPaths is
iterated and p is used) so that named packages (like 'less') are required
directly (require(p)) or only call path.resolve for explicit relative/absolute
paths (e.g., when p starts with '.' or '/'); change the require invocation
accordingly so the 'less' fallback uses Node's package resolution instead of
resolving to <cwd>/less.
In `@packages/less/benchmark/run-historical.sh`:
- Around line 135-137: RUN_FILE currently uses RUN_DATE (date -u +%Y-%m-%d) so
multiple runs the same day overwrite each other; change the timestamp used to
include a time component (e.g., hours/minutes/seconds or ISO timestamp) when
computing RUN_DATE or build a separate RUN_TIMESTAMP, and use that when
constructing RUN_FILE
("$RUNS_DIR/${RUN_DATE}_${RUN_TIMESTAMP}_${SYSTEM_ID}.json" or similar). Keep
LATEST_FILE/LATEST_DIR behavior as-is if you still want a single latest per
system, and update any references to RUN_FILE creation in this script to use the
new timestamp variable (referencing RUN_DATE, RUN_FILE, RUNS_DIR, RUN_TIMESTAMP,
SYSTEM_ID, LATEST_FILE, LATEST_DIR).
- Around line 27-31: ALL_VERSIONS currently skips Less v3.1.0–v3.4.0, so update
the ALL_VERSIONS array to include the missing minor releases (v3.1.0, v3.2.0,
v3.3.0, v3.4.0) between the existing v3.0.0 and v3.5.0 entries; modify the
ALL_VERSIONS definition in run-historical.sh so the matrix truly covers every
major/minor release as advertised.
In `@packages/less/bin/lessc`:
- Around line 411-413: During argv scanning the code currently emits deprecation
notices immediately (e.g., the switch case handling 'quiet-deprecations' and
other deprecated flags) using direct console.warn calls, which bypasses
suppression flags; change this by collecting deprecation messages into a
queue/array (e.g., collectDeprecation(msg) using a local deprecations array)
instead of calling console.warn inline, keep setting options.quietDeprecations =
true in the 'quiet-deprecations' case, then after argv parsing and options
resolution call a single flushDeprecations(deprecations, options) routine that
prints each queued message only if not suppressed by options.quietDeprecations /
options.quiet / options.silent; replace all direct console.warn usages in the
CLI parse logic (the instances near the shown case and the other mentioned
spots) to push into the queue so suppression works regardless of flag ordering.
---
Nitpick comments:
In `@packages/less/benchmark/benchmark-v3.less`:
- Around line 1-2: The header comment lists the Less v3 feature "@plugin" but
the file doesn't use it; either remove "@plugin" from the header comment or add
a minimal, valid `@plugin` usage example in this benchmark file (e.g., a short
`@plugin` declaration and a small rule that demonstrates it) so the header
accurately reflects the file contents; update the header comment or add the
plugin snippet near the top of the file where other feature examples are
located.
In `@packages/less/src/less/deprecation.js`:
- Around line 63-64: Update the default export to include MAX_REPETITIONS
alongside deprecations and DeprecationHandler so the default export mirrors the
named exports; modify the export statement that currently returns {
deprecations, DeprecationHandler } to add MAX_REPETITIONS (referencing the
symbols deprecations, DeprecationHandler, and MAX_REPETITIONS) ensuring
consistency between named and default exports.
In `@packages/less/src/less/parser/parser.js`:
- Line 63: The parser constructs a DeprecationHandler instance
(deprecationHandler) but never calls its summarize method, so suppressed
deprecation warnings are never reported; modify the parser's finish() callback
to invoke deprecationHandler.summarize(logger) (using the same logger passed
around in the parser) just before returning the final result so the summary of
omitted warnings is logged; locate the finish function in parser.js and add a
call to deprecationHandler.summarize(logger) immediately prior to the
return/resolve path that yields the parse result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9736ec06-2c4b-4a2d-94ce-869d2acdc709
📒 Files selected for processing (15)
packages/less/benchmark/benchmark-import-reference-target.lesspackages/less/benchmark/benchmark-import-target.lesspackages/less/benchmark/benchmark-runner.jspackages/less/benchmark/benchmark-v3.lesspackages/less/benchmark/benchmark-v37.lesspackages/less/benchmark/benchmark-v39.lesspackages/less/benchmark/benchmark.lesspackages/less/benchmark/results/.gitignorepackages/less/benchmark/results/latest/macbook-pro_arm64.jsonpackages/less/benchmark/run-historical.shpackages/less/bin/lesscpackages/less/src/less-node/lessc-helper.jspackages/less/src/less/contexts.jspackages/less/src/less/deprecation.jspackages/less/src/less/parser/parser.js
…nner
path.resolve('less') turns the package name into an absolute filesystem
path, preventing Node's package resolution from finding npm-installed
versions. Only resolve relative paths starting with '.'.
…iance_pct variance_pct was computing (max-min)/avg which is range-over-mean. Now uses stddev/avg (coefficient of variation) which is a proper variability statistic.
Prevents same-day runs from overwriting each other in the runs/ archive.
Deprecation warnings from flags like --js, --line-numbers, and --math=always were printed immediately during arg parsing, so --quiet-deprecations only worked if it appeared before the deprecated flag. Now all CLI deprecation messages are queued and flushed after parsing completes, respecting --silent, --quiet, and --quiet-deprecations regardless of flag order.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/less/benchmark/benchmark-runner.js (1)
117-134: Redeclared loop variablei.The loop variable
iis declared twice withvarin the same function scope (lines 117 and 131). While JavaScript hoistsvardeclarations so this works, it triggers linter warnings and can be confusing.♻️ Suggested fix
// Standard deviation and coefficient of variation var sumSqDiff = 0; - for (var i = 0; i < effective.length; i++) { + for (var j = 0; j < effective.length; j++) { - sumSqDiff += (effective[i] - avg) * (effective[i] - avg); + sumSqDiff += (effective[j] - avg) * (effective[j] - avg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/benchmark-runner.js` around lines 117 - 134, The code re-declares the loop variable "i" with var in two separate loops (used when computing total/min/max and when summing squared differences) which triggers linter warnings; change the declarations to block-scoped let (or remove the second var and reuse a single let-declared "i") so each loop uses a non-hoisted iterator, e.g., declare "let i" for the first loop that computes total/min/max and ensure the subsequent loop that accumulates sumSqDiff also uses "let i" (or a differently named iterator) to avoid redeclaration warnings while leaving variables like effective, avg, sumSqDiff, stddev unchanged.packages/less/benchmark/run-historical.sh (2)
300-305: File path in Python code could contain special characters.The
$result_filevariable is interpolated directly into the Python string on line 300. Whilemktemptypically produces safe paths, this pattern is inconsistent with the safer argument-passing approach used elsewhere (e.g., lines 282-298).♻️ Suggested fix for consistency
- if python3 -c "import json; json.load(open('$result_file'))" 2>/dev/null; then + if python3 -c "import json, sys; json.load(open(sys.argv[1]))" "$result_file" 2>/dev/null; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/run-historical.sh` around lines 300 - 305, The python call is injecting $result_file into a double-quoted -c string which can break for special characters; change the check to pass the filename as a separate argument to python instead of interpolating it (e.g., invoke python3 -c with a small script that imports json and sys and reads sys.argv[1]) and then reference the filename via sys.argv[1] so the shell handles quoting safely; update the conditional that currently uses python3 -c "import json; json.load(open('$result_file'))" to this safer argument-passing pattern and keep the surrounding log/err and rm -f "$result_file" behavior unchanged.
247-256: Shell variable interpolation in Python string is fragile.Shell variables (
$tag,$ver,$CURRENT_NODE,$CURRENT_DATE) are interpolated directly into the Python code. If any value contains single quotes or special characters, the Python code will break or behave unexpectedly.While the current inputs are controlled (from
ALL_VERSIONSarray and standard date/node commands), passing values as arguments is more robust.♻️ Suggested safer approach
# Initialize tag JSON via python for safety - tag_json=$(python3 -c " -import json -print(json.dumps({ - 'tag': '$tag', - 'version': '$ver', - 'node_version': '$CURRENT_NODE', - 'date': '$CURRENT_DATE', - 'benchmarks': {} -})) -") + tag_json=$(python3 -c " +import json, sys +print(json.dumps({ + 'tag': sys.argv[1], + 'version': sys.argv[2], + 'node_version': sys.argv[3], + 'date': sys.argv[4], + 'benchmarks': {} +})) +" "$tag" "$ver" "$CURRENT_NODE" "$CURRENT_DATE")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/run-historical.sh` around lines 247 - 256, The current python3 invocation builds JSON by embedding shell variables directly into the -c string (tag_json) which breaks if values contain quotes/special chars; change the call to pass the shell variables as safe arguments/environment to Python (e.g., invoke python3 with sys.argv or read from environment) and construct the JSON inside Python using those arguments (use sys.argv[1], sys.argv[2], etc., or os.environ) to populate 'tag', 'version', 'node_version', 'date', and 'benchmarks' instead of interpolating $tag, $ver, $CURRENT_NODE, $CURRENT_DATE into the command string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/less/benchmark/benchmark-runner.js`:
- Around line 117-134: The code re-declares the loop variable "i" with var in
two separate loops (used when computing total/min/max and when summing squared
differences) which triggers linter warnings; change the declarations to
block-scoped let (or remove the second var and reuse a single let-declared "i")
so each loop uses a non-hoisted iterator, e.g., declare "let i" for the first
loop that computes total/min/max and ensure the subsequent loop that accumulates
sumSqDiff also uses "let i" (or a differently named iterator) to avoid
redeclaration warnings while leaving variables like effective, avg, sumSqDiff,
stddev unchanged.
In `@packages/less/benchmark/run-historical.sh`:
- Around line 300-305: The python call is injecting $result_file into a
double-quoted -c string which can break for special characters; change the check
to pass the filename as a separate argument to python instead of interpolating
it (e.g., invoke python3 -c with a small script that imports json and sys and
reads sys.argv[1]) and then reference the filename via sys.argv[1] so the shell
handles quoting safely; update the conditional that currently uses python3 -c
"import json; json.load(open('$result_file'))" to this safer argument-passing
pattern and keep the surrounding log/err and rm -f "$result_file" behavior
unchanged.
- Around line 247-256: The current python3 invocation builds JSON by embedding
shell variables directly into the -c string (tag_json) which breaks if values
contain quotes/special chars; change the call to pass the shell variables as
safe arguments/environment to Python (e.g., invoke python3 with sys.argv or read
from environment) and construct the JSON inside Python using those arguments
(use sys.argv[1], sys.argv[2], etc., or os.environ) to populate 'tag',
'version', 'node_version', 'date', and 'benchmarks' instead of interpolating
$tag, $ver, $CURRENT_NODE, $CURRENT_DATE into the command string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 134cfeb7-0563-4841-a8ff-bd363b66ca49
📒 Files selected for processing (2)
packages/less/benchmark/benchmark-runner.jspackages/less/benchmark/run-historical.sh
matthew-dean
commented
Mar 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/bin/lessc`:
- Around line 412-414: The --quiet-deprecations flag currently only sets
options.quietDeprecations but the --no-js deprecation still prints immediately
via console.error; update the code path that emits the --no-js deprecation so it
respects options.quietDeprecations by either pushing the no-js message into the
same pendingDeprecations collection or by guarding the immediate console.error
with a check of options.quietDeprecations (use the existing pendingDeprecations
mechanism and the options.quietDeprecations symbol so the deprecation is
suppressed when the flag is set).
This was referenced Mar 10, 2026
This was referenced Jul 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prepares Less.js for the upcoming 5.x release with:
Deprecation System
New
deprecation.jsmodule with categorized deprecation IDs and automatic repetition limiting (max 5 warnings per type per compile). No extra flags needed — works out of the box.New deprecation warnings:
js-eval— inline JavaScript backtick expressionsat-plugin—@plugindirectiveExisting warnings now tagged with stable IDs:
mixin-call-no-parens,mixin-call-whitespace,dot-slash-operatorvariable-in-unknown-value,property-in-unknown-valueCLI:
--quiet-deprecations— suppress only deprecation warnings (keeps other warnings)--quiet— still suppresses everything (unchanged)--js,--line-numbers,--math=alwaysnow print deprecation noticesBenchmark Suite
Comprehensive benchmark harness that tests every major/minor Less release (v2.0–v4.4) using git worktrees for isolation and fnm for Node version management.
benchmark.less— enhanced with extend, guards, property merging, detached rulesets, loops, etc.benchmark-v3.less(v3.6+),benchmark-v37.less(v3.7+),benchmark-v39.less(v3.9+) — version-gated feature benchmarksbenchmark-runner.js— portable runner with ESM interop for v4.xrun-historical.sh— orchestrator with per-system result trackingResults organized as
results/latest/{system-id}.json(tracked) andresults/runs/{date}_{system-id}.json(gitignored).Test plan
@pluginand backtick JS usage--quiet-deprecationssuppresses only deprecation warningsbenchmark/run-historical.shon a clean checkoutSummary by CodeRabbit
New Features
New CLI
Improvements