lukeed / clsx

A tiny (239B) utility for constructing `className` strings conditionally.
MIT License
8.08k stars 141 forks source link

Add support for objects with `toString` methods #52

Open sandgraham opened 1 year ago

sandgraham commented 1 year ago

Hi there, I'm using a css-in-js library (stitches) which returns class names as objects with custom toString methods. This requires calling toString before passing the "class names" to clsx- otherwise clsx assumes the object is a config/mapping object.

// Current usage
const pink = css({ color: "pink" });
clsx(pink().toString())

I currently have a patch package which adds an additional check to the toVal function, checking to see if a mix object has it's own toString method and calls/returns that if so.

// General idea
if (mix.hasOwnProperty("toString")) {
  str += mix.toString();
}

I noticed that classnames has a condition for this (original PR). React also calls toString if you pass an object to className. I'm wondering if it might be useful to include this behavior in clsx? I can contribute a PR if so.

lukeed commented 1 year ago

it might be worth looking into a codemod to automatically call the stitches' toString() method. For example, it could definitely be worked into the clsx babel plugin.

Strings are always the best (as in most performant) path for clsx to operate, so our current usage is definitely the preferred approach. Adding a toString check to clsx itself would be a significant performance hit (and byte size, percentage-wise). And your hasOwnProperty example snippet wouldn't suffice as it wouldnt cover class instances that have a toString method on its prototype.

It'd have to be something like:

// ...
if (typeof mix.toString === 'function' && mix.toString !== Object.prototype.toString) {
  str && (str += ' ');
  str += mix.toString();
} else for (k in mix) {
  if (mix[k]) {
    str && (str += ' ');
    str += k;
  }
}

TBD - I'll run some benches to actually measure the impact