React 16: className with bem-cn like libraries

[ekfn]

Hello! Really love new features and architecture of Fiber.

But because of this new behavior:

Non-event attributes with function values:
React 16: Warns and ignores them.

I've got this issue:

const fn = () => null;
fn.toString = () => 'foo';

const render = () => (
  <div className={fn}>doesn't add class</div>
);

It wouldn't add className, even if i've override toString method, but with object it works:

const obj = { toString: () => 'foo' };

const render = () => (
  <div className={obj}>works</div>
);

I think it's OK to have same behavior with function and object.
This feature is needed in "bem-cn" like library, so I can write just fn, instead of fn() or fn.toString()

[aweary]

@ekfn can you expand on why you want to use a function with a custom toString method instead of just an object? We want to make sure it's possible to implement dynamic toString behavior, but IMO it makes sense to use an object here instead of defining a null returning function.

[ekfn]

@aweary
apparently it wasn't best idea to simplify example by null returning function :)

Using function in className is very common if you want to build class name dynamically, for example in bem-cn:

import block from 'bem-cn'

const b = block('MyBlock');

const render = () => (
  <div className={b}>
    <div className={b('title')}>My title</div>
    <div className={b('content')}>My content</div>
  </div>
);

// className={b} would return 'MyBlock'
// className={b('title')} would return 'MyBlock__title'
// etc

So you can use className={b('title')} for inner elements and just className={b} for base block, instead of className={b()} or className={b.toString()}, which is really nice.

[nhunzaker]

This makes sense. I have no idea what the edge cases are if we were to allow functions with custom toString methods. Maybe this could be done with className only.

I'm not really sure what the best call here is, but it would be great to identify if we want to continue to support this use case.

[ekfn]

Yeah, for className attribute I think it should works this way: convert value to string or die trying.

For custom attributes though - not sure. I'm curious what so special about objects, that we try to convert it to string, but boolean values would be just ignored. And how react understands which attributes is Non-event, what if browser-specific non-standard attributes uses functions?

[hrkv]

bem-cn wrapper to return strings instead functions https://github.com/mistakster/bem-cn-lite

[ekfn]

@hrkv
It's not a problem to return string (or object, which is work) instead of function, it's about do we support this feature or not.

[gaearon]

Why not just write className={b()}? Seems like more explicit and it's easier to understand what it does.

[0xAVX]

this tool https://github.com/alfa-laboratory/cn-decorator
is what you need

[ekfn]

@gaearon Actually, with bem-cn inner elements also need final call without arguments, cause it returns function after each call (chaining). I use my own library, which returns object after first call (sorry for misunderstanding).

import block from 'bem-cn'

const b = block('MyBlock');

const render = () => (
  <div className={b()}>
    <div className={b('title')()}>My title</div>
  </div>
);

@seriouslyfluffy doesn't it use function as well?

[ekfn]

But my question, in general, is why object with toString is valid value and function doesn't.

[ekfn]

So, what we have:

1. We can't use chainable interface with any attributes (like this b('icon')({name: 'check'}));
2. Anyone, who use bem-cn (or similar) library, would need a lot of refactoring to update to react v16;
3. I can't think of any disadvantages of calling toString on function, instead of just ignoring it.

@gaearon @aweary

[gaearon]

Even if we called the function, we would still want to print a warning. Because most often passing a function happens due to a mistake, not deliberate (e.g. if you forget to call it). Silently ignoring it is worse because it usually means a bug. Since we warn, libraries shouldn't rely on it anyway, as we encourage people to treat warnings as errors. So that's why passing it through doesn't look like a solution to me.

I don't quite understand the advantages of the chainable interface you're suggesting over simpler interface that accepts a variable number of arguments and always returns a string. I think that this is always possible. While React 16 behavior might force an API change in that library, I don't see any use cases that would work before but be impossible to implement now. So I think we can close this as for the vast majority of use cases the new behavior is better (it catches more bugs).

Anyone, who use bem-cn (or similar) library, would need a lot of refactoring to update to react v16;

We released an RC of React 16 with this behavior a few weeks ago, and publicly asked people to test it. Now that 16 is out, it is too late to change the behavior anyway, and this library is the first use case relying on custom fn.toString() that we've seen so it's pretty exotic. But if it's a big problem I encourage you to build a codemod to a new API so you won't have to migrate it by hand. That's how we change APIs at Facebook. I'm sorry if this isn't very helpful, but I hope you can see that this is a very unusual chaining mechanism, and is possible to expose in a different and hopefully more ergonomic way.

[ekfn]

Since we warn, libraries shouldn't rely on it anyway

Yeah, in this case, this is pretty same as ignoring (silently or not).

I don't quite understand the advantages of the chainable interface you're suggesting over simpler interface

Do i really need to explain why different interfaces are good and fun in deffirernt cases?

But if it's a big problem I encourage you to build a codemod to a new API

Thanks for suggestion, that's a good practice, but I don't use bem-cn by myself, so it's not a problem for me.

Personally, in this case, I still would prefer not changing API over it catches more bugs, but thanks anyway :)