React: bring classnames library into core react

[capaj]

Do you want to request a feature or report a bug?
a new feature
What is the current behavior?

What is the expected behavior?
It renders a className string as if I wrapped my object with a classnames

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
not it did not

I don't know anyone who would be using the current behaviour for their benefit. This is such a trivial thing to add, yet it provides a great value for every react developer I know.
I understand react tries to be as light as possible, but I can't help but wonder-is it worth the extra trouble by deferring such a trivial thing to the react ecosystem?
No, it's not worth the extra http request, it's not worth an extra npm dependency. The whole function is like 20 lines long!

Is there a consensus in the react community that this is the best way to transform objects into classnames?
Yes- on NPM react was downloaded 3.3 million times last month and classnames 2 million times. Seems like comfortable majority uses classnames library already.

So maybe let's add it to the core? Pretty please?

[aweary]

Hey @capaj, thanks for bringing up the idea. classnames is definitely a popular way to manage dynamic classes with React, but I think we'd be assuming too much to it's the solution to the problem. React has an explicit goal of maintaining a small API surface and making the className prop polymorphic would be extending that API for minimal gains (as you said, it's only 20 lines, not a big dependency to pull in).

It would also make className parsing slower for everyone by default since React would always have to check the type of the className prop, instead of assuming it was a string. Integrating community-driven packages into a core library is also rarely good for the long-term maintainability of the package since it now has to fight for attention with core issues. I think it's likely best that it stays the way it is. Thanks for taking the time to share your thoughts, though!

[gaearon]

No, it's not worth the extra http request

Just to be clear, if you use a library with a bundler like Webpack/Browserify/Brunch/Rollup, you don't actually make an extra request. It gets bundled in your code, just like if it existed in React. But people who don't need it (or use a custom way of doing it) won't have to pay the bandwidth for its implementation.

The question itself has been brought up before: https://github.com/facebook/react/issues/4644#issuecomment-132265668. We might come back to it, but probably not very soon.

[capaj]

@gaearon I was talking about the http request you do when you install classnames from npm.

@aweary

minimal gains (as you said, it's only 20 lines, not a big dependency to pull in).

Just to give an example-I have only a midsized project yet I use classnames() on more than 120 classNames props in 56 files. That means extra 56 lines just for imports and extra 120 calls of classnames(). For my typical project, my final bundle size would actually go down if react supported this.

It would also make className parsing slower for everyone by default since React would always have to check the type of the className prop, instead of assuming it was a string.

this could be easily mitigated by using an alternative name for the prop-something like classMap? Sounds silly, but probably better than classObject or classHash

Integrating community-driven packages into a core library is also rarely good for the long-term maintainability of the package since it now has to fight for attention with core issues.

I agree in general, but I think this just might be the "rare" case when it is a good idea. The whole thing is implemented like this:

function classNames () {
  var classes = [];
  for (var i = 0; i < arguments.length; i++) {
    var arg = arguments[i];
    if (!arg) continue;
    var argType = typeof arg;
    if (argType === 'string' || argType === 'number') {
      classes.push(arg);
    } else if (Array.isArray(arg)) {
      classes.push(classNames.apply(null, arg));
    } else if (argType === 'object') {
      for (var key in arg) {
        if (hasOwn.call(arg, key) && arg[key]) {
          classes.push(key);
        }
      }
    }
  }
  return classes.join(' ');
}

now I am not a very good programmer, but I'd bet my left pinky that it does not have any bugs. It might fall over if you do something nasty with your JS env like extend Object.prototype, but that's not the implementation's problem.

EDIT: for a standalone classMap property, it would be totally trivial :

function classMap () {
 var classes = [];
 for (var key in arg) {
    if (hasOwn.call(arg, key) && arg[key]) {
      classes.push(key);
    }
  }
  return classes.join(' ');
}

[gaearon]

As I mentioned, this was requested before, and we might consider it in future versions. We'd like to do it at the same time as we revisit how styling is done in React. We wouldn't like to introduce a new API now just to break everyone again later.

That means extra 56 lines just for imports

If importing something from React is less “expensive” than importing something from another library in terms of bundle size, this seems like a problem with the bundler.

[capaj]

If importing something from React is less “expensive” than importing something from another library in terms of bundle size, this seems like a problem with the bundler.

no, because you would not import anything:

import classnames from 'classnames'

() => <div className={classnames({conditionalClass: true})}></div>

vs

() => <div classMap={{conditionalClass: true}}></div>

We'd like to do it at the same time as we revisit how styling is done in React

:heart:

[gaearon]

Ah I got it, sorry for confusion. Yea, we might support this in the future, but as part of a larger effort. Probably not on the table in the next few versions.