feat: Add keep_at_rules option
#485
Conversation
kamilzych
force-pushed
the
feat/keep-at-rules
branch
from
155e7b6 to
64161f7
Compare
CodSpeed Performance ReportMerging #485 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #485 +/- ##
==========================================
- Coverage 88.05% 87.79% -0.27%
==========================================
Files 18 18
Lines 1993 1983 -10
==========================================
- Hits 1755 1741 -14
- Misses 238 242 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // TODO there must be a way for not having to instantiate Attributes class, see serializer.rs | ||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
I'd suggest restructuring start_elem with generics. For example, you can accept some attribute_writer: F:
where F: Fn(W) -> Result<(), InlineError>I.e., replace explicit attributes with a callback and call it inside start_elem. For the existing places, the callback could be a closure that writes to the writer, and then for the new place, it could be a no-op closure. I'd expect the compiler to inline that closure and produce the equivalent machine code as we have now, so the performance impact should be minimal.
Let me know if I can elaborate on this
There was a problem hiding this comment.
Or maybe just keep the old version and write tag name + content manually without calling start_elem? it could be both faster and simpler
| keep_at_rules: bool, | ||
| at_rules: Option<String>, |
There was a problem hiding this comment.
I am fine with >7 args, you could add allow attribute for that lint (seeing it right now in CI)
| // TODO this is not used here but I might need it for simplifying serialization logic below | ||
| keep_at_rules: bool, | ||
| at_rules: Option<String>, |
There was a problem hiding this comment.
Indeed, I believe, the Some variant for at_rules already indicates we need to apply that filtering logic, so keep_at_rules could be removed as redundant.
| @@ -60,6 +71,8 @@ impl<'a> Sink<'a> { | |||
| node, | |||
| self.keep_style_tags, | |||
| self.keep_link_tags, | |||
| self.keep_at_rules, | |||
| self.at_rules.clone(), | |||
There was a problem hiding this comment.
We need to avoid clone - it would be better to pass a reference to at_rules, i.e., Option<&T>. Right now, I am not sure if String would be the optimal structure here. But the idea is that you can pass at_rules.as_ref() to Sink. Lifetime could be the same as for document (in this context it is fine)
| @@ -114,6 +127,22 @@ impl<'a> Sink<'a> { | |||
|
|
|||
| serializer.start_elem(&element.name, &element.attributes, style_node_id)?; | |||
|
|
|||
| // TODO this part is the one that I don't like the most | |||
There was a problem hiding this comment.
I am not 100% sure about this one, even though it is a sound approach.
The first thought that I have is that the least surprising thing would be to keep @ rules in their original places, instead of pushing everything into a single node. This won't work for external stylesheets, so we need to store them somewhere anyway.
On the other hand, this approach is better for performance, and probably it would not be super surprising for the user anyway if we put everything under one <style> tag.
So, in the end, I am leaning towards this approach.
A few other notes:
- We can reduce the performance hit of checking each node name by having two versions of
serialize- the first one will work as before this PR. The new one will check forheadand once it is found, it will switch to the other version ofserializethat won't check the tag name. At the top level we check ifat_rulesare present and then call one of these two versions - I think it should minimize the impact asheadis usually in the very beginning of the nodes vector. - Are there cases when
headis not present? Maybe in HTML fragments? seeinline_fragment
There was a problem hiding this comment.
btw, if the user chose to keep style tags - I think we can only extract @ rule from link, as the old style tags will be around anyway
| let mut parser = CSSDeclarationListParser; | ||
| let parser = cssparser::RuleBodyParser::new(input, &mut parser); | ||
| let start = self.declarations.len(); | ||
| for item in parser.flatten() { | ||
| self.declarations.push(item); | ||
| } | ||
| Ok((prelude, (start, self.declarations.len()))) |
There was a problem hiding this comment.
It looks similar to the existing implementation - I suggest extracting it into a separate function.
| name: cssparser::CowRcStr<'i>, | ||
| input: &mut cssparser::Parser<'i, 't>, | ||
| ) -> Result<Self::Prelude, cssparser::ParseError<'i, Self::Error>> { | ||
| // TODO pushing @ feels odd, there should be a less dumb way of doing it? |
There was a problem hiding this comment.
I am not sure if the parser provides any methods to extract the whole thing, but it could, even though I would not put much trust in this assumption :) I am ok with pushing @ explicitly
Rel #265 (comment)