Take array as argument? about classnames HOT 14 CLOSED

What would the array do?

I've been considering an array-based syntax to allow dynamic class names to be conditionally added. It's bit of a weird syntax but one that would solve some otherwise awkward code.

e.g.

var a = 'variable-class-name';
var b = true;
classNames('static-class-name', [a, b]) === 'static-class-name variable-class-name';

That would, of course, be incompatible with using arrays as lists of classNames to include, which is what I think you're suggesting:

classNames(['a', 'b'], 'c') === 'a b c';

... however I think it would be more useful. Feedback welcome.

from classnames.

Yes, what I was suggesting is just using arrays as values. For one reason or another you might build up a list of classnames as an array and this function could then be used to apply them. Here's are two simple examples:

// conditionally build array of dynamic class names
var arr = ['a', 'b'];
if (c) arr.push('super' + c);
classNames(arr);

// use an array of classnames that is stored as data somewhere
var prefabArr = require('./classesForThing');
classNames(prefabArr.push('newone'));

Both of these situations (and others like them) could be addressed by dynamically building an object with true values. (The array is just more succinct.)

var obj = { a: true, b: true };
if (c) obj['super' + c] = true;
classNames(obj);

The same could be said for the array syntax you were considering. In fact, it seems especially appropriate in that case because you are essentially doing the same thing that an object does. Why not this instead (?):

classNames({
  'static-class-name': true,
  'variable-class-name': b
});

from classnames.

Just came here to open the same ticket, I'd be +1 on using arrays as values (and recursively, so you don't need to worry about flattening them).

In ES6 (via Babel) computed properties are valid so the [key, bool] array syntax eventually won't be necessary:

classNames({
  'static-class-name': true,
  [`variable-class-${name}`]: b
});

from classnames.

In ES6 (via Babel) computed properties are valid so the [key, bool] array syntax eventually won't be necessary

Good point. PR is in review.

from classnames.

Humm I'm somewhat not in favour of this, for a few reasons.

1. Cramming and trying to overload this library seems wrong. More so for recursive arrays. I feel it's more of the user's job to make sure the right arguments (and types) are passed, and not blindly somehow pass nested arrays and hope it'll all work out in the end.
2. Perf concerns. This function might be used a lot. This is penalizing 90% of the use-cases.
3. classNames(bla1, bla2, ...restOfYourArraySpread). Let's leverage (future) language constructs (or use apply, concat, whatever) instead of making this function accept arbitrary data structures.
4. Opens up a floodgate of possibilities: if [{a: true}] is allowed, why shouldn't {a: ['b']} be? etc. Might be a straw man argument though.

from classnames.

I'm not certain the change is important and I'm certainly open to it being turned down, but I'd be interested in hearing some more about the problems, @chenglou (again, not strongly opposed to your opposition, but would like to hear your arguments fleshed out --- in large part for my own education):

1. You really think adding array support is "cramming"? It's still a very simple module (and see 4).
2. What are the perf concerns? I'm curious. Is it just using a named function to allow for recursion? Or is there another concern?
3. If the alternatives are adding a few lines of code to this module or requiring that users be doing es6 transpilation to get that functionality, isn't it easier on users to add the few lines of code?
4. I think you're right that this is a straw man argument. The change does not mean accepting "arbitrary data structures" that people can "blindly somehow pass and hope it'll work out": it's accepting another structure that can be as clearly documented, tested, and delimited as strings and objects.

from classnames.

@chenglou ah, good call on classNames(bla1, bla2, ...restOfYourArraySpread) didn't even think of that. With that said, I could go either way on this.

from classnames.

Following up on @chenglou's comment I tried to do some comparisons with JSPerf tests, and I did see some performance losses with the PR changes. Definitely the recursion is a big culprit, and so is the regular expression. If the recursive functionality @tgriesser asked about were dropped, both could be done away with, and other tweaks could probably be done to micro-optimize the PR code and bring it up closer to the current speed. That said, though, if nobody else is really interested in accepting arrays (without .apply()) I don't think that work would be worthwhile and we could just close this issue and PR. Thoughts?

from classnames.

To start with, I'm basically in agreement with @chenglou. I think performance and simplicity should be our highest priorities.

In ES6 (via Babel) computed properties are valid so the [key, bool] array syntax eventually won't be necessary

... same argument goes for array support with ..spread arguments. So I think this is a fairly negligible enhancement to the usability of the function.

however I had an idea on how it could be implemented with extremely little overhead (I think) where you only pay the cost if you actually use the feature, so I thought I'd turn it into a PR for feedback.

Let me know what you think of #14.

@davidtheclark would you mind sharing your JSPerf setup? it would be great (for this and future PRs) to get some benchmarking in this package.

from classnames.

That PR is pretty much exactly what I had going before the switch intruded into my mind -- so looks good to me :) I did not think of perf implications when I made the switch switch.

I didn't have anything set up in JSPerf that you'd want to reuse. Pretty much just plugged in the existing function and some variations. Here's a starting point if it helps: http://jsperf.com/classnames

And yeah, I'd agree that if performance is priority (which I guess seems justified by the fact that somebody -- me, even -- might use this for every class list on their page) then we'd probably want to go with your PR #14 or even just keep the current .apply() recommendation (which will be faster when using an array than the #14 method, because won't have to type check).

from classnames.

Cheers @davidtheclark, that JSPerf was a good base.

After discussion with @dcousens I implemented a much faster check for Arrays in my PR: arg.constructor === Array which works in IE8+ and every other browser that React is compatible with. That seems like a reasonable compatibility base.

Based on my updated JSPerf tests (http://jsperf.com/classnames/3) it adds basically no performance overhead, so I think that concern's been taken off the table.

Going to leave this open for further feedback for now, but at this point I think I'd be happy to merge #14 and support array recusion as part of the API going forward.

from classnames.

Actually after some more testing (different browsers) I'm seeing about a 3% negative performance impact on the new version in Safari (OS X) with about 1.3m ops per second. Would be great if everybody else could hit the perf test too and report back if they also see much of an impact so we can gauge whether this is worth it.

http://jsperf.com/classnames/3

from classnames.

classNames(bla1, bla2, ...restOfYourArraySpread). Let's leverage (future) language constructs (or use apply, concat, whatever) instead of making this function accept arbitrary data structures.

I personally think this is the smarter approach over accepting an Array argument, the increased code complexity combined with maybe a tiny performance hit, just seems redundant when this is so readily applied.

Write programs that do one thing, and do it well

from classnames.

#16 added this and made classNames much faster, so it's in.

from classnames.