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

no-custom-classname: perf optimization #338

Conversation

XantreDev
Copy link
Contributor

@XantreDev XantreDev commented May 14, 2024

no-custom-classname: perf optimization

Description

Reduced both sync IO time and reduced js performance overhead

Fixes #276

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Runned repository tests

Test Configuration:

  • OS + version: Windows 11
  • NPM version: 10.5.0
  • Node version: 20.12.0

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@XantreDev
Copy link
Contributor Author

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%

@francoismassart
Copy link
Owner

Hi @XantreDev,
thank you for contributing to the project,
I just published [email protected] which contains your fix.

It should be published soon (finally 😅)

npm i [email protected] -D

@XantreDev XantreDev marked this pull request as ready for review May 23, 2024 20:22
@XantreDev
Copy link
Contributor Author

I think I can be improved even further but I need to separate LSP from regular eslint. In case of client we don't need to make fs call ones

@francoismassart francoismassart merged commit d865d93 into francoismassart:master Jun 5, 2024
if (!stats) {}
// file is not changed -> we do need to do extra work
else if (prevEditedTimestamp.get(file) === stats.mtimeMs) {
continue;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XantreDev @francoismassart This seems to have introduced a regression if the linter takes longer than cssFilesRefreshRate to complete.

When iterating through the css files to parse for class names, if the file is unchanged we skip it, meaning that none of the classnames in any of the CSS files are honored.

skipping file .../globals.css
{ classnamesFromFiles: [] }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Detected class names need to be also cached

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snyamathi Should it resolve the problem? #341

}
}
classnamesFromFiles = [...detectedClassnames];
return classnamesFromFiles

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ends up being an empty array since we hit the file is not changed block for every file.

@RaenonX
Copy link

RaenonX commented Jun 6, 2024

Hi, sorry if this is not the right place to ask/report, but after this update, my main code repo starts reporting false-negative of tailwindcss/no-custom-classname with the class names defined in the custom css of the section @layer utilies. I can confirm that it worked fine with 3.17.0.

I did try create a reproduction repo, but unfortunately I am unable to reproduce the bug. Since my main repo is quite large, it will be very hard to nail down what the other dependency might be causing the issue :(

The main repo is using next.js, and the repro repo was initialized using yarn create-next-app. The main repo is currently private, but let me know what else might be needed to help investigating.

I took a look in the code of this, which I assume is the only change for 3.17.1 if I am not mistaken, but I couldn't found something suspicious that causes this disconnection between globals.css of the repro repo from yarn create-next-app and this.

@francoismassart
Copy link
Owner

HI, for now,
I'll revert back to 3.17.0 and publish as a new patch version => 3.17.2

@RaenonX
Copy link

RaenonX commented Jun 6, 2024

HI, for now, I'll revert back to 3.17.0 and publish as a new patch version => 3.17.2

Thanks! Appreciate your fast response.

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

Successfully merging this pull request may close these issues.

[BUG] Poor performance of no-custom-classname and classnames-order
4 participants