-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
feat: add one-letter-css plugin #1181
Conversation
Codecov ReportBase: 96.81% // Head: 98.83% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1181 +/- ##
==========================================
+ Coverage 96.81% 98.83% +2.01%
==========================================
Files 12 12
Lines 1131 773 -358
Branches 411 236 -175
==========================================
- Hits 1095 764 -331
+ Misses 27 9 -18
+ Partials 9 0 -9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/plugins/hash-len-suggest.js
Outdated
|
||
console.log(); | ||
console.log('Suggest Minify Plugin'); | ||
console.log('Matched length (len: number):', matchLen); |
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.
We have logger
for webpack, avoid using console.log
https://webpack.js.org/api/logging/
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.
fix
src/plugins/hash-len-suggest.js
Outdated
}); | ||
|
||
return matchLen; | ||
} |
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.
We should use weak cache here, otherwise is was memory leak in watch mode
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.
need help with it
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.
@alexander-akait help plz
src/plugins/hash-len-suggest.js
Outdated
} | ||
} | ||
|
||
module.exports = HashLenSuggest; |
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 think we can avoid using plugin, we have the identifier of module, so we can generate names based on this
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.
we use file path to make long cash. and make shortest hash as we can (but need check for collisions)
this.a = 'a'.charCodeAt(0); | ||
this.A = 'A'.charCodeAt(0); | ||
// file hashes cache | ||
this.files = {}; |
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.
Here potential memory leak in watch mode, if will have a lot of renamed files, the object will grow
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.
fixed at last call - getStat()
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.
Maybe we can do it using pattern like localIdentName:
[single-letter]` and keep login there, it will allow more flexibility
# Conflicts: # src/plugins/index.js
Is this dead? |
This PR contains a:
resolve issue #1028
For efficient gzip/br compression, plugin combine css hash via one symbol name,
as a classname position at file, with filepath hash:base64:8, to have strong sequences.