mangling of CSS variables (custom properties)?

[tameraydin]

Hi, thanks for this project!

Although the focus of plugin's is clearly class-names only, it would be nice if it also mangles CSS variables (custom properties) optionally.

:root {
  --l-long-variable-names: 1px;
}
... {
  padding: var(--l-long-variable-names)
}

->

:root {
  --a: 1px;
}
... {
  padding: var(--a)
}

Practically below change will enable it:

-    classnameRegex = new RegExp(`\\\.(${opts.classNameRegExp})`, 'g');
+    classnameRegex = new RegExp(`\(?:\.?|--)(${opts.classNameRegExp})`, 'g');

If you are willing to introduce this, happy to create a PR with tests.

[sndyuk]

@tameraydin Thank you for the suggestion! It sounds good. But I'm not sure the proposed regex works fine since the original regex has \\\. and the new one doesn't.

[tameraydin]

@sndyuk you might be right, however while creating the PR I realized that none of your test cases will cover that branch (L22) but goes through L24 😬 (I guess this is because of the webpack test environment, due to style-loader perhaps; all of the files being processed are counting as JS)

Created a new spec using the existing setup, so still no coverage for CSS files - I don't have time for it, but in my private repo (where I use exact same diff ^ that I suggested) things seem to be working fine 😅
I also tested other existing cases having the new option enabled - they all pass.

[tameraydin]

Also FYI; npm warned that the lock-file needed to be updated, so I did that - and also fixed found vulnerabilities from npm audit, and deduped dependencies. (You can see each in separate commits)

npm WARN old lockfile
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile
npm WARN deprecated stringify-package@1.0.1: This module is not used anymore, and has been replaced by @npmcli/package-json

...

6 vulnerabilities (1 moderate, 3 high, 2 critical)
...

->

removed 4 packages, changed 6 packages, and audited 307 packages in 936ms

36 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

[sndyuk]

@tameraydin Thank you for the P/R! Checked the new regex deeply and there is something I don't understand. Please review the diff I made below and fix the code if your code works well with this.

\(?:\.?|--)

Is this the first letter \ required?

\(?:\.?|--)

The first dot(.) is necessary. so ``.?should be.`(without `?`) ? Otherwise `.cl-test` and `url("cl-test.jpg")` are both matched.

(?:\\\.|--)

Added one more \ here. but I'm actually not sure what is the difference between (?:\\\.|--) and (?:\.|--). There will be no difference, I guess. So let's make it be same as original one.

diff --git a/lib/optimizer.js b/lib/optimizer.js
index 9111ad1..a13579d 100644
--- a/lib/optimizer.js
+++ b/lib/optimizer.js
@@ -20,11 +20,11 @@ const optimize = (compiler, [file, originalSource], compilation, opts, classGene
   let classnameRegex;
   if (file.match(/.+\.css.*$/)) {
     classnameRegex = opts.mangleCssVariables
-      ? new RegExp(`\(?:\.?|--)(${opts.classNameRegExp})`, 'g')
+      ? new RegExp(`(?:\\\.|--)(${opts.classNameRegExp})`, 'g')
       : new RegExp(`\\\.(${opts.classNameRegExp})`, 'g');
   } else if (file.match(/.+\.js.*$/) || file.match(/.+\.html.*$/)) {
     classnameRegex = opts.mangleCssVariables
-      ? new RegExp(`\(?:["'\`.\\\s]|--)(${opts.classNameRegExp})`, 'g')
+      ? new RegExp(`(?:["'\`.\\\s]|--)(${opts.classNameRegExp})`, 'g')
       : new RegExp(`["'\`.\\\s](${opts.classNameRegExp})`, 'g');
   }
   if (!classnameRegex) {

[tameraydin]

Thanks for the review and pointing out that case - you are right! (Again, there needs to be some refactoring on the test setup in order to cover such CSS specific scenarios - currently they are all processed as JS content, and properties such as url or content are "expectedly" being mangled 🙃)

1 & 3 makes sense. Applied your suggestions (amended) - all tests are passing (with mangleCssVariables on and off).

[sndyuk]

@tameraydin I've just released the new version (5.1.0).

Again, there needs to be some refactoring on the test setup in order to cover such CSS specific scenarios - currently they are all processed as JS content, and properties such as url or content are "expectedly" being mangled 🙃

True... I'll create the issue!