Closed EdwardDrapkin closed 6 years ago
Merging #26 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #26 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 13 19 +6
Branches 7 10 +3
=====================================
+ Hits 13 19 +6
Impacted Files | Coverage Δ | |
---|---|---|
src/index.js | 100% <100%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 6baabf3...538115b. Read the comment docs.
@EdwardDrapkin The length > 1 check makes this particular case fail:
cc(["ok"])
...which should be infrequent, but still surprising. In other words, cc(["ok"]) === "ok"
, but is not in this branch. 😢
Alright, I updated the check to be length > 0 (whoops lol) and I also added a test case for it.
I also optimized the for loop based on this blog post: https://medium.com/@AamuLumi/voyage-to-the-most-efficient-loop-in-nodejs-and-a-bit-js-5961d4524c2e
I can't notice any improvement, but I'm testing on a laptop so it's probably being lost in statistical noise.
I also changed the semantics a little bit in that passing 0 will result in '0' instead of '' because I figured if someone is passing a number, they are doing some kind of weird class replacement based on array indices in a babel plugin or something and they would want their 0 to be preserved.
Tests added for that too brings branch coverage up to 100%.
A minor beneficial side effect is that you can now pass nested arrays (added a test case): [foo, [bar, [foo-bar]], bar-foo, [[[[test]]]]]
winds up becoming "foo bar foo-bar bar-foo test".
@EdwardDrapkin Ah, the numbers as CSS classes debate. 😄
Technically a class that starts with a number is not valid CSS. But even if it was (or is going to be in some future spec), a number on its own is semantically meaningless without some sort of prefix, so I'd prefer to optimize for size in this instance.
I can't notice any improvement, but I'm testing on a laptop so it's probably being lost in statistical noise.
Haha bummer. I was quite surprised when I saw that 95,576,430 ops/sec at first.
@EdwardDrapkin A minor beneficial side effect is that you can now pass nested arrays (added a test case):
[foo, [bar, [foo-bar]], bar-foo, [[[[test]]]]]
winds up becoming "foo bar foo-bar bar-foo test".
This can be done with our current version of classcat. I added a test for that in 85af3f52aa6bab38a4741922fe2e57a8f3fa6faa.
also changed the semantics a little bit in that passing 0 will result in '0' instead of ''...
I am going to pass on this one for now. 0
is still converted to ""
, so we'll have to cast to string the value if there's a chance the number 0 could be used as a class name.
Closing as there was no noticeable perf result difference and for the reasons given above.
Thank you, anyway @EdwardDrapkin! 👋
By reordering some of the logic, we can optimize the function's runtime efficiency.
By doing the Array.isArray() check first, we catch the most common cases and can elide the (moderately expensive) typeof operations. By separating the isArray() check from the length check, we can elide even more operations in the case where an empty array is passed. We can reduce even more operations by not recursing when there are simple (string/number) types in the array and concatenating them directly.
This fork should be faster in all cases.
Here's some benchmark results (classcat = my fork, classcatNpm = as it exists now).