Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(postcss-colormin): switch to colord and solve multiple issues #1107

Merged
merged 1 commit into from May 19, 2021

Conversation

ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented May 18, 2021

  • ensure that the replacement color values are shorter than the original. Previously, the plugin returned the converted value without checking whether it was shorter than the original, we now ensure we always return the shortest color value
  • rework the colomin cache so use only one cache as in refactor(postcss-colormin): Improve caching. #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS property|value key and one that cached the single colors.
  • change the underlying library to colord.
    Using colord comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an open issue without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here).
    On the minus side, the colord parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code.
  • get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check.

fix(postcss-colormin): do not replace original value if shorter than conversion

fix #1042

feat(postcss-colormin): change to colord

perf(postcss-colormin): improve caching

Fix #771

refactor(postcss-colormin): drop the generate script

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2021

Codecov Report

Merging #1107 (1d36ad1) into master (ea1ff0b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
- Coverage   96.37%   96.37%   -0.01%     
==========================================
  Files         115      115              
  Lines        3589     3582       -7     
  Branches     1059     1054       -5     
==========================================
- Hits         3459     3452       -7     
  Misses        121      121              
  Partials        9        9              
Impacted Files Coverage Δ
packages/postcss-colormin/src/colours.js 100.00% <100.00%> (ø)
packages/postcss-colormin/src/index.js 98.11% <100.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea1ff0b...1d36ad1. Read the comment docs.

* ensure that the replacement color values are shorter than the original. Previously, the plugin returned the converted value without checking whether it was shorter than the original, we now ensure we always return the shortest color value
* rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors.
* change the underlying library to [colord](https://github.com/omgovich/colord).
Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here).
On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code.
* get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check.

fix(postcss-colormin): do not replace original value with longer one

fix #1042

feat(postcss-colormin): switch to colord

* round color values fix #819
* reduce dependencies

perf(postcss-colormin): improve caching

Fix #771

refactor(postcss-colormin): drop the generate script
@alexander-akait
Copy link
Member

Great job!

@ludofischer
Copy link
Collaborator Author

Performance might have improved as well. I've run a quick test with benchmark.js and I get

Branch Mean Standard deviation
master 64ms 27ms
colord 39ms 18ms

@ludofischer ludofischer merged commit a7f0be4 into master May 19, 2021
@ludofischer ludofischer deleted the update-colormin branch May 19, 2021 16:21
@omgovich
Copy link
Contributor

Hey guys! Thanks for choosing colord as a color manipulation library.
I'll check the code later and think about how to improve your DX.

If you'll face any issues, contact me here or via issues in the colord's repo.
I'm always glad to help.

@omgovich
Copy link
Contributor

BTW guys, are 4 and 8 digit color formats are supported by the cssnano?
Performing conversion to the shorthand alpha-hex string might make the color output way lighter.

@alexander-akait
Copy link
Member

BTW guys, are 4 and 8 digit color formats are supported by the cssnano?

https://github.com/cssnano/cssnano/pull/1107/files#diff-7f46afa35b95730b615fe24cb6667201f6471b587abd70c48841fe78c8c047fdR136

@ludofischer
Copy link
Collaborator Author

BTW guys, are 4 and 8 digit color formats are supported by the cssnano?
Performing conversion to the shorthand alpha-hex string might make the color output way lighter.

It tries to convert it to a shorthand hex only if alpha is 1

Is that enough?

@omgovich
Copy link
Contributor

omgovich commented May 19, 2021

By using 4 digit hexadecimal "#RGBA" format we could minify some rgba values.

Here is an example:
rgba(0, 0, 0, 0.4) -> #0006 (same as #00000066)
#0006 is shorter than any other option.

@omgovich
Copy link
Contributor

omgovich commented May 19, 2021

The only thing that bothers me is the browser support. As far as I know, #RGBA format is part of CSS Color Level 4 (not 100% sure), and I don't know a good way to check how many browsers support that at the moment.

@alexander-akait
Copy link
Member

We can implement this as feature and check support using browserslist, feel free to open an issue

@omgovich
Copy link
Contributor

If you don't mind I'll create a pull request and ask for your help with configuring the browserlist there.

@omgovich
Copy link
Contributor

omgovich commented May 19, 2021

Created #1109. Take a look please when you'll have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants