Skip to content

сhore(agtree): add node-specific clone utility#186

Open
SatoruFF wants to merge 1 commit intoAdguardTeam:masterfrom
SatoruFF:feat/agtree-node-clone-utility
Open

сhore(agtree): add node-specific clone utility#186
SatoruFF wants to merge 1 commit intoAdguardTeam:masterfrom
SatoruFF:feat/agtree-node-clone-utility

Conversation

@SatoruFF
Copy link
Copy Markdown

Closes? #153

The converters were using clone-deep to copy rule nodes before mutating them.
Since we know the exact shape of every node type, a generic deep-clone is overkill —
so this adds node-specific clone functions instead.

What's added

  • cloneAnyRule — main entry point, dispatches by node.category
  • cloneAnyCommentRule, cloneAnyNetworkRule, cloneAnyCosmeticRule — typed generics
    so the specific subtype is preserved at call sites
  • Existing helpers (cloneScriptletRuleNode etc.) are untouched

What's changed

Replaced clone(rule) + TODO comments in three converter files with the new functions.

Tests

test/ast-utils/clone.test.ts — covers all rule categories, checks both value equality
and that no object references are shared between original and clone.

Copy link
Copy Markdown
Member

@scripthunter7 scripthunter7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting together this PR, but I think it's a bit too early to merge it. There is already a task for this in AGTree v5 (which you also linked), and we're planning to implement it at the appropriate stage.

It's important to note that v5 will also modify and improve the node types to some extent, and we’re not planning to add this kind of feature to v4 anymore.

// raws only has primitive fields (text, nl), but it's still an object reference
type Raws = NonNullable<import('../nodes').RuleBase['raws']>;
function cloneRaws(raws: Raws | undefined): Raws | undefined {
return raws ? { ...raws } : undefined;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a small benchmark script that showed it's more efficient to list properties explicitly rather than using the spread operator, enumerating properties is about 1.5x faster, but depends on the platform. If we want to avoid generic solutions anyway, it's a better idea to go with that approach.

function cloneHtmlFilteringRuleBody(node: HtmlFilteringRuleBody): HtmlFilteringRuleBody {
const selectorList: SelectorList = {
...node.selectorList,
children: node.selectorList.children.map((complexSelector) => ({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using map, it's more efficient to preallocate an array of the same length and fill it using a simple for loop without any spread operator.

result.params = cloneScriptletRuleNode(node.params);
} else {
// expression nodes are rare and have no dedicated clone path yet
result.params = structuredClone(node.params);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, we should avoid using structuredClone where possible, as it is quite slow, although this can be platform-dependent. It's interesting by the way, because at first you wouldn't expect this, since it's a native implementation, but even clone-deep can outperform it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants