-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
increase perf && smaller size #23
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 22 22
=========================================
Hits 22 22
Continue to review full report at Codecov.
|
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.
Thank you :) A couple comments, although I'd also recommend isolating the two clsx
candidates and doing a pass with prev first and then another with this PR first.
@@ -2,14 +2,16 @@ function toVal(mix) { | |||
var k, y, str=''; | |||
|
|||
if (typeof mix === 'string' || typeof mix === 'number') { | |||
str += mix; | |||
} else if (typeof mix === 'object') { | |||
return mix; |
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 is a regression. Number type is returned.
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.
Added tests for this case. I found this is not regression :-) Because https://github.com/lukeed/clsx/blob/master/src/index.js#L35 converts toVal
return value to string.
for (k=0; k < mix.length; k++) { | ||
if (mix[k]) { | ||
if (y = toVal(mix[k])) { | ||
for (y=0; y < mix.length; y++) { |
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.
Why did these vars get swapped?
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.
Codebase contains
str && (str += ' ');
str += k;
thrice. I try to create more identical pieces of code for better work for the "deflate" algorithm. This trick saves 2 bytes of gzip size. I don't rename the third piece(https://github.com/lukeed/clsx/blob/master/src/index.js#L34) because the total size not changed.
I run the bench for current master and my PR separately and found some perf regression Master:
PR:
Is this tradeoff applicable? |
A variant I did: function toVal(mix) {
var k, y, str='';
if (typeof mix === 'string' || typeof mix === 'number') {
return mix;
}
if (typeof mix === 'object') {
if (Array.isArray(mix)) {
for (y=0; y < mix.length; y++) {
if (mix[y]) {
if (k = toVal(mix[y])) {
str && (str += ' ');
str += k;
}
}
}
return str;
} else {
for (k in mix) {
if (mix[k]) {
str && (str += ' ');
str += k;
}
}
return str;
}
}
}
export function clsx() {
var i=0, tmp, x, str='';
while (i < arguments.length) {
if (tmp = arguments[i++]) {
if (x = toVal(tmp)) {
str && (str += ' ');
str += x
}
}
}
return str;
}
export default clsx; Moving |
i've small little suggestion tho... why not var k, y, str='', a = typeof mix;
if (a === 'string' || a === 'number') {}
if (a === 'object') {} |
@AzrizHaziq Believe or not, this does not make the Gzipped version smaller |
Thanks for this PR & your patience. I know it's been a while & there are now conflicts, but there were 2 points raised & I'll address them here rather than continue the inline conversations:
Thanks again! |
Size before:
Size after:
Perf on my Dell xps 13 9360 Core-i78550U: