refactor: convert prototype-based tree nodes to ES6 classes#4412
Merged
matthew-dean merged 4 commits intoless:masterfrom Mar 10, 2026
Merged
refactor: convert prototype-based tree nodes to ES6 classes#4412matthew-dean merged 4 commits intoless:masterfrom
matthew-dean merged 4 commits intoless:masterfrom
Conversation
Convert all 30 tree node files from `Object.assign(new Node(), {...})`
prototype pattern to proper `class extends Node` syntax. This enables
TypeScript to understand the inheritance chain, reducing checkJs errors
from 2756 to 0.
- All tree nodes now use `class X extends Node` (or appropriate parent)
- Node.type converted from instance property to getter for clean override
- Factory functions in index.js updated to use `new` instead of
Object.create + apply (required for ES6 class compatibility)
- Benchmark script converted to ESM
- Added @types/node devDependency for checkJs support
- Enabled checkJs in tsconfig.json
- Added JSDoc types to node.js base class and several utility files
No behavioral changes - all 139 tests pass, benchmark performance
unchanged vs historical baselines (avg 36-39ms for 104KB).
dosubot
bot
added
the
size:XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLarge refactor converting ~40 tree node implementations from function+prototype to ES6 classes, expanding the Node base class with helpers and typedefs, adding JSDoc/type annotations across browser/node utilities and visitors, migrating the benchmark script to ESM, and a few wording/timing/exception handling tweaks. No public API signatures changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
dosubot
bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/less/lib/less/tree/ruleset.js (1)
571-588:⚠️ Potential issue | 🔴 CriticalBug:
createParenthesisaccesseselementsToPak[0]when length is 0.When
elementsToPak.length === 0, the code createsnew Paren(elementsToPak[0])which passesundefinedtoParen. The condition should be=== 1to handle the single-element case; otherwise iterate through the array to wrap each element.🐛 Proposed fix
function createParenthesis(elementsToPak, originalElement) { let replacementParen, j; - if (elementsToPak.length === 0) { + if (elementsToPak.length === 1) { replacementParen = new Paren(elementsToPak[0]); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 571 - 588, The bug is in createParenthesis: it uses elementsToPak[0] when elementsToPak.length === 0; change the conditional to check for elementsToPak.length === 1 and only call new Paren(elementsToPak[0]) in that case; leave the existing loop that builds insideParent and new Paren(new Selector(insideParent)) for lengths > 1, ensuring createParenthesis (and symbols Paren, Element, Selector, originalElement) correctly handle the single-element and multi-element cases (and let the empty-array case fall through to the multi-element branch or be handled explicitly if needed).
🧹 Nitpick comments (8)
packages/less/lib/less/tree/variable.js (1)
14-55: Consider initializingevaluatingproperty in constructor.The
evaluatingproperty (used as a recursion guard at lines 21 and 28) is dynamically added but never declared in the constructor. While this works in JavaScript, explicitly initializing it would improve clarity and align with the pattern of declaring all instance properties in the constructor.♻️ Optional: Declare evaluating in constructor
constructor(name, index, currentFileInfo) { super(); this.name = name; this._index = index; this._fileInfo = currentFileInfo; + this.evaluating = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/variable.js` around lines 14 - 55, Add an explicit initializing declaration for the evaluating flag in the Variable class constructor: declare this.evaluating = false in the constructor (the same constructor that sets this.name/this.index/fileInfo/etc.) so the recursion-guard used in eval (references to this.evaluating at the start of eval and when clearing it) is clearly defined on all instances.packages/less/lib/less/tree/property.js (1)
60-65: Inconsistent property accessor usage.Lines 63-64 use
this.currentFileInfo.filenameandthis.index, while lines 23-24 usethis.fileInfo().filenameandthis.getIndex(). Although they resolve to the same values (the properties are getters delegating to the methods), using a consistent style would improve readability.♻️ Suggested consistency fix
} else { throw { type: 'Name', message: `Property '${name}' is undefined`, - filename: this.currentFileInfo.filename, - index: this.index }; + filename: this.fileInfo().filename, + index: this.getIndex() }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/property.js` around lines 60 - 65, In the thrown error object in property.js, replace direct property access this.currentFileInfo.filename and this.index with the consistent accessor calls used elsewhere: this.fileInfo().filename and this.getIndex(); update the throw block in the same method (the block that constructs the Name error) so it uses these accessor methods to match other usages like fileInfo() and getIndex().packages/less/lib/less/visitors/set-tree-visibility-visitor.js (1)
35-54: Consider usingArray.isArray()instead of constructor comparison.Line 39 uses
node.constructor === Arraywhich can fail for arrays from different realms (iframes, etc.). UsingArray.isArray(node)is more robust and idiomatic.💡 Suggested improvement
visit(node) { if (!node) { return node; } - if (node.constructor === Array) { + if (Array.isArray(node)) { return this.visitArray(node); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/visitors/set-tree-visibility-visitor.js` around lines 35 - 54, In the visit method, replace the fragile constructor check node.constructor === Array with a robust Array.isArray(node) check so arrays from other realms are detected; update the condition that currently calls this.visitArray(node) to use Array.isArray(node) instead, leaving the rest of visit, visitArray, and subsequent behavior (ensureVisibility/ensureInvisibility and node.accept(this)) unchanged.packages/less/lib/less/tree/declaration.js (1)
101-107: Consider preserving thevariableparameter inmakeImportant.The
makeImportantmethod doesn't pass thevariableparameter to the newDeclaration, which means it will be re-derived from the name (Line 31-32). If the original declaration had an explicitly setvariablevalue that differs from the name-based derivation, this could be lost.This appears to be pre-existing behavior from before the class conversion, so it may be intentional. Verify if preserving
this.variableis needed.💡 Optional fix to preserve variable
makeImportant() { return new Declaration(this.name, this.value, '!important', this.merge, - this.getIndex(), this.fileInfo(), this.inline); + this.getIndex(), this.fileInfo(), this.inline, this.variable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/declaration.js` around lines 101 - 107, The makeImportant() factory currently constructs a new Declaration without preserving the original this.variable, so any explicitly-set variable flag is lost; update makeImportant() to pass this.variable into the Declaration constructor (preserving the original variable value) and ensure you place it at the correct parameter position to match Declaration's signature (e.g., include this.variable before/at the parameter spot where Declaration expects the variable flag such as the variable/merge boundary) so the new Declaration retains the same variable state as the original.packages/less/lib/less/tree/unit.js (1)
51-53: Consider caching the RegExp forisLength.Creating a new
RegExpon every call is slightly inefficient. This is pre-existing behavior, but since you're refactoring this file, it could be a good opportunity to optimize.💡 Optional optimization
+const LENGTH_UNITS_RE = /^(px|em|ex|ch|rem|in|cm|mm|pc|pt|ex|vw|vh|vmin|vmax)$/i; + class Unit extends Node { // ... other code ... isLength() { - return RegExp('^(px|em|ex|ch|rem|in|cm|mm|pc|pt|ex|vw|vh|vmin|vmax)$', 'gi').test(this.toCSS()); + return LENGTH_UNITS_RE.test(this.toCSS()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/unit.js` around lines 51 - 53, The isLength method creates a new RegExp on every call; define a module-level cached RegExp (e.g., const LENGTH_RE = /^(px|em|ex|ch|rem|in|cm|mm|pc|pt|vw|vh|vmin|vmax)$/i) and replace the RegExp(...) construction in isLength with LENGTH_RE.test(this.toCSS()); ensure you drop the global 'g' flag to avoid stateful .test() behavior and reference the isLength method and this.toCSS() when making the change.packages/less/lib/less/tree/color.js (2)
32-38: UseforEachinstead ofmapwhen not using return values.Same issue as above—the callback only produces side effects.
♻️ Suggested fix
- rgb.split('').map(function (c, i) { + rgb.split('').forEach(function (c, i) { if (i < 3) { self.rgb.push(parseInt(c + c, 16)); } else { self.alpha = (parseInt(c + c, 16)) / 255; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/color.js` around lines 32 - 38, The callback passed to rgb.split('').map in color.js produces only side effects, so replace .map with .forEach to avoid creating an unused array; update the call on the rgb.split('') result (within the constructor/parse logic that pushes into self.rgb and sets self.alpha) to use .forEach with the same callback so behavior is unchanged but no unnecessary array is allocated.
23-29: UseforEachinstead ofmapwhen not using return values.The callback doesn't return a value, making
map()misleading. Since you're only using the side effect of populatingthis.rgb,forEach()is the correct method.♻️ Suggested fix
- rgb.match(/.{2}/g).map(function (c, i) { + rgb.match(/.{2}/g).forEach(function (c, i) { if (i < 3) { self.rgb.push(parseInt(c, 16)); } else { self.alpha = (parseInt(c, 16)) / 255; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/color.js` around lines 23 - 29, Replace the use of Array.prototype.map (which returns a new array) with Array.prototype.forEach since the callback only performs side effects; specifically change the call on rgb.match(/.{2}/g).map(...) to use .forEach(...) and keep the body that pushes into self.rgb and sets self.alpha (referencing self.rgb and self.alpha) unchanged.packages/less/lib/less/tree/expression.js (1)
63-66: Operator precedence issue: missing parentheses in condition.The
||has lower precedence thaninstanceofand&&, so the current expression is parsed as:(!(... instanceof Anonymous)) || ((... instanceof Anonymous) && (value !== ','))This appears to be the intended logic (add space unless next element is an Anonymous with value
,), so the behavior is correct. However, the redundant secondinstanceofcheck could be simplified.♻️ Suggested simplification
- if (!(this.value[i + 1] instanceof Anonymous) || - this.value[i + 1] instanceof Anonymous && this.value[i + 1].value !== ',') { + if (!(this.value[i + 1] instanceof Anonymous) || this.value[i + 1].value !== ',') { output.add(' '); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/expression.js` around lines 63 - 66, The condition around output.add(' ') is redundant: instead of checking "if (!(this.value[i+1] instanceof Anonymous) || this.value[i+1] instanceof Anonymous && this.value[i+1].value !== ',')" simplify to a single, clear test that adds a space unless the next element is an Anonymous with value ','; replace it with a single negated conjunction such as "if (!(this.value[i+1] instanceof Anonymous && this.value[i+1].value === ','))" referencing the same this.value and Anonymous symbols so the logic is equivalent and easier to read.
🤖 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/index.js`:
- Line 9: The code uses path.join when overriding file with CLI input (the
expression handling process.argv[2] and assigning to file), which incorrectly
concatenates absolute paths; change that assignment to use path.resolve instead
of path.join so absolute paths are preserved and relative paths are resolved
against process.cwd(); update the branch that checks process.argv[2] and replace
path.join(...) with path.resolve(...) wherever the file override occurs (look
for the variable file and the use of path.join in that conditional).
In `@packages/less/lib/less/tree/atrule.js`:
- Around line 21-24: The code constructs a Selector with this._index and
this._fileInfo before those properties are initialized; move the assignments
that set this._index and this._fileInfo from the constructor parameters (index,
fileInfo) so they occur before the call to (new
Selector(...)).createEmptySelectors(), i.e., assign this._index = index;
this._fileInfo = fileInfo; (and any related property initializations) prior to
creating the empty selectors so Selector receives the correct values.
In `@packages/less/lib/less/tree/node.js`:
- Around line 190-206: The comparison logic in Node.compare uses aVal and bVal
but only checks that aVal is an array; add a defensive check that bVal is an
array too before reading bVal.length or indexing into bVal. In the Node.compare
implementation, after confirming Array.isArray(aVal) and before comparing
lengths or iterating, ensure Array.isArray(bVal) is true and return undefined if
not, so both arrays are guaranteed when using bVal.length and bVal[i].
In `@packages/less/package.json`:
- Line 75: The package.json currently pins "@types/node" to "~25.4.0" which
mismatches the declared Node support in "engines.node" (Node 18+); update the
dependency so typings align with the lowest supported Node major: change the
"@types/node" entry to a version compatible with Node 18 (e.g., "^18.x.x") or
alternatively tighten "engines.node" to require Node 25+, ensuring consistency
between the "engines.node" field and the "@types/node" dependency in
package.json.
---
Outside diff comments:
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 571-588: The bug is in createParenthesis: it uses elementsToPak[0]
when elementsToPak.length === 0; change the conditional to check for
elementsToPak.length === 1 and only call new Paren(elementsToPak[0]) in that
case; leave the existing loop that builds insideParent and new Paren(new
Selector(insideParent)) for lengths > 1, ensuring createParenthesis (and symbols
Paren, Element, Selector, originalElement) correctly handle the single-element
and multi-element cases (and let the empty-array case fall through to the
multi-element branch or be handled explicitly if needed).
---
Nitpick comments:
In `@packages/less/lib/less/tree/color.js`:
- Around line 32-38: The callback passed to rgb.split('').map in color.js
produces only side effects, so replace .map with .forEach to avoid creating an
unused array; update the call on the rgb.split('') result (within the
constructor/parse logic that pushes into self.rgb and sets self.alpha) to use
.forEach with the same callback so behavior is unchanged but no unnecessary
array is allocated.
- Around line 23-29: Replace the use of Array.prototype.map (which returns a new
array) with Array.prototype.forEach since the callback only performs side
effects; specifically change the call on rgb.match(/.{2}/g).map(...) to use
.forEach(...) and keep the body that pushes into self.rgb and sets self.alpha
(referencing self.rgb and self.alpha) unchanged.
In `@packages/less/lib/less/tree/declaration.js`:
- Around line 101-107: The makeImportant() factory currently constructs a new
Declaration without preserving the original this.variable, so any explicitly-set
variable flag is lost; update makeImportant() to pass this.variable into the
Declaration constructor (preserving the original variable value) and ensure you
place it at the correct parameter position to match Declaration's signature
(e.g., include this.variable before/at the parameter spot where Declaration
expects the variable flag such as the variable/merge boundary) so the new
Declaration retains the same variable state as the original.
In `@packages/less/lib/less/tree/expression.js`:
- Around line 63-66: The condition around output.add(' ') is redundant: instead
of checking "if (!(this.value[i+1] instanceof Anonymous) || this.value[i+1]
instanceof Anonymous && this.value[i+1].value !== ',')" simplify to a single,
clear test that adds a space unless the next element is an Anonymous with value
','; replace it with a single negated conjunction such as "if (!(this.value[i+1]
instanceof Anonymous && this.value[i+1].value === ','))" referencing the same
this.value and Anonymous symbols so the logic is equivalent and easier to read.
In `@packages/less/lib/less/tree/property.js`:
- Around line 60-65: In the thrown error object in property.js, replace direct
property access this.currentFileInfo.filename and this.index with the consistent
accessor calls used elsewhere: this.fileInfo().filename and this.getIndex();
update the throw block in the same method (the block that constructs the Name
error) so it uses these accessor methods to match other usages like fileInfo()
and getIndex().
In `@packages/less/lib/less/tree/unit.js`:
- Around line 51-53: The isLength method creates a new RegExp on every call;
define a module-level cached RegExp (e.g., const LENGTH_RE =
/^(px|em|ex|ch|rem|in|cm|mm|pc|pt|vw|vh|vmin|vmax)$/i) and replace the
RegExp(...) construction in isLength with LENGTH_RE.test(this.toCSS()); ensure
you drop the global 'g' flag to avoid stateful .test() behavior and reference
the isLength method and this.toCSS() when making the change.
In `@packages/less/lib/less/tree/variable.js`:
- Around line 14-55: Add an explicit initializing declaration for the evaluating
flag in the Variable class constructor: declare this.evaluating = false in the
constructor (the same constructor that sets this.name/this.index/fileInfo/etc.)
so the recursion-guard used in eval (references to this.evaluating at the start
of eval and when clearing it) is clearly defined on all instances.
In `@packages/less/lib/less/visitors/set-tree-visibility-visitor.js`:
- Around line 35-54: In the visit method, replace the fragile constructor check
node.constructor === Array with a robust Array.isArray(node) check so arrays
from other realms are detected; update the condition that currently calls
this.visitArray(node) to use Array.isArray(node) instead, leaving the rest of
visit, visitArray, and subsequent behavior (ensureVisibility/ensureInvisibility
and node.accept(this)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4da9e184-2ac1-4781-b468-d711b31662ca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
packages/less/benchmark/index.jspackages/less/lib/less-browser/add-default-options.jspackages/less/lib/less-browser/utils.jspackages/less/lib/less-node/fs.jspackages/less/lib/less/deprecation.jspackages/less/lib/less/functions/style.jspackages/less/lib/less/index.jspackages/less/lib/less/transform-tree.jspackages/less/lib/less/tree/anonymous.jspackages/less/lib/less/tree/assignment.jspackages/less/lib/less/tree/atrule.jspackages/less/lib/less/tree/attribute.jspackages/less/lib/less/tree/call.jspackages/less/lib/less/tree/color.jspackages/less/lib/less/tree/combinator.jspackages/less/lib/less/tree/comment.jspackages/less/lib/less/tree/condition.jspackages/less/lib/less/tree/container.jspackages/less/lib/less/tree/declaration.jspackages/less/lib/less/tree/detached-ruleset.jspackages/less/lib/less/tree/dimension.jspackages/less/lib/less/tree/element.jspackages/less/lib/less/tree/expression.jspackages/less/lib/less/tree/extend.jspackages/less/lib/less/tree/import.jspackages/less/lib/less/tree/javascript.jspackages/less/lib/less/tree/js-eval-node.jspackages/less/lib/less/tree/keyword.jspackages/less/lib/less/tree/media.jspackages/less/lib/less/tree/mixin-call.jspackages/less/lib/less/tree/mixin-definition.jspackages/less/lib/less/tree/namespace-value.jspackages/less/lib/less/tree/negative.jspackages/less/lib/less/tree/node.jspackages/less/lib/less/tree/operation.jspackages/less/lib/less/tree/paren.jspackages/less/lib/less/tree/property.jspackages/less/lib/less/tree/query-in-parens.jspackages/less/lib/less/tree/quoted.jspackages/less/lib/less/tree/ruleset.jspackages/less/lib/less/tree/selector.jspackages/less/lib/less/tree/unicode-descriptor.jspackages/less/lib/less/tree/unit.jspackages/less/lib/less/tree/url.jspackages/less/lib/less/tree/value.jspackages/less/lib/less/tree/variable-call.jspackages/less/lib/less/tree/variable.jspackages/less/lib/less/visitors/set-tree-visibility-visitor.jspackages/less/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/less/lib/less/tree/atrule.js (1)
140-152:⚠️ Potential issue | 🔴 CriticalGuard empty blocks before touching
rules[0].
outputRuleset()already treats[]as valid, buteval()does not. An empty simple block such as@font-face {}leavesrules === []; Line 141 passes that intoevalRoot(), and Line 205 dereferencesrules[0]. Even if that path is skipped, Line 152 makes the same assumption. Add a zero-length guard so empty at-rules can round-trip instead of throwing.🐛 Proposed fix
- if (rules) { + if (rules && rules.length > 0) { rules = this.evalRoot(context, rules); } @@ - if (this.simpleBlock && rules) { + if (this.simpleBlock && rules && rules.length > 0) { rules[0].functionRegistry = context.frames[0].functionRegistry.inherit(); rules = rules.map(function (rule) { return rule.eval(context); }); } @@ evalRoot(context, rules) { + if (!rules.length) { + return rules; + } + let ampersandCount = 0; let noAmpersandCount = 0; let noAmpersands = true;Also applies to: 162-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/atrule.js` around lines 140 - 152, The code assumes rules[0] exists when handling at-rules, which breaks for empty simple blocks (rules === []); update the checks in the eval logic (around evalRoot usage and the branches using declarationsBlock, mergeRules, and simpleBlock) to guard against zero-length arrays before dereferencing rules[0] (e.g., ensure Array.isArray(rules) && rules.length > 0 before accessing rules[0].rules or setting rules[0].functionRegistry); apply the same zero-length guard to the other similar block in the 162-207 region so empty at-rules like `@font-face` {} are preserved instead of throwing.
🧹 Nitpick comments (1)
packages/less/lib/less/tree/atrule.js (1)
22-24: Avoid allocating and parenting a throwaway selector list.The selector array created at Line 23 is never attached to the instance. In the non-array branch, Line 53 creates a second empty selector list, while Line 61 only calls
setParent()on the detached first one. This adds extra work on a hot path and makes the ownership of the real selectors harder to follow. Create the empty selectors lazily in the branch that needs them and reuse that same reference.♻️ Possible cleanup
- let i; - var selectors = (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors(); + let i; + let selectors; @@ - this.rules = [rules]; - this.rules[0].selectors = (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors(); + this.rules = [rules]; + selectors = (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors(); + this.rules[0].selectors = selectors; @@ - this.setParent(selectors, this); + if (selectors) { + this.setParent(selectors, this); + } this.setParent(this.rules, this);Also applies to: 53-53, 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/atrule.js` around lines 22 - 24, The code currently always allocates a throwaway Selector list via new Selector(...).createEmptySelectors() assigned to the local selectors var but only needs it in the non-array branch; instead, create the empty selector list lazily inside the branch that requires it and reuse that same reference for both attaching and calling setParent(), removing the early allocation and the separate second creation at the non-array branch. Update the logic around the selectors variable, Selector.createEmptySelectors() usage, and the setParent() call so the same selector list instance is created once when needed and then passed/parented where required.
🤖 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/index.js`:
- Around line 9-13: The fs.readFile callback uses data.length without checking
the error parameter (e), so a failed read will cause a TypeError and hide the
real I/O error; update the fs.readFile callback in
packages/less/benchmark/index.js (the function handling fs.readFile(file,
'utf8', function (e, data) {...})) to first check if e is truthy and log or
throw the actual error (and exit/return) before accessing data or data.length,
ensuring the real fs error is surfaced instead of dereferencing undefined.
---
Outside diff comments:
In `@packages/less/lib/less/tree/atrule.js`:
- Around line 140-152: The code assumes rules[0] exists when handling at-rules,
which breaks for empty simple blocks (rules === []); update the checks in the
eval logic (around evalRoot usage and the branches using declarationsBlock,
mergeRules, and simpleBlock) to guard against zero-length arrays before
dereferencing rules[0] (e.g., ensure Array.isArray(rules) && rules.length > 0
before accessing rules[0].rules or setting rules[0].functionRegistry); apply the
same zero-length guard to the other similar block in the 162-207 region so empty
at-rules like `@font-face` {} are preserved instead of throwing.
---
Nitpick comments:
In `@packages/less/lib/less/tree/atrule.js`:
- Around line 22-24: The code currently always allocates a throwaway Selector
list via new Selector(...).createEmptySelectors() assigned to the local
selectors var but only needs it in the non-array branch; instead, create the
empty selector list lazily inside the branch that requires it and reuse that
same reference for both attaching and calling setParent(), removing the early
allocation and the separate second creation at the non-array branch. Update the
logic around the selectors variable, Selector.createEmptySelectors() usage, and
the setParent() call so the same selector list instance is created once when
needed and then passed/parented where required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e7cd3de-d287-47f3-a1d0-4bec32bdebba
📒 Files selected for processing (4)
packages/less/benchmark/index.jspackages/less/lib/less/deprecation.jspackages/less/lib/less/parser/parser.jspackages/less/lib/less/tree/atrule.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/less/lib/less/deprecation.js
4 tasks
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
Foo.prototype = Object.assign(new Node(), {...})to properclass Foo extends NodesyntaxNode.typechanged from instance property to getter, enabling clean subclass overrides viaget type() { return 'Foo'; }index.jsupdated fromObject.create(t.prototype) + t.apply()tonew t(...)(required for ES6 class compatibility)Inheritance chain
NodeRulesetNodeAtRuleNode(+NestableAtRulePrototypemixin)MixinDefinitionRulesetMedia,ContainerAtRule(+NestableAtRulePrototypemixin)JsEvalNodeNodeJavaScriptJsEvalNodePerformance
No regression vs historical baselines (benchmark.less, 104KB):
Test plan
grunt shell:test)tsc --noEmitpasses with 0 errors (checkJs enabled)Object.create/.applypatterns that could break with class constructorsSummary by CodeRabbit
Refactoring
Documentation
Bug Fixes / Reliability
Chores