[material-ui][docs] Fade (and other transition helpers) fails silently when children aren't able to receive the style prop

[emattias]

Expected Behavior 🤔

That the Fade component complains if it gets children that doesnt take the style prop.

Current Behavior 😯

If you give Fade an element that doesnt take and apply the style prop the Fade component does nothing.

See this codesandbox: https://codesandbox.io/s/n3z26zp6n0 There you can see the the component that returns a React.Fragment the root never fades but the one with a div does.

Here are some suggestions/thoughts/options:

• Validate that the element that it receives takes the style prop (via typescript types and/or prop-types)
• Receive a Component prop and render that instead of receiving an element and doing cloneElement. I think there is some problems typing react elements, see this
• Output its own element (default to div and make it overridable?) to ensure that the style prop is applied
• Take a render function that receives the style and have the user apply the style props (and other logic depending on the transition state it gets in its own callback function).

[eps1lon]

Validate that the element that it receives takes the style prop (via typescript types and/or prop-types)

1. TypeScript can't verify children
2. prop-types cannot infer if a child handles certain props

Output its own element (default to div and make it overridable?) to ensure that the style prop is applied

Sounds tempting but we try to avoid a div soup. I think this makes this even harder in certain layouts

Take a render function that receives the style and have the user apply the style props (and other logic depending on the transition state it gets in its own callback function).

That doesn't solve the issue. The user can still drop the passed styles.

I'm not sure we should add warnings here if we don't see the styles. We cannot know where/how the child treats the style. It might even actually want to drop them.

We can improve the docs and explicitly document what props we pass to the child and expect it to handle. Adding warnings doesn't seem viable.

[emattias]

Output its own element (default to div and make it overridable?) to ensure that the style prop is applied

Sounds tempting but we try to avoid a div soup. I think this makes this even harder in certain layouts

I think it can make sense, since the functionality depends on style being applied. I guess this would work similarly to how Typography component works with the component prop. You can send in a fragment yourself as children to avoid another div.

Take a render function that receives the style and have the user apply the style props (and other logic depending on the transition state it gets in its own callback function).

That doesn't solve the issue. The user can still drop the passed styles.

True although I think its less of a risk since you can type the function to take the styles. We got bit by this bug because of a refactoring that changed the root element to a fragment and suddenly the fades stopped working. If we had a render prop that takes a style prop I think we would have discovered this regression when refactoring.

I'm not sure we should add warnings here if we don't see the styles. We cannot know where/how the child treats the style. It might even actually want to drop them.

I cant think of a use case for using the Fade component and not applying the style prop. Maybe there is one that I have missed?

We can improve the docs and explicitly document what props we pass to the child and expect it to handle. Adding warnings doesn't seem viable.

I hope it can be enforced programatically and/or with api design aswell.

[eps1lon]

True although I think its less of a risk since you can type the function to take the styles. We got bit by this bug because of a refactoring that changed the root element to a fragment and suddenly the fades stopped working

This is true for every refactoring in react where parent-child are tightly coupled. Maybe TypeScript will address some of the JSX issues but I don't see how we can check this at runtime.

I cant think of a use case for using the Fade component and not applying the style prop. Maybe there is one that I have missed?

It's not about the if but about the when. And then we have to solve how we verify if the style is actually the style we want i.e. how to determine if some CSSDeclaration is a subset of another and if we should warn in every case.

This would definitely improve DX but it's a very niche case. If you can provide a concrete implementation I'm happy to review it.

[emattias]

True although I think its less of a risk since you can type the function to take the styles. We got bit by this bug because of a refactoring that changed the root element to a fragment and suddenly the fades stopped working

This is true for every refactoring in react where parent-child are tightly coupled.

That is why I want the requirments that Fade has on its children to be enforced :)

Maybe TypeScript will address some of the JSX issues but I don't see how we can check this at runtime.

Would be nice. This issue might help this: microsoft/TypeScript#21699

I cant think of a use case for using the Fade component and not applying the style prop. Maybe there is one that I have missed?

It's not about the if but about the when. And then we have to solve how we verify if the style is actually the style we want i.e. how to determine if some CSSDeclaration is a subset of another and if we should warn in every case.

This would definitely improve DX but it's a very niche case. If you can provide a concrete implementation I'm happy to review it.

What happened with this suggestion, I dont think you adressed it :) :

Receive a Component prop and render that instead of receiving an element and doing cloneElement.

[emattias]

If it receives a component through the Component prop (instead of receiving an element and adding the style prop with cloneElement) you can type prop requirements.