classnames
classnames copied to clipboard
Fix bug where objects are converted to '[Object object]' in a VM
Fixes #240
I added a test case that fails against the original implementation.
This comes with a bit of a perf hit but unfortunately there's no way around it if you want to reliably detect the native toString
function. I made a JSBench so you can see the difference: https://jsbench.me/1xkpq8eozj
I think we might have to languish in the fact that we added this feature knowing that this could be a risk. No free lunch.
Thanks for this pull request @markdalgleish , I'll try it out as soon as I can :+1:
hey, guys, wondering if there's anything blocking this from being merged.
@juangl as first step, we probably need a benchmark to compare this against the existing code - some questions might appear if the benchmark is severely impacted
@juangl as first step, we probably need a benchmark to compare this against the existing code - some questions might appear if the benchmark is severely impacted
I agree with your statement about the existing code
Benchmark output from today
Mozilla/5.0 (X11; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0
* local#strings x 10,290,716 ops/sec ±1.33% (67 runs sampled)
* npm#strings x 10,321,700 ops/sec ±1.40% (67 runs sampled)
* local/dedupe#strings x 2,231,927 ops/sec ±0.33% (68 runs sampled)
* npm/dedupe#strings x 2,251,416 ops/sec ±0.28% (68 runs sampled)
> Fastest is npm#strings
* local#object x 1,764,792 ops/sec ±1.92% (49 runs sampled)
* npm#object x 7,663,848 ops/sec ±1.90% (63 runs sampled)
* local/dedupe#object x 1,620,503 ops/sec ±1.94% (57 runs sampled)
* npm/dedupe#object x 4,105,373 ops/sec ±0.85% (64 runs sampled)
> Fastest is npm#object
* local#strings, object x 1,733,121 ops/sec ±2.83% (33 runs sampled)
* npm#strings, object x 5,691,563 ops/sec ±0.64% (66 runs sampled)
* local/dedupe#strings, object x 1,184,113 ops/sec ±1.60% (61 runs sampled)
* npm/dedupe#strings, object x 2,082,282 ops/sec ±0.23% (67 runs sampled)
> Fastest is npm#strings, object
* local#mix x 624,883 ops/sec ±2.59% (58 runs sampled)
* npm#mix x 3,176,306 ops/sec ±0.69% (67 runs sampled)
* local/dedupe#mix x 432,948 ops/sec ±4.70% (60 runs sampled)
* npm/dedupe#mix x 837,616 ops/sec ±7.09% (62 runs sampled)
> Fastest is npm#mix
* local#arrays x 572,171 ops/sec ±1.72% (57 runs sampled)
* npm#arrays x 1,285,368 ops/sec ±0.73% (64 runs sampled)
* local/dedupe#arrays x 444,003 ops/sec ±1.69% (61 runs sampled)
* npm/dedupe#arrays x 754,877 ops/sec ±0.40% (66 runs sampled)
> Fastest is npm#arrays
Finished
This change has a severe performance impact on the object
path, with performance dropping by half, down to a fifth in some instances.
I don't know if that trade-off is ecofriendly for running this in a vm
, but I understand the plight.
Merged in https://github.com/JedWatson/classnames/pull/281, with the typical usage performance problems resolved