return undefined for empty class list

[jakubsadura]

Fixes #182

[dcousens]

This is a breaking change unfortunately.

If anyone assumed the returned value was '' (a string always), then they might be performing string concatenation based on that assumption (I know I have).

The end result is that 'undefined' may appear.

[jakubsadura]

You're right. Can you ship it with major version bump? I can update the docs if needed

[dcousens]

I don't think this will ever ship to be honest.
The library is so prolific, bumping to a new major would still be quite disruptive for users.

My understanding is the benefits do not out weigh the risks.

[jakubsadura]

I think that this should be shipped - in the worst case if someone does not follow semver there would be unexpected undefined class which I believe would not result in style changes (I didn't check but I don't expect many projects use undefined as CSS class name and at the same time concatenate string from your lib). If this change would harm someone then it may be a good thing - chance to improve (add e2e / snapshot tests / start following semver).

[dcousens]

I think this change is scary enough, that I'd want confirmation from @JedWatson and other users of the package to weigh in before anything was changed or released.

[JedWatson]

So in terms of avoiding unexpected behaviour, I don't think we can ship this. It's not so much the hassle of a major version, as the possible usage where undefined gets coerced into a string.

Imagine the following case where the usage of classNames is abstracted away in a library:

// in a library
function getClassnameForButton(opts) {
  // this could return undefined
  return classnames(opts.specialClass);
}

// in user-land
const MyButton = (props) => <button class={`${getClassnameForButton(props)} my-button`} />

In this case the button would have a class of undefined my-button

That's pretty contrived but I'm sure there are more valid scenarios where the usage of classnames is not necessarily known to the component, and while I can totally appreciate not wanting empty class properties on components (especially for SSR) I think the priority for this library is safety and predictability.

You can always wrap it, if concatenating strings onto the result is something you'd never do in your components:

const cx = args => classnames(...args) || undefined;
const MyThing = ({ customClass }) => <span class={cx(customClass)} />;

[JedWatson]

Also worth mentioning that we're talking about React here but this library is framework-agnostic, and I'm not sure we can guarantee that this isn't used in an environment where the result could be coerced into a string anyway.

[vladshcherbin]

@JedWatson maybe it's possible to export a separate function which return undefined, cxu or any other name so we don't need wrappers in our projects 🤔

[jakubsadura]

Good point @JedWatson
What do you think of @vladshcherbin idea?

[kylemh]

@JedWatson I don't think your example is contrived, but one that's possible. More importantly, I'd say that shouldn't be a concern in terms of making an improvement to the library. That same user should be told that they should be using classNames in the class attribute value as well (since the function could potentially return undefined).

You say that the library's priority should be safety and predictability, but I found the issue this PR attempts to solve to be unexpected. While a massive micro-optimization, you'll also be helping the web to ship smaller bundles collectively with this change.

Major version bumps are exactly for this purpose, and people can choose not to upgrade if they'd like to maintain old behavior. I just wanted to add another voice to the discussion in hopes that you'd change your mind. No worries if not.

[dcousens]

LGTM, but, its a breaking change.

[lokeshjain2008]

you should check if undefined will become string in the template

[lokeshjain2008]

empty string is better and predictable for UI developers.

[kylemh]

... the whole point of this PR is to make it undefined. Read the above posts to understand why.

[soluml]

The npm package has an alternate dedupe and bind version. I understand this PR as being problematic for projects that expect empty string so, thoughts on another alternate version? All it'd need to do is bring in the base class and and add the empty string check to return undefined.

I use classnames in many of my projects and in most of those I end up writing a wrapper util to do the above. Would love to get it OOTB in a single npm install! :)

[dcousens]

As is, I think this PR should close for the aforementioned breaking change reasons, but as hinted by @soluml maybe the path forward is another variant.
If you feel strongly about the feature, I'd be open to that PR, but, a reasonable warning is that feature development on this package is generally quite slow.