6 ms·

I was curious so I checked out their Github page. I found this example: classNames('foo', 'bar'); // => 'foo bar' classNames('foo', { bar: true }); // => '

I was curious so I checked out their Github page. I found this example: classNames('foo', 'bar'); // => 'foo bar' classNames('foo', { bar: true }); // => 'foo bar' classNames({ 'foo-bar': true }); // => 'foo-bar' classNames({ 'foo-bar': false }); // => '' classNames({ foo: true }, { bar: true }); // => 'foo bar' classNames({ foo: true, bar: true }); // => 'foo bar' ... var btnClass = classNames({ btn: true, 'btn-pressed': this.state.isPressed, 'btn-over': !this.state.isPressed && this.state.isHovered }); Look like the button example is a good enough convention for me, I don't need any of the other conventions. Can anyone tell me why this is not sufficient?: const classNames = (pojo, c=[]) => { for (let k in pojo) if (pojo[k]) c.push(k); return !!c.length && c.join(" "); } Do I need array-flattening support, multiple argument support? The size of this library probably comes from it supporting a handful of different coding styles. I would say the size (50 lines) is too large, if anything. Dependency worthy? I'm not convinced. I am very much so against requiring trivial utility functions from other people. Roll your own if it's trivial! It's better than depending on something over the network.

paperpunk 8y ago
Roll your own, untested function vs a small, widely used library with a well-understood, well-tested API that your devs have a good chance of being familiar with already. I too want to avoid bloated npm dependencies but this seems like a fair engineering trade-off..
- anonytrary 8y ago
  This is a trivial function, your argument applies to complex utilities. It is piss easy to roll your own, and it's not worth introducing a potential attack vector via a new dependency. In this case, if your devs can't keep up with your classNames function, fire them, because it's trivial. it("should work with no classes", () => { expect(classNames({}).to.be.false }) it("should work with a single class", () => { expect(classNames({a:true})).to.equal("a") }) it("should work with booleans", () => { expect(classNames({a:true, b: false}).to.equal("a b") }) it("should work with falsy and truthy values", () => { expect(classNames({a: 5, b: ""}).to.equal("a b") }) This isn't redux. It's a trivial utility function. Your kind of thinking is too often applied without thinking. Before depending on someone's code, ask yourself if you can roll your own in under 5 minutes. The answer could easily be yes. There is no good reason to depend on someone else's code if it's trivial. Having more dependencies than you need is a cardinal sin that way too many javascript developers commit.
  - rckclmbr 8y ago
    it("should work with booleans", () => { expect(classNames({a:true, b: false}).to.equal("a b") }) Shouldnt this just equal "a" though? See, already a bug in your homegrown solution!
    - anonytrary 8y ago
      My test would fail, and I would fix it. + 2 seconds. Actually, the next test has the same problem. 2 more seconds. I'm gonna be in trouble with my boss!
      - littlecranky67 8y ago
        No, your test wouldn't fail because you wrote your code already being confident it is correct and went on. The bug would surface later in your team, somebody else in your team not familiar with your code would spent hours figuring out what is wrong, will consult you about your intentions when you wrote the code etc.
        anonytrary 8y ago
        No, the test would fail in this case because the code was written properly because it was a TRIVIAL 2-line function (see above). A few people in this thread have regurgitated the same reasoning you have. The problem is that your reasoning is NOT an invariant under complexity. This reasoning does not apply nearly as much to trivial utility functions such as: const isObj = o => !!o && typeof o === "object" and classNames, etc. as it does to larger, more complex code. > will consult you about your intentions when you wrote the code etc. No, no they won't. No one will consult me on what my 2-line classNames functions does. It's 2 non-obfuscated lines. These aren't undergrads in CS101.
  - jhall1468 8y ago
    If you write 100 ~50 line "trivial functions" you've now written 5000 lines of code that doesn't actually solve your underlying problem, it just makes it slightly easier. And you can't write this particular utility function in 5 minutes and support all the uses-cases, particularly if you are going to write a bunch of unit tests. The reason to depend on someone else's code in trivial use-cases is entirely because they are trivial. There's nothing you're doing aside from wasting time by writing it. lodash is literally a massive library of mostly "trival" functions that no one is every going to write themselves because WHY waste the time? The only cardinal sin at play here is thinking that wasting your time reinventing the wheel is time well spent. This approach is what I'd expect from either a very young developer or a very self-centered one.
city41 8y ago
My usage is typically const cmpClass = classnames('componentsMainClass', props.className, { dynamicClassesHere }); I like that the component's main class stands out and is easy to find, that allowing the consumer to pass along their own class is trivial, and that the dynamic classes are grouped together. So I'd argue multi argument and falsy thinning are welcome features. I've never used array flattening. Does it really need to be a dependency? I guess that's a matter of opinion. I like that the various React projects I deal with on a regular basis all handle classes in a consistent manner. I also don't usually mind dependencies that don't pull the entire world in with them.
- anonytrary 8y ago
  > const cmpClass = classnames('componentsMainClass', props.className, { dynamicClassesHere }); This is a matter of convention. Another, equally valid convention: classNames({ mainClass: true, // first key is main class [props.className]: true, // derived otherClass: true, // rest yetAnotherClass: true // ... }) At the end of the day, everyone has to learn their codebase's convention, and that's all that really matters. One convention isn't better or worse than any other, but it's best to have one and stick with it. I don't need a library which supports 10 different conventions if my chosen convention only requires 2 lines of code. Your convention could be implemented in a few lines of code, as well, and then you'd be forced to stick with it, which is probably better. Using this library, 5 different developers could be using 5 different conventions. The helper function I suggest forces a single convention, which lowers complexity.
  - city41 8y ago
    Readability is important, your example above would need to be classNames({ mainClass: true, [props.className]: props.className, otherClass: somePredicate }) The props.className one in particular is pretty garish. Probably should do !!props.className to help explain intent a bit, which adds to the tedium. Once you've added falsy thinning and arbitrary arguments, you're 99% of the way to classnames.
matb33 8y ago
We’ve been doing: const classNames = [ “btn”, this.props.isSelected && “btn-selected” ].filter(Boolean).join(“ “)

paperpunk 8y ago

Roll your own, untested function vs a small, widely used library with a well-understood, well-tested API that your devs have a good chance of being familiar with already. I too want to avoid bloated npm dependencies but this seems like a fair engineering trade-off..

anonytrary 8y ago
This is a trivial function, your argument applies to complex utilities. It is piss easy to roll your own, and it's not worth introducing a potential attack vector via a new dependency. In this case, if your devs can't keep up with your classNames function, fire them, because it's trivial. it("should work with no classes", () => { expect(classNames({}).to.be.false }) it("should work with a single class", () => { expect(classNames({a:true})).to.equal("a") }) it("should work with booleans", () => { expect(classNames({a:true, b: false}).to.equal("a b") }) it("should work with falsy and truthy values", () => { expect(classNames({a: 5, b: ""}).to.equal("a b") }) This isn't redux. It's a trivial utility function. Your kind of thinking is too often applied without thinking. Before depending on someone's code, ask yourself if you can roll your own in under 5 minutes. The answer could easily be yes. There is no good reason to depend on someone else's code if it's trivial. Having more dependencies than you need is a cardinal sin that way too many javascript developers commit.
- rckclmbr 8y ago
  it("should work with booleans", () => { expect(classNames({a:true, b: false}).to.equal("a b") }) Shouldnt this just equal "a" though? See, already a bug in your homegrown solution!
  - anonytrary 8y ago
    My test would fail, and I would fix it. + 2 seconds. Actually, the next test has the same problem. 2 more seconds. I'm gonna be in trouble with my boss!
    - littlecranky67 8y ago
      No, your test wouldn't fail because you wrote your code already being confident it is correct and went on. The bug would surface later in your team, somebody else in your team not familiar with your code would spent hours figuring out what is wrong, will consult you about your intentions when you wrote the code etc.
      - anonytrary 8y ago
        No, the test would fail in this case because the code was written properly because it was a TRIVIAL 2-line function (see above). A few people in this thread have regurgitated the same reasoning you have. The problem is that your reasoning is NOT an invariant under complexity. This reasoning does not apply nearly as much to trivial utility functions such as: const isObj = o => !!o && typeof o === "object" and classNames, etc. as it does to larger, more complex code. > will consult you about your intentions when you wrote the code etc. No, no they won't. No one will consult me on what my 2-line classNames functions does. It's 2 non-obfuscated lines. These aren't undergrads in CS101.
- jhall1468 8y ago
  If you write 100 ~50 line "trivial functions" you've now written 5000 lines of code that doesn't actually solve your underlying problem, it just makes it slightly easier. And you can't write this particular utility function in 5 minutes and support all the uses-cases, particularly if you are going to write a bunch of unit tests. The reason to depend on someone else's code in trivial use-cases is entirely because they are trivial. There's nothing you're doing aside from wasting time by writing it. lodash is literally a massive library of mostly "trival" functions that no one is every going to write themselves because WHY waste the time? The only cardinal sin at play here is thinking that wasting your time reinventing the wheel is time well spent. This approach is what I'd expect from either a very young developer or a very self-centered one.

city41 8y ago

My usage is typically const cmpClass = classnames('componentsMainClass', props.className, { dynamicClassesHere }); I like that the component's main class stands out and is easy to find, that allowing the consumer to pass along their own class is trivial, and that the dynamic classes are grouped together. So I'd argue multi argument and falsy thinning are welcome features. I've never used array flattening. Does it really need to be a dependency? I guess that's a matter of opinion. I like that the various React projects I deal with on a regular basis all handle classes in a consistent manner. I also don't usually mind dependencies that don't pull the entire world in with them.

anonytrary 8y ago
> const cmpClass = classnames('componentsMainClass', props.className, { dynamicClassesHere }); This is a matter of convention. Another, equally valid convention: classNames({ mainClass: true, // first key is main class [props.className]: true, // derived otherClass: true, // rest yetAnotherClass: true // ... }) At the end of the day, everyone has to learn their codebase's convention, and that's all that really matters. One convention isn't better or worse than any other, but it's best to have one and stick with it. I don't need a library which supports 10 different conventions if my chosen convention only requires 2 lines of code. Your convention could be implemented in a few lines of code, as well, and then you'd be forced to stick with it, which is probably better. Using this library, 5 different developers could be using 5 different conventions. The helper function I suggest forces a single convention, which lowers complexity.
- city41 8y ago
  Readability is important, your example above would need to be classNames({ mainClass: true, [props.className]: props.className, otherClass: somePredicate }) The props.className one in particular is pretty garish. Probably should do !!props.className to help explain intent a bit, which adds to the tedium. Once you've added falsy thinning and arbitrary arguments, you're 99% of the way to classnames.

matb33 8y ago

We’ve been doing: const classNames = [ “btn”, this.props.isSelected && “btn-selected” ].filter(Boolean).join(“ “)