fix: units being removed in math functions

[wongjn]

When using min(), max() or clamp(), 0 values get their units stripped, making the value invalid. This pull request preserves any units within these CSS functions.

[anikethsaha]

Please fix the CI, else LGTM

[alexander-akait]

We need to look on CSS spec for all possible values

[wongjn]

min(), max() and clamp() were introduced in CSS Values Module Level 4, although I don't know if there are other CSS functions in other modules that don't accept unitless 0.

[alexander-akait]

Hm, what about calc?

[wongjn]

I've had a look and units on 0 values are preserved already in calc — maybe these should be moved to the same place? This would mean the same value conversions that happen in calc would happen in these Level 4 functions.

[alexander-akait]

It is strange, I think logic for calc should be here too

[wongjn]

I think that would introduce a lot of regressions in the current transformations in calc.

How about something like this to bring the functions together:

         if (
           lowerCasedValue === 'calc' ||
+          lowerCasedValue === 'min' ||
+          lowerCasedValue === 'max' ||
+          lowerCasedValue === 'clamp' ||
           lowerCasedValue === 'hsl' ||
           lowerCasedValue === 'hsla'
         ) {
           walk(node.nodes, (n) => {
             if (n.type === 'word') {
               parseWord(n, opts, true);
             }
           });
           return false;
         }
-        if (
-          lowerCasedValue === 'url' ||
-          lowerCasedValue === 'min' ||
-          lowerCasedValue === 'max' ||
-          lowerCasedValue === 'clamp'
-        ) {
+        if (lowerCasedValue === 'url') {
           return false;
         }
       }

[alexander-akait]

Yes, let's do it

[wongjn]

New pull request?

[alexander-akait]

Yep

[wongjn]

Most of the failing CI seems to be disconnected from the changes I made, so not sure if it is in the scope of this pull request to fix them?

[codecov-io]

Codecov Report

Merging #894 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #894   +/-   ##
=======================================
  Coverage   97.27%   97.28%           
=======================================
  Files         118      118           
  Lines        3455     3459    +4     
  Branches     1037     1041    +4     
=======================================
+ Hits         3361     3365    +4     
  Misses         86       86           
  Partials        8        8           

Impacted Files	Coverage Δ
packages/postcss-convert-values/src/index.js	100.00% <100.00%> (ø)

---

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 09bdf86...80f50ec. Read the comment docs.

[anikethsaha]

jest timeout has to be increased .. I guess 30s might be enough.

[wongjn]

Would it be appropriate to do that in this PR?

[alexander-akait]

Yes

[anikethsaha]

LGTM
I couldn't find any other function, let me know if there is any !

[alexander-akait]

Thanks!

[helins]

I believe this is an important fix. I've just spend 3 hours trying to figure this one out in a project. It is not a well-known fact fact a unitless 0's mess with function such as max or clamp and you wouldn't expect cssnano to misbehave with something like that.

Meanwhile, a hack would be to use a very low value (eg. 0.01px) or just "0px" via a custom properties. But those are just hacks.

[srsly4]

Would it be possible to backport this fix into any 4.x.x release available via npm?

At the moment there is no possibility to fix this while working on a react-script project without upgrading the whole webpack subsystem and even yarn resolutions can't force commit for this exact subpackage.