lukeed/clsxPublic

A tiny (239B) utility for constructing `className` strings conditionally.

increase perf && smaller size #23

ClosedXaveScorclosed 4 commits intomasterfromrefs/pull/23/head
  1. XA

    Size before: ``` Filename Filesize (gzip) dist\clsx.js 358 B 230 B dist\clsx.m.js 357 B 228 B dist\clsx.min.js 517 B 290 B ``` Size after: ``` Filename Filesize (gzip) dist\clsx.js 357 B 226 B dist\clsx.m.js 356 B 224 B dist\clsx.min.js 516 B 286 B ```

    Perf on my Dell xps 13 9360 Core-i78550U: ``` # Strings classcat* x 6,205,951 ops/sec ±1.11% (88 runs sampled) classnames x 3,171,871 ops/sec ±0.81% (87 runs sampled) clsx (prev) x 7,099,800 ops/sec ±1.39% (88 runs sampled) clsx x 7,602,434 ops/sec ±1.93% (82 runs sampled)

    # Objects classcat* x 5,344,027 ops/sec ±1.82% (86 runs sampled) classnames x 2,986,700 ops/sec ±0.80% (92 runs sampled) clsx (prev) x 4,597,446 ops/sec ±1.39% (86 runs sampled) clsx x 6,105,959 ops/sec ±1.16% (91 runs sampled)

    # Arrays classcat* x 4,923,076 ops/sec ±2.19% (85 runs sampled) classnames x 1,643,135 ops/sec ±0.86% (90 runs sampled) clsx (prev) x 5,329,721 ops/sec ±1.08% (87 runs sampled) clsx x 5,498,393 ops/sec ±1.56% (88 runs sampled)

    # Nested Arrays classcat* x 3,995,324 ops/sec ±1.78% (83 runs sampled) classnames x 1,010,578 ops/sec ±0.74% (93 runs sampled) clsx (prev) x 3,977,924 ops/sec ±1.73% (86 runs sampled) clsx x 4,258,622 ops/sec ±1.76% (86 runs sampled)

    # Nested Arrays w/ Objects classcat* x 4,425,302 ops/sec ±1.25% (88 runs sampled) classnames x 1,578,490 ops/sec ±0.89% (90 runs sampled) clsx (prev) x 3,967,503 ops/sec ±1.34% (85 runs sampled) clsx x 4,465,872 ops/sec ±2.08% (86 runs sampled)

    # Mixed classcat* x 4,336,818 ops/sec ±2.65% (85 runs sampled) classnames x 2,016,569 ops/sec ±0.94% (91 runs sampled) clsx (prev) x 4,304,082 ops/sec ±1.77% (86 runs sampled) clsx x 4,876,592 ops/sec ±1.97% (86 runs sampled)

    # Mixed (Bad Data) classcat* x 1,092,890 ops/sec ±3.88% (78 runs sampled) classnames x 946,727 ops/sec ±1.09% (91 runs sampled) clsx (prev) x 1,400,982 ops/sec ±2.10% (84 runs sampled) clsx x 1,613,103 ops/sec ±1.57% (86 runs sampled) ```

  2. CC

    # [Codecov](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=h1) Report > Merging [#23](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=desc) into [master](https://codecov.io/gh/lukeed/clsx/commit/c5b2b21ab8aea48ff0b48649f386a2c7280fabf2&el=desc) will **not change** coverage. > The diff coverage is `100.00%`.

    [![Impacted file tree graph](https://codecov.io/gh/lukeed/clsx/pull/23/graphs/tree.svg?width=650&height=150&src=pr&token=EYANLEDHtq)](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=tree)

    ```diff @@ Coverage Diff @@ ## master #23 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 22 22 ========================================= Hits 22 22 ```

    | [Impacted Files](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [src/index.js](https://codecov.io/gh/lukeed/clsx/pull/23/diff?src=pr&el=tree#diff-c3JjL2luZGV4Lmpz) | `100.00% <100.00%> (ø)` | |

    ------

    [Continue to review full report at Codecov](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=footer). Last update [c5b2b21...064a2e5](https://codecov.io/gh/lukeed/clsx/pull/23?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

  3. LU

    This is a regression. Number type is returned.

    src/index.js:5head35deeaa
  4. LU

    Why did these vars get swapped?

    src/index.js:10head35deeaa
  5. LU

    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.

  6. XA
    Author

    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.

    src/index.js:5head35deeaa
  7. XA
    Author

    No review summary.

  8. XA
    Author

    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.

    src/index.js:10head35deeaa
  9. XA
    Author

    No review summary.

  10. XA
    Author

    I run the bench for current master and my PR separately and found some perf regression

    Master: ``` # Strings clsx x 8,041,328 ops/sec ±0.63% (91 runs sampled)

    # Objects clsx x 5,900,388 ops/sec ±1.84% (88 runs sampled)

    # Arrays clsx x 5,443,679 ops/sec ±1.21% (89 runs sampled)

    # Nested Arrays clsx x 4,078,254 ops/sec ±1.87% (86 runs sampled)

    # Nested Arrays w/ Objects clsx x 4,239,252 ops/sec ±2.31% (84 runs sampled)

    # Mixed clsx x 4,817,408 ops/sec ±1.43% (90 runs sampled)

    # Mixed (Bad Data) clsx x 1,638,964 ops/sec ±0.96% (89 runs sampled) ```

    PR: ``` # Strings clsx x 7,336,325 ops/sec ±1.83% (87 runs sampled)

    # Objects clsx x 5,717,162 ops/sec ±2.41% (83 runs sampled)

    # Arrays clsx x 5,551,438 ops/sec ±1.53% (89 runs sampled)

    # Nested Arrays clsx x 4,098,543 ops/sec ±1.70% (85 runs sampled)

    # Nested Arrays w/ Objects clsx x 4,149,760 ops/sec ±2.45% (84 runs sampled)

    # Mixed clsx x 4,865,544 ops/sec ±1.20% (88 runs sampled)

    # Mixed (Bad Data) clsx x 1,572,783 ops/sec ±1.56% (86 runs sampled) ```

    Is this tradeoff applicable?

  11. lukeedreferenced c520353bd6ff3 years ago on GitHub

  12. WO

    A variant I did:

    ```js 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 `return str` to BOTH `if` and `else` parts of `toVal()` shaved off additional 3 bytes!

  13. AZ

    i've small little suggestion tho... why not

    ```ts var k, y, str='', a = typeof mix;

    if (a === 'string' || a === 'number') {} if (a === 'object') {} ```

  14. WO

    @AzrizHaziq Believe or not, this does not make the Gzipped version smaller

  15. AzrizHaziqmentioned3 years ago on GitHub

  16. AzrizHaziqsubscribed3 years ago on GitHub

  17. LU

    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:

    1. While the `return val; // number` isn't a regression in tests/output, it's a regression in the function signature itself. Because the function can now _sometimes_ return numbers instead of only strings, it's now polymorphic which is a deopt in V8 & therefore less performant. The benchmark results posted don't show this because there are no numbers in the benchmark itself. 2. Minifying (via terser or most any) automatically rewrites the variables such that the variables are either 1) already the same or 2) something different anyway. So it doesn't really matter what we call them in the source since it's across function boundaries. This PR, however, did show me that I should be using the `y` variable [here](https://github.com/lukeed/clsx/blob/master/src/index.js#L18-L23) – but only because `k` is reserved for number types.

    Thanks again!

  18. lukeedclosed2 years ago on GitHub

  19. lukeedreferenced 554ad31e92b22 years ago on GitHub