Closed nex3 closed 6 months ago
Hi @nex3,
Thank you for reaching out about this.
I think the root cause is how we walk
AST's.
This is used in parseComponentValue
The current walk
function implementations roughly work like this :
The issue you are encountering is between 2.
and 4.
.
We don't check that the child node is still a child node of the current AST tree after 2.
I think we can trivially check in 3.
that a child node is not only walkable, but also still part of the same parent node on which walk
was last called.
In your case it would no longer be part of the same parent and baz
would not be visited.
But it also introduces another question.
For this tree :
A
- 1
- 2
- I
- 9
If we replace A.2
with this tree :
3
- III
We form :
A
- 1
- 3
- II
- 9
We can prevent visiting A.2.I
with a new check, but is it expected that A.3
and/or A.3.II
are visited?
There is no plugin system here, as there is in PostCSS. So I assume that there is always only a single walker callback and that the AST being inserted is always final and doesn't need further manipulation in the same AST walk.
So I think it's fine to assume that we don't want to visit A.3
and A.3.II
and that the next callback would be on A.9
?
I hope this notation and naming is a bit clear and that I am not making things worse.
I think not visiting A.3
and A.3.II
is the best behavior. I think that's what PostCSS does in a similar situation.
One thing that's worth noting: PostCSS provides strong guarantees that modifying an AST during a walk is safe by storing the index of each active iteration and updating them as necessary when replacements are made. It would be really nice if this tool did so as well, although somewhat out-of-scope for this issue.
I took a closer look at what PostCSS does and found that it does visit child nodes, even after they are replaced :
const creator = () => {
return {
postcssPlugin: "creator",
Root: (root, helpers) => {
root.walk((x) => {
console.log(x.type);
if (x.type === 'rule') {
x.replaceWith(helpers.atRule({ name: 'media', params: '(min-width: 100px)' }));
}
});
}
};
};
creator.postcss = true;
.baz {
/* A comment */
color: green;
}
This prints
rule
comment
decl
And postcss-value-parser
has the same behavior :
const ast = valueParser('foo(bar(baz))');
ast.walk((node) => {
console.log(node.type, node.value);
if (node.type === 'function' && node.value === 'bar') {
// postcss-value-parser doesn't expose the parent during walking
ast.nodes[0].nodes.splice(0, 1, {
type: 'word',
value: 'x',
})
}
})
function foo
function bar
word baz
Is it possible that this goes unnoticed because it is more common to mutate nodes with PostCSS and PostCSS-like libs?
That said, I think it's obvious that this is sub optimal behavior and that we should fix this.
the patch hasn't been released yet
@nex3 Patches for this issue have been released as part of 2.4.0
: https://github.com/csstools/postcss-plugins/blob/main/packages/css-parser-algorithms/CHANGELOG.md#240
Can you let us know if this resolves the bugs you encountered?
It'll be a bit before I can get back to trying to integrate this, but I'll keep you posted when we do.
@nex3 going to close this for now. Please let me know if you are still facing issues with this.
(Runkit)
In line with other AST-walking and -replacement APIs, I would expect that if I replace a parent node in the AST, the replacement algorithm will stop traversing any of that node's children. (Note that this is how PostCSS's
walk()
andreplaceWith()
interact.) Instead,replaceComponentValues()
continues traversing the removed nodes. This is extra unnecessary work and can even cause data corruption if, for example, information is being stored about nodes encountered during the traversal.