lukeed/clsxPublic

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

feat: enhance performance #26

Mergedjalalazimimerged 4 commits intomasterfromrefs/pull/26/head
  1. JA

    Hi friends, I did some minor changes to increase performance. the benchmark result is:

    ``` # Strings classcat* x 6,206,358 ops/sec ±1.24% (90 runs sampled) classnames x 3,408,198 ops/sec ±0.88% (92 runs sampled) clsx (prev) x 7,434,060 ops/sec ±0.92% (93 runs sampled) clsx x 8,112,152 ops/sec ±0.90% (92 runs sampled)

    # Objects classcat* x 5,865,653 ops/sec ±0.89% (92 runs sampled) classnames x 3,108,156 ops/sec ±0.90% (92 runs sampled) clsx (prev) x 4,765,299 ops/sec ±0.83% (93 runs sampled) clsx x 6,063,173 ops/sec ±0.93% (93 runs sampled)

    # Arrays classcat* x 5,471,773 ops/sec ±0.81% (92 runs sampled) classnames x 1,811,028 ops/sec ±0.71% (94 runs sampled) clsx (prev) x 5,606,905 ops/sec ±1.12% (91 runs sampled) clsx x 5,710,108 ops/sec ±0.94% (92 runs sampled)

    # Nested Arrays classcat* x 4,333,798 ops/sec ±0.97% (90 runs sampled) classnames x 1,066,391 ops/sec ±0.70% (93 runs sampled) clsx (prev) x 4,351,329 ops/sec ±1.05% (93 runs sampled) clsx x 4,505,775 ops/sec ±0.94% (92 runs sampled)

    # Nested Arrays w/ Objects classcat* x 4,524,749 ops/sec ±0.99% (92 runs sampled) classnames x 1,716,114 ops/sec ±0.78% (94 runs sampled) clsx (prev) x 4,239,522 ops/sec ±0.96% (91 runs sampled) clsx x 4,702,464 ops/sec ±0.96% (93 runs sampled)

    # Mixed classcat* x 4,826,735 ops/sec ±0.89% (94 runs sampled) classnames x 2,144,730 ops/sec ±0.92% (91 runs sampled) clsx (prev) x 4,651,749 ops/sec ±1.04% (89 runs sampled) clsx x 5,129,713 ops/sec ±0.89% (91 runs sampled)

    # Mixed (Bad Data) classcat* x 1,400,863 ops/sec ±0.87% (92 runs sampled) classnames x 1,038,999 ops/sec ±0.81% (93 runs sampled) clsx (prev) x 1,708,531 ops/sec ±0.98% (93 runs sampled) clsx x 1,830,703 ops/sec ±1.03% (93 runs sampled) ```

    > Perf on MacBook Pro, 2.9 GHz Quad-Core Intel Core i7, 16 GB Ram

  2. CC

    ## [Codecov](https://app.codecov.io/gh/lukeed/clsx/pull/26?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Luke+Edwards) Report All modified and coverable lines are covered by tests :white_check_mark: > Comparison is base [(`6da37d6`)](https://app.codecov.io/gh/lukeed/clsx/commit/6da37d67472ea9eb177a4ea9a9fa2f3f4a3c445e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Luke+Edwards) 100.00% compared to head [(`8adf785`)](https://app.codecov.io/gh/lukeed/clsx/pull/26?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Luke+Edwards) 100.00%. > Report is 12 commits behind head on master.

    :exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

    <details><summary>Additional details and impacted files</summary>

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

    </details>

    [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/lukeed/clsx/pull/26?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Luke+Edwards). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Luke+Edwards).

  3. lukeedreferenced this in Could default exported function perform a tiny bit better? #286 years ago on GitHub

  4. SA

    Looks good. Could this be merged?

  5. DS

    Possible for this to be merged?

  6. ET

    @lukeed I bet you're really busy, but any chance to get this reviewed/merged? The PR's been open for nearly 8 months.

  7. lukeedmentioned5 years ago on GitHub

  8. lukeedsubscribed5 years ago on GitHub

  9. ZeeshanTambolireferenced this in [internal] Replace `classnames` utility with `clsx` dependency #15865 years ago on GitHub

  10. KA

    @lukeed any chance to get this merged? thanks for this great library :)

  11. lukeedmentioned5 years ago on GitHub

  12. lukeedsubscribed5 years ago on GitHub

  13. SU
    Hidden as resolved.
  14. SU

    No review summary.

  15. JA
    Author
    Hidden as resolved.
  16. JA
    Author

    No review summary.

  17. ER

    No review summary.

  18. BR

    Has this been merged?

  19. LU

    Does it look merged? :D

    Waiting because all these PRs have varying results depending on the minor (or patch) version when benchmarking. There’s needs to be a clear/obvious pattern before accepting and releasing a new patch for all to consume, especially with a cost of a few extra bytes

  20. YO

    Cashing the value avoids transversing the prototype chaining by looking for such a `.length` property, as some of you know.

    @lukeed I know you said, "It doesn't matter": https://github.com/lukeed/clsx/issues/28#issuecomment-703781445

    I tend to agree with you.

    Except, it is such simple things to change without trading off complexity that I would take the code snippet from https://github.com/lukeed/clsx/issues/28

    I am biased towards taking the "improvements."

  21. lukeedmentioned3 years ago on GitHub

  22. lukeedsubscribed3 years ago on GitHub

  23. ernes-tounsubscribed3 years ago on GitHub

  24. LU
    Hidden as resolved.
  25. LU
    Hidden as resolved.
  26. LU
    lukeedChanges requested

    No review summary.

  27. LU

    The reason for the delay is that the code changes performed worse (if anything) for most of the recent Node/V8 versions due to the change in `while`->`for` loop. Caching the length had no noticeable effect for the majority of this time. (These things often come & go)

    Merging now only because there's reproducible effect in the latest 20.x track. It's an increase of 5 bytes (2%) for an increase of -2% to 6% performance change. Numbers below. It's worth the trade-off only because the 6% perf gain is seen in the `Strings` case which is how nearly everyone uses clsx.

    Thanks for the PR & patience @jalalazimi. However if you plan to keep your project you're currently in violation of MIT license. The source is near identical & there's no acknowledgement nor attribution anywhere. It's most clearly a fork & should be labeled as such.

    **Benchmark Results**

    ``` Node 20.8.1

    # Strings classnames x 6,576,471 ops/sec ±0.38% (95 runs sampled) clsx (pr26) x 12,933,752 ops/sec ±0.11% (95 runs sampled) clsx (main) x 12,142,343 ops/sec ±0.15% (99 runs sampled)

    # Objects classnames x 6,318,942 ops/sec ±0.12% (99 runs sampled) clsx (pr26) x 9,473,723 ops/sec ±0.10% (100 runs sampled) clsx (main) x 9,466,986 ops/sec ±0.10% (99 runs sampled)

    # Arrays classnames x 3,545,087 ops/sec ±0.22% (99 runs sampled) clsx (pr26) x 9,444,148 ops/sec ±0.09% (102 runs sampled) clsx (main) x 9,118,411 ops/sec ±0.17% (96 runs sampled)

    # Nested Arrays classnames x 2,072,197 ops/sec ±0.20% (98 runs sampled) clsx (pr26) x 7,087,216 ops/sec ±0.32% (98 runs sampled) clsx (main) x 7,202,070 ops/sec ±0.52% (99 runs sampled)

    # Nested Arrays w/ Objects classnames x 3,155,991 ops/sec ±0.40% (94 runs sampled) clsx (pr26) x 7,312,535 ops/sec ±0.50% (95 runs sampled) clsx (main) x 7,121,246 ops/sec ±0.27% (100 runs sampled)

    # Mixed classnames x 4,163,925 ops/sec ±0.28% (97 runs sampled) clsx (pr26) x 8,086,030 ops/sec ±0.40% (98 runs sampled) clsx (main) x 7,921,147 ops/sec ±0.20% (97 runs sampled)

    # Mixed (Bad Data) classnames x 1,909,345 ops/sec ±0.08% (101 runs sampled) clsx (pr26) x 2,973,733 ops/sec ±0.19% (99 runs sampled) clsx (main) x 2,964,893 ops/sec ±0.10% (101 runs sampled) ```

  28. jalalazimimentioned2 years ago on GitHub

  29. jalalazimisubscribed2 years ago on GitHub

  30. lukeedmerged deff09b0fe402 years ago on GitHub

  31. lukeedclosed2 years ago on GitHub

  32. JA
    Author

    Thank you, @lukeed, for your prompt response; I really appreciate your support. I'll make sure to add a note explaining the purpose of my project and update the benchmark results accordingly.

  33. lukeedmentioned2 years ago on GitHub

  34. lukeedsubscribed2 years ago on GitHub

  35. rsazreferenced this in [Snyk] Upgrade clsx from 2.0.0 to 2.1.1 #512 years ago on GitHub

  36. salgadevreferenced this in [Snyk] Upgrade clsx from 2.0.0 to 2.1.1 #172 years ago on GitHub

  37. wierzareferenced this in [Snyk] Upgrade clsx from 2.0.0 to 2.1.1 #1472 years ago on GitHub

  38. gweesinreferenced this in chore: bump deps #8last year on GitHub