Preserve rules priority while expanding shorthands #119

ojab · 2021-01-19T17:39:06Z

We can piggyback on Hash preserving it's order in all supported rubies & drop order. Looks quite ugly, but does the trick.
I assume that it will be faster if we'll properly track and propagate order in all places, but I suspect it will be also harder to work with.

And we're little faster at parsing because of order is removed:
Before:

Executing: bundle exec rake benchmark
Warming up --------------------------------------
 import1.css loading   376.000  i/100ms
 complex.css loading    14.000  i/100ms
Calculating -------------------------------------
 import1.css loading      3.670k (± 3.6%) i/s -     18.424k in   5.027200s
 complex.css loading    142.421  (± 4.2%) i/s -    714.000  in   5.021913s

Loading `import1.css` allocated 372 objects, 26 KiB
Loading `complex.css` allocated 10437 objects, 752 KiB

After:

Executing: bundle exec rake benchmark
Warming up --------------------------------------
 import1.css loading   394.000  i/100ms
 complex.css loading    14.000  i/100ms
Calculating -------------------------------------
 import1.css loading      3.809k (± 3.7%) i/s -     19.306k in   5.075438s
 complex.css loading    147.464  (± 3.4%) i/s -    742.000  in   5.037139s

Loading `import1.css` allocated 357 objects, 25 KiB
Loading `complex.css` allocated 9536 objects, 680 KiB

And little slower at expanding shorthands (tested on github's 500KB css with parser.each_rule_set { |rule_set, _media_type| rule_set.expand_shorthand! }, not sure how relevant this file is though)
Before:

Executing: bundle exec ruby b.rb
Warming up --------------------------------------
github-bf18d76d7dfd1ad6ed77f31817d6c749.css loading
                         1.000  i/100ms
Calculating -------------------------------------
github-bf18d76d7dfd1ad6ed77f31817d6c749.css loading
                          2.798  (± 0.0%) i/s -     84.000  in  30.089636s
Loading `github.css` and expanding shothands allocated 439154 objects, 29732 KiB

After:

Executing: bundle exec ruby b.rb
Warming up --------------------------------------
github-bf18d76d7dfd1ad6ed77f31817d6c749.css loading
                         1.000  i/100ms
Calculating -------------------------------------
github-bf18d76d7dfd1ad6ed77f31817d6c749.css loading
                          2.789  (± 0.0%) i/s -     84.000  in  30.161549s
Loading `github.css` and expanding shothands allocated 437074 objects, 30186 KiB
Successfully rebased and updated refs/heads/fixup_expanding_shorthands.

Fixes #111

lib/css_parser/rule_set.rb

grosser · 2021-01-19T17:46:29Z

lib/css_parser/rule_set.rb

@@ -113,8 +113,44 @@ def delete(property)
      end
      alias remove_declaration! delete

+      def replace_declaration!(property, replacements)


I was expecting this to be something like:

declarations.fetch(property) # blow up when unknown declarations.each_with_object({}) do |(k, v), h| next if replacements.key?(k) if k == property h.merge! replacements else h[k] = v end end

We should obey !important & declarations order, because we shouldn't override subsequent declarations and declarations before with !important if we're not, so I don't see easier way to do that. Will add .key? check for the replaced property though.

grosser

I like the idea!

ojab · 2021-01-19T18:03:31Z

So yeah, it's more or less RFC at this point and I will add Declarations#replace_declaration! tests, just want to make sure that this approach is acceptable.

grosser · 2021-01-21T18:33:25Z

lib/css_parser/rule_set.rb

        end

        def value=(value)
-          value = value.to_s.sub(/\s*;\s*\Z/, '').gsub(CssParser::IMPORTANT_IN_PROPERTY_RX, '').strip
+          value = value.to_s.sub(/\s*;\s*\Z/, '')
+          self.important = !value.slice!(CssParser::IMPORTANT_IN_PROPERTY_RX).nil?


grosser · 2021-01-21T18:36:16Z

lib/css_parser/rule_set.rb

+        replacement_values = declarations.values
+        property_index = replacement_keys.index(property)
+
+        replacements = replacement_declarations.each.with_object({}) do |(key, value), result|


some comment on what this section does / why it's needed

grosser · 2021-01-21T18:36:58Z

lib/css_parser/rule_set.rb

+            next
+          end
+
+          next if declarations[key].important && !value.important


looks like the the if above, maybe assign to a variable ?

... or do some if + if/else flow

it's opposite of the one above, made order the same to make it more clear.

grosser · 2021-01-21T18:37:38Z

lib/css_parser/rule_set.rb

      def each(&block)
-        declarations.sort_by { |_name, value| value.order }.each(&block)
+        declarations.each(&block)
      end


can forward this

grosser

looks great!
... lmk when you thing this is ready to merge

grosser · 2021-01-21T18:59:06Z

CI failed

grosser · 2021-01-21T19:04:15Z

lib/css_parser/rule_set.rb

+          if !declarations.key?(key) || (value.important && !declarations[key].important)
+            result[key] = value
+            next unless declarations.key?(key)
+
+            replaced_index = replacement_keys.index(key)
+            replacement_keys.delete_at(replaced_index)
+            replacement_values.delete_at(replaced_index)
+            property_index -= 1 if replaced_index < property_index
+            next
+          end


Suggested change

if !declarations.key?(key) || (value.important && !declarations[key].important)

result[key] = value

next unless declarations.key?(key)

replaced_index = replacement_keys.index(key)

replacement_keys.delete_at(replaced_index)

replacement_values.delete_at(replaced_index)

property_index -= 1 if replaced_index < property_index

next

end

if !declarations.key?(key)

result[key] = value

elsif !declarations[key].important && value.important # explain what this means and does

result[key] = value

replaced_index = replacement_keys.index(key)

replacement_keys.delete_at(replaced_index)

replacement_values.delete_at(replaced_index)

property_index -= 1 if replaced_index < property_index

elsif declarations[key].important && value.important # explain what this means

# discard

elsif property_index < replacement_keys.index(key) # explain what this means

# discard

else

result[key] = value

end

Added more comments to the existing code.

great comments!

ojab · 2021-01-21T19:18:07Z

Yeah, trying to test forwardable, but looks like it's not trivial here minitest/minitest#545

Fixes premailer#111

ojab · 2021-01-21T19:28:09Z

Comments are more or less addressed, CI is passing, I think it's ready if everything is fine for you.

ojab · 2021-01-21T19:35:51Z

Thanks 🎉! Can I haz release?

grosser · 2021-01-21T19:41:05Z

1.8.0

…

On Thu, Jan 21, 2021 at 11:36 AM Slava Kardakov ***@***.***> wrote: Thanks 🎉! Can I haz release? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#119 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACYZ372C227ZMGZHBL4F3S3B62PANCNFSM4WJEACNA> .

ojab marked this pull request as draft January 19, 2021 17:40

grosser reviewed Jan 19, 2021

View reviewed changes

lib/css_parser/rule_set.rb Show resolved Hide resolved

grosser reviewed Jan 19, 2021

View reviewed changes

ojab force-pushed the fixup_expanding_shorthands branch 2 times, most recently from bb456c4 to d06c5d3 Compare January 19, 2021 17:55

ojab marked this pull request as ready for review January 19, 2021 18:02

ojab force-pushed the fixup_expanding_shorthands branch 3 times, most recently from e134cc1 to 6cfcca1 Compare January 21, 2021 18:30

grosser reviewed Jan 21, 2021

View reviewed changes

ojab requested a review from grosser January 21, 2021 18:35

grosser reviewed Jan 21, 2021

View reviewed changes

grosser approved these changes Jan 21, 2021

View reviewed changes

ojab force-pushed the fixup_expanding_shorthands branch 3 times, most recently from 0fc2d37 to c2d54df Compare January 21, 2021 18:45

grosser reviewed Jan 21, 2021

View reviewed changes

Preserve rules priority while expanding shorthands

e5421ee

Fixes premailer#111

ojab force-pushed the fixup_expanding_shorthands branch from c2d54df to e5421ee Compare January 21, 2021 19:25

grosser merged commit c6321e6 into premailer:master Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve rules priority while expanding shorthands #119

Preserve rules priority while expanding shorthands #119

ojab commented Jan 19, 2021

grosser Jan 19, 2021 •

edited

Loading

ojab Jan 19, 2021

grosser left a comment

ojab commented Jan 19, 2021

grosser Jan 21, 2021

grosser Jan 21, 2021

ojab Jan 21, 2021

grosser Jan 21, 2021

grosser Jan 21, 2021

ojab Jan 21, 2021

grosser Jan 21, 2021

ojab Jan 21, 2021

grosser left a comment

grosser commented Jan 21, 2021

grosser Jan 21, 2021 •

edited

Loading

ojab Jan 21, 2021

grosser Jan 21, 2021

ojab commented Jan 21, 2021

ojab commented Jan 21, 2021

ojab commented Jan 21, 2021

grosser commented Jan 21, 2021 via email

Preserve rules priority while expanding shorthands #119

Preserve rules priority while expanding shorthands #119

Conversation

ojab commented Jan 19, 2021

grosser Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser left a comment

Choose a reason for hiding this comment

ojab commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser left a comment

Choose a reason for hiding this comment

grosser commented Jan 21, 2021

grosser Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojab commented Jan 21, 2021

ojab commented Jan 21, 2021

ojab commented Jan 21, 2021

grosser commented Jan 21, 2021 via email

grosser Jan 19, 2021 •

edited

Loading

grosser Jan 21, 2021 •

edited

Loading