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

Refactor/speed improvement 2 #93

Closed

Conversation

beaunus
Copy link

@beaunus beaunus commented Jan 11, 2022

Speed Improvements with cssFiles

This code change is best observed without whitespace. https://github.com/francoismassart/eslint-plugin-tailwindcss/pull/93/files?diff=split&w=1

Description

This is an alternative approach to #92.

After trying v2.0.0-beta I still observed that the execution time was too long. My suspicion is that maybe we don't need to call fg.sync if the previous fg.sync results have not expired.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • OS + version: e.g. macOS Mojave
  • NPM version: ...
  • Node version: ...

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
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

The call to `fg.sync` is expensive.
Perhaps we don't need to call it within
the interval where the old values are "fresh".
@beaunus beaunus marked this pull request as ready for review January 11, 2022 05:56
@beaunus beaunus mentioned this pull request Jan 11, 2022
10 tasks
@francoismassart
Copy link
Owner

francoismassart commented Jan 11, 2022

@beaunus I tested #93
while it is working when running the lint process in a npm script
I get the same issue than with the first version of your PR when used through the eslint extension of VSCode IDE:
after the first lint process updating the css file won't be reflected in the plugin and the lint results are inconsistent.

What value(s) did you use in v2 beta for the refreshrate ?
You could set it really high like 60_000 or even 600_000...

Another solution would be to:

  • generate the list of white listed classnames in a file (via a npm script like prelint)
  • load generated list in your eslintrc for the whitelist option of the plugin
  • provide an empty array for the cssFiles

To be honest I have almost no extra css file, my current project has less than 10 tiny *.module.css files because 99% is regular tailwind classes.

@beaunus
Copy link
Author

beaunus commented Jan 11, 2022

😮 Wow! Thank you @francoismassart for the conversation here.

I followed your advice and discovered this:

  • The pg.sync call was certainly the bottleneck.
  • The default values for cssFiles is ["**/*.css", "!**/node_modules"] (as documented 🙏 ).
    • Because of ☝️ , the pg.sync call was looking through every .git folder and every NextJS out folder, etc.

If I explicitly set the cssFiles option to what it needs to be (['styles/**/*.css', 'src/**/*.css']), the system is super fast.

Thus:

  • My situation is resolved. Thank you. 🙏
  • I think you should consider this commit, since it seems to be a bug.

Thank you again for your support. Please let me know if I should close this PR or take any further action to wrap this up. 🤝

@francoismassart
Copy link
Owner

@beaunus do you have suggestions on potential additional default values for cssFiles?

=> ["**/*.css", "!**/node_modules", "!.*/"]

!.*/ would negate .git/ and other hidden folders.

I'll check for the potential bug too.

@beaunus
Copy link
Author

beaunus commented Jan 11, 2022

I like your suggestion about !.*/. You may also consider ignoring typical artifact folders, like /out or /dist or /build.

Perhaps it might be useful to include a sentence or two about how slow performance might be caused by "loose" globs. I can help draft a proposed PR if you think it would be helpful.

I work on GMT+9. I can help with this tomorrow, if you'd like.

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.

2 participants