chore(postcss-minify-gradients): replace is-color-stop with own code #1181

ludofischer · 2021-08-15T13:33:11Z

Get rid of 7 dependencies in a cssnano install.
is-color-stop duplicates functionality available from other dependencies. It copies
unit parsing code directly from postcss-value-parser.
cssnano has color detection logic is already available in colord.

Bringing the code inside might also help #650

ludofischer · 2021-08-15T13:35:53Z

@sigveio did you make any progress on #650? This change might be relevant to that issue.

codecov-commenter · 2021-08-15T13:36:24Z

Codecov Report

Merging #1181 (2f82285) into master (04bd16e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
+ Coverage   96.55%   96.56%   +0.01%     
==========================================
  Files         116      117       +1     
  Lines        3627     3644      +17     
  Branches     1065     1071       +6     
==========================================
+ Hits         3502     3519      +17     
  Misses        117      117              
  Partials        8        8

Impacted Files	Coverage Δ
packages/postcss-minify-gradients/src/index.js	`98.01% <ø> (ø)`
...ckages/postcss-minify-gradients/src/isColorStop.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 04bd16e...2f82285. Read the comment docs.

sigveio · 2021-08-15T15:30:23Z

@sigveio did you make any progress on #650? This change might be relevant to that issue.

Yes indeed! I made good progress on building up a thorough and up to date suite of functional tests to support all the gradient types (repeating and non-repeating) with both one and two position syntax... and how they can be shortened according to the various 'fixup' rules etc. Which is quite a lot of work - but allowing me to build a good understanding of the relatively large and complex specs around this. In preparation for diving in with some heavy handed refactoring coupled with unit tests!

I stalled a bit due to what I'll summarise as #life (😅 )... and also in between things trying to give a hand with triage and such in the Jest issue tracker while the maintainer was on vacation. But I aim to shift more focus back towards cssnano this coming week, and in addition to continuing the gradient-work also follow up with a PR on this. =)

Nuking is-color-stop seems like a good idea. I had a todo to look at it as well, because when experimenting with a rewrite for a simplified and more efficient postcss-merge-longhand I noticed that the original use yet another library called css-color-names for parsing/validating the border width-style-color properties. Which, in that case, even seemed fully redundant... as the logic I had for detecting width and style (using postcss-value-parser and a tiny bit of extra code) seemed robust enough that I could reliably detect color merely by exclusion. (Although that'll definitely need to be tested more thoroughly to say for sure)

I'm guessing these are not the only places in the codebase where there are these kind of redundancies... for example duplicating the functionality of postcss-value-parser like you mention.

ludofischer · 2021-08-15T16:49:54Z

Nuking is-color-stop seems like a good idea.

Yes, it does not even detect a color stop according to the current spec, since it expects only two values.
Also, I've found the current code uses is-color-stop only for minifying the -webkit special syntax

cssnano/packages/postcss-minify-gradients/src/index.js

Line 132 in 04bd16e

lowerCasedValue === '-webkit-radial-gradient' ||

So it does not affect #650 but replacing it should have limited impact.

Which is quite a lot of work - but allowing me to build a good understanding of the relatively large and complex specs around this.

Can these tests be added to the repository? Working on these optimizations is indeed a lot of work, I think you have to undertake it more for fun than for anything else. From the various tests I've found on the web, switching to brotli compression from gzip gives a size decrease that's much bigger than the difference between the output of any two css minifiers. So a generic text compression algorithm beats techniques that use CSS domain knowledge.

ludofischer · 2021-08-15T16:54:36Z

I noticed that the original use yet another library called css-color-names for parsing/validating the border width-style-color properties.

css-color-names only exports a list of all color names with matching hex value, so that's already included in colord. The issue is more with special names like currentcolor.

sigveio

I suggested a couple of small tweaks just to bring the code style more in line with our code base. I'll leave the rest be, since the main benefit here is to get rid of the dependency - and spending time on it beyond that adds little value at this point, considering (as you pointed out) the limited impact. :)

Since the code is pretty much copied straight out of is-color-stop should that be mentioned somewhere to satisfy licensing/copyright? I'm not entirely sure what applies here, so just asking to be on the safe side

sigveio · 2021-08-15T22:29:20Z

packages/postcss-minify-gradients/src/isColorStop.js

+];
+
+function isCSSLengthUnit(input) {
+  return lengthArray.indexOf(input.toUpperCase()) >= 0;


Suggested change

return lengthArray.indexOf(input.toUpperCase()) >= 0;

return lengthArray.includes(input.toLowerCase());

sigveio · 2021-08-15T22:31:30Z

packages/postcss-minify-gradients/src/isColorStop.js

+const lengthArray = [
+  'PX',
+  'IN',
+  'CM',
+  'MM',
+  'EM',
+  'REM',
+  'POINTS',
+  'PC',
+  'EX',
+  'CH',
+  'VW',
+  'VH',
+  'VMIN',
+  'VMAX',
+  '%',
+];


Suggested change

const lengthArray = [

'PX',

'IN',

'CM',

'MM',

'EM',

'REM',

'POINTS',

'PC',

'EX',

'CH',

'VW',

'VH',

'VMIN',

'VMAX',

'%',

];

const lengthArray = [

'px',

'in',

'cm',

'mm',

'em',

'rem',

'points',

'pc',

'ex',

'ch',

'vw',

'vh',

'vmin',

'vmax',

'%',

];

I've left it uppercase because it looks like an unintuitive choice so I thought there might be a reason behind it?

looks like an unintuitive choice

Indeed

might be a reason behind it?

Hmm.. well... we are not expecting any exotic characters - and even if we were... then forcing case in either direction wouldn't be a good way of doing case insensitive comparison anyway. So it strikes me as merely being down to personal preference / coding style... as in... the author thought it looked prettier? I at least can't think of anything else that should differ between them in the context of comparison; can you?

We use lower case comparison everywhere else, as far as I know successfully, that's why I suggested the change. I'm okay with leaving it as-is if you feel that is safer though

sigveio · 2021-08-17T14:15:58Z

Can these tests be added to the repository?

Yep; my plan is to add the tests as the basis for a draft PR. Then we can use that for tracking further progress. 👍

The rest I followed up in #1173

Get rid of 7 dependencies in a cssnano install. is-color-stop duplicates functionality available from other dependencies. It copies unit parsing code directly from postcss-value-parser. cssnano has color detection logic is already available in colord.

ludofischer

I have applied some of the suggestions and added a link to the original code.

ludofischer · 2021-08-17T15:22:29Z

packages/postcss-minify-gradients/src/isColorStop.js

+const lengthArray = [
+  'PX',
+  'IN',
+  'CM',
+  'MM',
+  'EM',
+  'REM',
+  'POINTS',
+  'PC',
+  'EX',
+  'CH',
+  'VW',
+  'VH',
+  'VMIN',
+  'VMAX',
+  '%',
+];


I've left it uppercase because it looks like an unintuitive choice so I thought there might be a reason behind it?

ludofischer · 2021-08-17T15:24:49Z

packages/postcss-minify-gradients/src/isColorStop.js

+
+extend([namesPlugin]);
+
+/* Code derived from https://github.com/pigcan/is-color-stop */


Added a link to the orignal code, I don't think the MIT license requires it but I agree it is nicer to have proper attribution

sigveio · 2021-08-17T16:00:59Z

packages/postcss-minify-gradients/src/isColorStop.js

+const lengthArray = [
+  'PX',
+  'IN',
+  'CM',
+  'MM',
+  'EM',
+  'REM',
+  'POINTS',
+  'PC',
+  'EX',
+  'CH',
+  'VW',
+  'VH',
+  'VMIN',
+  'VMAX',
+  '%',
+];


looks like an unintuitive choice

Indeed

might be a reason behind it?

Hmm.. well... we are not expecting any exotic characters - and even if we were... then forcing case in either direction wouldn't be a good way of doing case insensitive comparison anyway. So it strikes me as merely being down to personal preference / coding style... as in... the author thought it looked prettier? I at least can't think of anything else that should differ between them in the context of comparison; can you?

We use lower case comparison everywhere else, as far as I know successfully, that's why I suggested the change. I'm okay with leaving it as-is if you feel that is safer though

sigveio · 2021-08-17T16:06:07Z

packages/postcss-minify-gradients/src/isColorStop.js

+
+extend([namesPlugin]);
+
+/* Code derived from https://github.com/pigcan/is-color-stop */


ludofischer force-pushed the is-color-stop branch from 91f4daa to 52d865c Compare August 15, 2021 13:34

sigveio previously approved these changes Aug 17, 2021

View reviewed changes

ludofischer dismissed sigveio’s stale review via 2f82285 August 17, 2021 15:15

ludofischer force-pushed the is-color-stop branch from 52d865c to 2f82285 Compare August 17, 2021 15:15

ludofischer commented Aug 17, 2021

View reviewed changes

sigveio approved these changes Aug 17, 2021

View reviewed changes

ludofischer merged commit 50eb53e into master Aug 17, 2021

ludofischer deleted the is-color-stop branch August 17, 2021 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(postcss-minify-gradients): replace is-color-stop with own code #1181

chore(postcss-minify-gradients): replace is-color-stop with own code #1181

ludofischer commented Aug 15, 2021 •

edited

ludofischer commented Aug 15, 2021

codecov-commenter commented Aug 15, 2021 •

edited

sigveio commented Aug 15, 2021 •

edited

ludofischer commented Aug 15, 2021 •

edited

ludofischer commented Aug 15, 2021 •

edited

sigveio left a comment

sigveio Aug 15, 2021

sigveio Aug 15, 2021

ludofischer Aug 17, 2021

sigveio Aug 17, 2021

sigveio commented Aug 17, 2021

ludofischer left a comment

ludofischer Aug 17, 2021

ludofischer Aug 17, 2021

sigveio Aug 17, 2021

sigveio Aug 17, 2021

sigveio Aug 17, 2021

	return lengthArray.indexOf(input.toUpperCase()) >= 0;
	return lengthArray.includes(input.toLowerCase());


		extend([namesPlugin]);

		/* Code derived from https://github.com/pigcan/is-color-stop */

chore(postcss-minify-gradients): replace is-color-stop with own code #1181

chore(postcss-minify-gradients): replace is-color-stop with own code #1181

Conversation

ludofischer commented Aug 15, 2021 • edited

ludofischer commented Aug 15, 2021

codecov-commenter commented Aug 15, 2021 • edited

Codecov Report

sigveio commented Aug 15, 2021 • edited

ludofischer commented Aug 15, 2021 • edited

ludofischer commented Aug 15, 2021 • edited

sigveio left a 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

sigveio commented Aug 17, 2021

ludofischer left a 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

ludofischer commented Aug 15, 2021 •

edited

codecov-commenter commented Aug 15, 2021 •

edited

sigveio commented Aug 15, 2021 •

edited

ludofischer commented Aug 15, 2021 •

edited

ludofischer commented Aug 15, 2021 •

edited