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

[BUG] Poor performance of no-custom-classname and classnames-order #276

Open
kfarwell opened this issue Sep 19, 2023 · 19 comments · May be fixed by #338
Open

[BUG] Poor performance of no-custom-classname and classnames-order #276

kfarwell opened this issue Sep 19, 2023 · 19 comments · May be fixed by #338
Labels
bug Something isn't working

Comments

@kfarwell
Copy link

Describe the bug
no-custom-classname and classnames-order take a very long time to run relative to other rules.

To Reproduce
Steps to reproduce the behavior:

  1. Clone https://github.com/flirtual/flirtual
  2. Run eslint in apps/frontend/

Expected behavior
Execution time more in line with other rules. These two rules account for over 80% (>50s on a fast machine) of our eslint runtime.

Screenshots

$ TIMING=1 ./node_modules/.bin/eslint src/**/*.ts src/**/*.tsx
Rule                                           | Time (ms) | Relative
:----------------------------------------------|----------:|--------:
tailwindcss/no-custom-classname                | 34219.388 |    55.4%
tailwindcss/classnames-order                   | 17837.675 |    28.9%
prettier/prettier                              |  1704.244 |     2.8%
tailwindcss/no-contradicting-classname         |  1603.051 |     2.6%
tailwindcss/enforces-negative-arbitrary-values |  1420.780 |     2.3%
tailwindcss/enforces-shorthand                 |  1173.931 |     1.9%
import/no-self-import                          |   933.115 |     1.5%
@typescript-eslint/no-floating-promises        |   672.175 |     1.1%
import/export                                  |   305.687 |     0.5%
import/no-named-as-default-member              |   236.529 |     0.4%

Environment (please complete the following information):

  • OS: macOS 13 (M1 Pro)
  • Softwares + version used:
    • eslint 8.49.0

Additional context
Setting cssFiles to [] per #174 makes no-custom-classname almost 40% faster, but these rules still take around 40 seconds to run.

eslint config file or live demo

@kfarwell kfarwell added the bug Something isn't working label Sep 19, 2023
@venables
Copy link

venables commented Sep 22, 2023

We're hitting the same issue (next.js project with a single css file: app/globals.css). Setting the cssFiles array to that single file has helped but the rule is still quite slow.

@ariesclark
Copy link

Added the following to my eslint-config package as a stop-gap measure, but the underlying issue is incredibility unacceptable, given these rules often account for over 50s. If you don't have time to resolve this, could you perhaps point me in the right direction?

// ...
export = declare({
  overrides: [
    {
      files: ["**/*.{js,jsx,ts,tsx}"],
      plugins: ["tailwindcss"],
      extends: ["plugin:tailwindcss/recommended"],
      settings: {
        /**
         * Performance issue with the plugin, somewhat mitigated setting cssFiles to an empty array.
         * @see https://github.com/francoismassart/eslint-plugin-tailwindcss/issues/276
         * @see https://github.com/francoismassart/eslint-plugin-tailwindcss/issues/174
         */
        tailwindcss: {
          cssFiles: [],
        },
      },
      rules: {
        "tailwindcss/no-custom-classname": ["warn", options],
        "tailwindcss/no-contradicting-classname": ["warn", options],
        "tailwindcss/classnames-order": ["warn", options],
      },
    },
  ],
});

@cjpearson
Copy link

It looks like a lot of time is spend in this function

const generateClassnamesListSync = (patterns, refreshRate = 5_000) => {

Perhaps a quick improvement would be to move the fg.sync call inside the if block, and only re-glob if the last result has expired. @francoismassart Do you know if there might be any problems with this approach?

@OussamaFadlaoui
Copy link

+1 on this. Also experiencing extreme slowdown of ESLint within VSCode, especially when format on save.

@kilakewe
Copy link

kilakewe commented Feb 2, 2024

+1 Here too

Rule Time (ms) Relative
tailwindcss/classnames-order 15747.303 61.1%
prettier/prettier 8297.710 32.2%
tailwindcss/no-contradicting-classname 681.510 2.6%
@typescript-eslint/no-unused-vars 227.954 0.9%
tailwindcss/enforces-shorthand 126.805 0.5%
tsdoc/syntax 90.832 0.4%
vue/block-lang 38.474 0.1%
vue/first-attribute-linebreak 36.415 0.1%
vue/no-mutating-props 23.631 0.1%
vue/no-async-in-computed-properties 22.879 0.1%

@Codykilpatrick
Copy link

+1 here

:---------------------------------------|----------:|--------:
tailwindcss/no-contradicting-classname  |  9409.590 |    42.0%
tailwindcss/enforces-shorthand          |  4583.070 |    20.4%
tailwindcss/classnames-order            |  2386.195 |    10.6%
tailwindcss/no-custom-classname         |  2032.716 |     9.1%
@typescript-eslint/no-misused-promises  |   987.254 |     4.4%
import/namespace                        |   387.072 |     1.7%
@typescript-eslint/no-floating-promises |   286.450 |     1.3%
@typescript-eslint/no-unsafe-assignment |   274.832 |     1.2%
import/no-cycle                         |   185.191 |     0.8%
@typescript-eslint/no-unsafe-argument   |    93.103 |     0.4%

@rickvandermey
Copy link

any way of skipping this rule?

Rule                                           |  Time (ms) | Relative
:----------------------------------------------|-----------:|--------:
tailwindcss/no-custom-classname                | 471981.648 |    98.7%
tailwindcss/classnames-order                   |   3747.085 |     0.8%
tailwindcss/no-contradicting-classname         |   1503.595 |     0.3%
tailwindcss/enforces-shorthand                 |    379.227 |     0.1%
@typescript-eslint/no-unused-vars              |    148.400 |     0.0%
simple-import-sort/imports                     |    101.507 |     0.0%
@nx/enforce-module-boundaries                  |     82.911 |     0.0%
tailwindcss/enforces-negative-arbitrary-values |     15.900 |     0.0%
no-global-assign                               |      9.525 |     0.0%
@typescript-eslint/no-empty-function           |      9.173 |     0.0%

@kachkaev
Copy link
Contributor

kachkaev commented Feb 23, 2024

@rickvandermey you can add this to your .eslintrc:

{
  "rules": {
    "tailwindcss/no-custom-classname": "off",
  }
}

Even though the rule is slow, I personally find it very useful and don’t recommend disabling it.

@rickvandermey
Copy link

@rickvandermey you can add this to your .eslintrc:

{
  "rules": {
    "tailwindcss/no-custom-classname": "off",
  }
}

Even though the rule is slow, I personally find it very useful and don’t recommend disabling it.

ended up by removing "extends": ["plugin:tailwindcss/recommended"], and adding tailwindcss as plugin "plugins": ["@nx", "simple-import-sort", "tailwindcss"], this change made it 10x faster. this is good enough for us right now

@XantreDev
Copy link

XantreDev commented Apr 25, 2024

In my case cssFiles: [] boost performance up to 17x times

diff --git a/.eslintrc b/.eslintrc
index ceda0b46..16039373 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -4,7 +4,8 @@
   "ignorePatterns": ["**/node_modules"],
   "settings": {
     "tailwindcss": {
-      "config": "./workspace/packages/config/src/tailwind.config.js"
+      "config": "./workspace/packages/config/src/tailwind.config.js",
+      "cssFiles": []
     }
   },
   "extends": [

Before:

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
tailwindcss/no-custom-classname         | 17177.859 |    51.0%

After:

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
tailwindcss/no-custom-classname         |   920.311 |     5.5%

I think cssFiles detection logic is ineffective, we should investigate it

@XantreDev
Copy link

There are 3 problems.

  1. High refresh rate
  2. Sync access to fs
  3. Usage fast glob even if previous fast glob value is still valid

const generateClassnamesListSync = (patterns, refreshRate = 5_000) => {
const now = new Date().getTime();
const files = fg.sync(patterns, { suppressErrors: true });
const newGlobs = previousGlobsResults.flat().join(',') != files.flat().join(',');
const expired = lastUpdate === null || now - lastUpdate > refreshRate;
if (newGlobs || expired) {
previousGlobsResults = files;
lastUpdate = now;
let detectedClassnames = [];
for (const file of files) {
const data = fs.readFileSync(file, 'utf-8');
const root = postcss.parse(data);
root.walkRules((rule) => {
const regexp = /\.([^\.\,\s\n\:\(\)\[\]\'~\+\>\*\\]*)/gim;
const matches = [...rule.selector.matchAll(regexp)];
const classnames = matches.map((arr) => arr[1]);
detectedClassnames.push(...classnames);
});
detectedClassnames = removeDuplicatesFromArray(detectedClassnames);
}
classnamesFromFiles = detectedClassnames;
}
return classnamesFromFiles;
};

I can provide PR that fixes this behavior

@akodkod
Copy link

akodkod commented Apr 28, 2024

For me tailwindcss/classnames-order works fine, but tailwindcss/no-custom-classname is very slow.

Rule                                    |  Time (ms) | Relative
:---------------------------------------|-----------:|--------:
tailwindcss/no-custom-classname         | 277592.606 |    92.9%
tailwindcss/classnames-order            |  11603.064 |     3.9%
tailwindcss/no-contradicting-classname  |   4678.986 |     1.6%
tailwindcss/enforces-shorthand          |   2526.492 |     0.8%
import/order                            |    457.364 |     0.2%
unused-imports/no-unused-vars           |    252.460 |     0.1%
indent                                  |    217.022 |     0.1%
react/jsx-no-constructed-context-values |    139.501 |     0.0%
react/no-unstable-nested-components     |    113.356 |     0.0%
react/no-direct-mutation-state          |    101.368 |     0.0%

@damianobarbati
Copy link

The tailwindcss/no-custom-classname is taking 2mins on my project as well.

@francoismassart can you suggest an approach to mitigate this? Or is there any plan to do something about it? Thanks!

@XantreDev
Copy link

Main problem of this rule - it tries make sync scan of file system .css files to find class names. We can improve this speed of this rule in couple of times, but it will not be able to be fast until eslint will support async rules

@XantreDev XantreDev linked a pull request May 14, 2024 that will close this issue
10 tasks
@XantreDev
Copy link

After my fix there are still a problem: plugin will traverses file system every 5s. Basically we should not do it at all for cases when we are not inside of LSP. Probably it will add this kind of flag for further improvements

@XantreDev
Copy link

XantreDev commented May 14, 2024

Checked in our react-native project
15s -> 2.4s. 6x performance boost

Before:

✖ 13 problems (0 errors, 13 warnings)

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
tailwindcss/no-custom-classname         | 15016.088 |    47.9%
@typescript-eslint/no-misused-promises  |  8721.884 |    27.8%
import/no-duplicates                    |  1842.079 |     5.9%
@typescript-eslint/no-floating-promises |   828.843 |     2.6%
tailwindcss/classnames-order            |   809.069 |     2.6%
tailwindcss/enforces-shorthand          |   765.008 |     2.4%
tailwindcss/no-contradicting-classname  |   655.226 |     2.1%
react/no-unstable-nested-components     |   446.134 |     1.4%
unused-imports/no-unused-imports        |   260.493 |     0.8%
jest/no-disabled-tests                  |   246.434 |     0.8%

After:

✖ 13 problems (0 errors, 13 warnings)

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
@typescript-eslint/no-misused-promises  |  8704.810 |    46.5%
tailwindcss/no-custom-classname         |  2405.613 |    12.9%
import/no-duplicates                    |  1930.955 |    10.3%
@typescript-eslint/no-floating-promises |   880.225 |     4.7%
tailwindcss/enforces-shorthand          |   792.588 |     4.2%
tailwindcss/no-contradicting-classname  |   630.478 |     3.4%
tailwindcss/classnames-order            |   576.570 |     3.1%
react/no-unstable-nested-components     |   467.783 |     2.5%
unused-imports/no-unused-imports        |   253.199 |     1.4%
jest/no-disabled-tests                  |   244.996 |     1.3%

You can improve speed of plugin with this patch:

diff --git a/lib/util/cssFiles.js b/lib/util/cssFiles.js
index 4a49a33..900aaff 100644
--- a/lib/util/cssFiles.js
+++ b/lib/util/cssFiles.js
@@ -3,12 +3,17 @@
 const fg = require('fast-glob');
 const fs = require('fs');
 const postcss = require('postcss');
-const removeDuplicatesFromArray = require('./removeDuplicatesFromArray');
 
-let previousGlobsResults = [];
+const classRegexp = /\.([^\.\,\s\n\:\(\)\[\]\'~\+\>\*\\]*)/gim;
+
 let lastUpdate = null;
 let classnamesFromFiles = [];
 
+/**
+ * @type {Map<string, number}
+ */
+const prevEditedTimestamp = new Map()
+
 /**
  * Read CSS files and extract classnames
  * @param {Array} patterns Glob patterns to locate files
@@ -17,27 +22,56 @@ let classnamesFromFiles = [];
  */
 const generateClassnamesListSync = (patterns, refreshRate = 5_000) => {
   const now = new Date().getTime();
-  const files = fg.sync(patterns, { suppressErrors: true });
-  const newGlobs = previousGlobsResults.flat().join(',') != files.flat().join(',');
   const expired = lastUpdate === null || now - lastUpdate > refreshRate;
-  if (newGlobs || expired) {
-    previousGlobsResults = files;
-    lastUpdate = now;
-    let detectedClassnames = [];
-    for (const file of files) {
-      const data = fs.readFileSync(file, 'utf-8');
-      const root = postcss.parse(data);
-      root.walkRules((rule) => {
-        const regexp = /\.([^\.\,\s\n\:\(\)\[\]\'~\+\>\*\\]*)/gim;
-        const matches = [...rule.selector.matchAll(regexp)];
-        const classnames = matches.map((arr) => arr[1]);
-        detectedClassnames.push(...classnames);
-      });
-      detectedClassnames = removeDuplicatesFromArray(detectedClassnames);
+
+  if (!expired) {
+    return classnamesFromFiles;
+  }
+  const files = fg.sync(patterns, { suppressErrors: true, stats: true });
+  lastUpdate = now;
+
+  /**
+   * @type {Set<string}
+   */
+  const detectedClassnames = new Set();
+  /**
+   * @type {Set<string>}
+   */
+  const filesSet = new Set();
+  for (const { path: file, stats } of files) {
+    filesSet.add(file);
+    if (!stats) {} 
+    // file is not changed -> we do need to do extra work
+    else if (prevEditedTimestamp.get(file) === stats.mtimeMs) {
+      continue;
+    } else {
+      prevEditedTimestamp.set(file, stats.mtimeMs);
     }
-    classnamesFromFiles = detectedClassnames;
+    const data = fs.readFileSync(file, 'utf-8');
+    const root = postcss.parse(data);
+    root.walkRules((rule) => {
+      for (const match of rule.selector.matchAll(classRegexp)) {
+        detectedClassnames.add(match[1]);
+      }
+    });
   }
-  return classnamesFromFiles;
+  // avoiding memory leak
+  {
+    /**
+     * @type {string[]}
+     */
+    const keysToDelete = []
+    for (const cachedFilePath of prevEditedTimestamp.keys()) {
+      if (!filesSet.has(cachedFilePath)) {
+        keysToDelete.push(cachedFilePath);
+      }
+    }
+    for (const key of keysToDelete) {
+      prevEditedTimestamp.delete(key);
+    }
+ }
+  classnamesFromFiles = [...detectedClassnames];
+  return classnamesFromFiles
 };
 
 module.exports = generateClassnamesListSync;

@ariesclark
Copy link

ariesclark commented May 14, 2024

@XantreDev Can you turn this into a pull request? I'd like to see this merged.
EDIT: Oop, just saw you already did, well done!

@XantreDev
Copy link

Btw. Say how it performs in your cases?

@ariesclark
Copy link

Btw. Say how it performs in your cases?

To be honest, I don't need much improvement in the case where cssFiles: []. I think the performance issues are something more fundamental.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.