Comments (14)
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.
- 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.
- Perf concerns. This function might be used a lot. This is penalizing 90% of the use-cases.
classNames(bla1, bla2, ...restOfYourArraySpread)
. Let's leverage (future) language constructs (or useapply
,concat
, whatever) instead of making this function accept arbitrary data structures.- 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):
- You really think adding array support is "cramming"? It's still a very simple module (and see 4).
- What are the perf concerns? I'm curious. Is it just using a named function to allow for recursion? Or is there another concern?
- 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?
- 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.
Related Issues (20)
- why not using one build tool for publish the npm package? HOT 2
- Incomplete type signature for classnames/bind HOT 2
- Advanced typing for classnames/bind HOT 4
- Explain to me about "cx" HOT 1
- Getting error says 'this' context of type 'void' is not assignable HOT 4
- [Question] When I used Object's value to computed key, what happened? HOT 2
- SyntaxError: Unexpected end of script (Mobile Safari) HOT 5
- When no classes are selected, classNames should return null HOT 5
- Vulnerable dependencies (indirect security issues) HOT 1
- How to erase or replace the properties of the conditional class from external component? HOT 1
- why number can be a className HOT 1
- Can we add exports field for this package? HOT 4
- Need a maintainer? HOT 7
- Preparing for version 3.0 HOT 9
- Should we support CommonJS for 3.0? HOT 5
- No default export (v2.5.1) HOT 5
- Import from 'classnames' broken since v2.5.0 HOT 5
- Please use GitHub Releases HOT 6
- Unable to do v2 releases HOT 2
- Feature Request: Enhanced Responsive Classnames Support HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from classnames.