-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
no-custom-classname: perf optimization #338
Conversation
Checked in our react-native project Before:
After:
|
Hi @XantreDev, It should be published soon (finally 😅)
|
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 |
if (!stats) {} | ||
// file is not changed -> we do need to do extra work | ||
else if (prevEditedTimestamp.get(file) === stats.mtimeMs) { | ||
continue; |
There was a problem hiding this comment.
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: [] }
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 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 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 |
HI, for now, |
Thanks! Appreciate your fast response. |
@RaenonX & @snyamathi
Changes made: #346 Thank you for your feedbacks |
@francoismassart Just tested, seems fine on my end. Ran |
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.
How Has This Been Tested?
Test Configuration:
Checklist: