className may be a function #518
Conversation
hola :) we use [bem-cn] (https://github.com/albburtsev/bem-cn) that returns function which react casts into string while rendering, but your framework doesn't consider it. So let's cast to string!
| @@ -27,6 +27,9 @@ export function childrenOfNode(node) { | |||
|
|
|||
| export function hasClassName(node, className) { | |||
| let classes = propsOfNode(node).className || ''; | |||
| if (typeof classes !== 'string') { | |||
| classes = classes.toString(); | |||
| } | |||
There was a problem hiding this comment.
Hey! Thanks for the PR! couple of things here:
- I'm not familar with
bem-cn, can you explain a bit about how it works? From my understanding, React expectsclassNameto be a string at render time. How doesbem-cnproperly pass a function to that? - Your identation is off.
- We need unit tests or there is a good chance obscure code like this could be regressed.
You can hold off on executing 2 & 3 until we determine the necessity of this PR if you want. Thanks for starting the conversation around this!
There was a problem hiding this comment.
For reference: https://github.com/albburtsev/bem-cn#proptypes-warnings
I'm going to wait for other maintainers to respond. I personally feel like option #2 can solve this problem in user-land and this PR is unnecassary. Is there any reason you couldn't just call toString() on your side? instead of expecting enzyme to do it?
There was a problem hiding this comment.
If React stringifies it without a warning, then I'm ok with us doing it too (using String(), never .toString). Otherwise, yes, this is the explicit responsibility of the user (i.e. option 2)
There was a problem hiding this comment.
I agree with @ljharb that we should support whatever react supports, so if react supports this without any warnings we should too. we do however have to confirm that this is indeed supported by react, in all 3 versions that we support
bem-cn is a helper for building BEM names using carrying. it returns a function, that have own toString method, that return a string containing sequence of BEM css classes. |
|
Given that, we should definitely be coercing it to a string. However, it must use |
|
Ok. Shall we edit our PR or you will do you own with tests? |
|
@f0rmat1k if you're willing to write the tests, it'd be great if you could edit the PR :-) |
|
Looks like we can close this in favor of #519 |
hola :)
we use bem-cn that returns function which react casts into string while rendering, but your framework doesn't consider it. So let's cast to string!