All pure components should accept all DOM props

[wyuenho]

cf-ui has a ton of pure components that only do 2 things:

1. Apply BEM classes
2. Prevents outside code from directly setting any event handlers and props on the contained React DOM element that's not whitelisted by the pure components themselves.

2. is preventing us from setting event handlers on form elements and sometimes completely valid HTML 5 properties. All pure components should allow all of the attributes and event handlers accepted by the wrapped elements themselves when possible. Special handling for some attributes and event handlers might be needed, but they should be dealt with on a component-by-component basis and should not hinder this issue.

[koddsson]

Oh this has been open for a while and nobody has any comments yet! Let me take take a stab at it :)

What's the downside of doing something like:

<input {...props} />

in our components?

I can see the benefit being for example we don't have to make a PR to cf-ui to make sure that we can disable a toggle which would speed up our process.

[wyuenho]

Sure. Let's ID a list of components we want to do this first. As it stands, a lot of the typographic components are not useful at all and I don't see the value of working on them further. There may be others that aren't useful.

[wyuenho]

Also, we might not want all props to be expanded, just those we know are accepted by HTML5.

[koddsson]

Ok, we could keep a list maybe of allowed props? What would be the downside of expanding all props?

[wyuenho]

I have no good reason to continue with a whitelist approach so far, just something we need to keep in mind. Best go thru all the components now and see if there are idioms in them that have a high probability in resulting accidental overwrites of attributes.

[jwineman]

The only downside I can think of is propTypes don't warn when you pass in extra props. I can't think of any scenarios where this would actually affect us negatively though.

[wyuenho]

This is a living ticket as PRs that allow arbitrary props to be passed into the cf-ui components are opened and merged, until all components are fixed.

[koddsson]

@wyuenho In order for this to be actively worked on (and maybe one day finished) could you list up the places where we are not passing the props through so we have a idea of how much is left of this ticket?

Maybe it even goes hand-in-hand with the fela migration since createComponent accepts a passThroughProps parameter?

[tajo]

It actually goes against fela migration since createComponent needs a white list of props that should pass through (passThroughProps ). Although, it's probably going to change since it's not a good approach.

I like props validation a lot because it gives us some clear contract and you can see it in docs. If there's something missing just open a PR. Eventually, we will cover everything. It's not that hard.

[wyuenho]

I'm going to close this one. There is one and only one practical reason that convinced me passing thru arbitrary props is a bad idea when combined with Fela, which is enough to challenge my initial rationale.

When implementing selector heavy styles, where some style props have to propagate from multiple levels down, the component at each level will have to know both what will be passed down implicitly, and what to pass down explicitly. If we allow arbitrary prop spreading, undeclared extra props may be passed all the way down from great grand parent to child unexpectedly. This makes debugging really hard. If the child happen to be leave, we may get React warnings too.