classnames
classnames copied to clipboard
JedWatson
Remove support for numbers
Numbers are currently allowed, but they yield unexpected results
> const classNames = require('classnames')
> classNames(0) // Falsy values are always filtered
''
> classNames(42) // Stringified integers aren’t valid class names
'42'
> classNames(3.14) // Stringified floats aren’t valid class names
'3.14'
> classNames(Infinity) // This is the only number which yields a valid class name
'Infinity'
> classNames(NaN) // Falsy values are always filtered
''
This may not be much of an issue, but the readme explicitly states this package is for joining class names. Numeric values aren’t valid class names, but they’re explicitly supported in the implementation.
According to the type definitions, numbers are supported.
Since breaking changes are on the way, I think it makes sense to remove support for numbers.
Following the history, numbers are explicitly supported in the code since https://github.com/JedWatson/classnames/pull/8.
I concur they don't make sense, maybe they could be useful in the bind variation?
Removing them now would be a breaking change, but, +1 for the future.
Maybe I'm unaware of the potential use-case.
Stringified integers aren’t valid class names, Stringified floats aren’t valid class names
The CSS Spec says you can't start a class name with a number. HOWEVER you can start it with a backslash-escaped identifier. So .2 {} is invalid, but .\32 {} - the character sequence for '2' is valid.
You need a bit of trickery to define the class in css but the html class="2" is totally valid and references a valid css selector. See this codepen for some demos.
Passing numbers into classNames feels strange, and personally I'd never do it but if supporting them isn't any more complex than a toString() call then it feels like it'd be worth keeping support.
HOWEVER you can start it with a backslash-escaped identifier. So
.2 {}is invalid, but.\32 {}- the character sequence for '2' is valid.
That's weird, but it makes sense to keep support for this then.
If something requires trickery to do it isn't a recommended pattern. While I don't think this particular Github issue is the biggest deal in the world, I do not follow the logic of this conversation. So, our working assumption/premise is: we support all edge cases including those that can be described as "trickery"? To me, this is why we should not do something.
Again, not a big deal! Just saying I'm lost.
I opened this issue earlier today: https://github.com/JedWatson/classnames/issues/256
Removing number now is a breaking change, but, I don't think we'd risk the problem of being too opinionated by removing type number from the acceptable types in the next major version.
These edge cases are still trivially within reach by passing the string "2" without concern. Maybe it's still a worthwhile discussion for next?
If something requires trickery to do it isn't a recommended pattern.
I agree it's not recommended but it is valid and currently supported so its removal is a breaking change as @dcousens notes. My comment was noting that the claim "a number such as '42' or '3.14' is not a valid class name" is false. My working assumption is not "we should support all edge cases and must continue to do so" but "classnames currently support these edge cases and its maintainers need to consider if it worth making breaking changes to remove support".
I agree that for the sake of simple APIs number should not be a valid type and that classnames(2) should be unsupported and a type error and that users should write classnames('2') instead as part of a v3 where breaking changes are allowed.
I would also be tempted to remove boolean from the valid values too - and thus only allowing strings / undefined / null as a breaking change
Adding this issue to the potential 3.0 release that might be coming up, which will allow us to introduce some breaking changes. Removing support for numbers will also allow us to reduce the amount of logic, as we no longer need to check it explicitly.
I think we should consider simplifying the API even more, dropping support for everything but strings and objects, even going as far as to remove support for deeply nested objects. But I will save that as a topic of discussion for a later time.
