[question] Why custom Emotion shouldForwardProps?

[ivanbanov]

I was wondering why it is needed to manually set the shouldForwardProps for the box elements 🤔 https://github.com/gregberge/xstyled/blob/main/packages/emotion/src/createStyled.ts#L63

Wouldnt emotion handle the props that should be sent to the next component by itself? I'm probably not seeing some of the issues of skipping it

[agriffis]

Emotion doesn't know anything about system props, and it only filters non-HTML attrs when the wrapped component is directly an HTML tag.

const H1 = props => <h1 {...props} />

const StyledH1 = styled(H1)(system)

<StyledH1 backgroundColor="blue" />

With that example, you'd get the expected background color setting, but you'd also get backgroundColor in the HTML.

For some more convoluted history, along with some explanation of missing functionality in emotion, see https://github.com/gregberge/xstyled/pull/231

[ivanbanov]

But the styles are going to the DOM right now 🤷🏻‍♂️

If I use the "normal" styled.<tag> and add the system to it everything works as expected because it does not modify the shouldForwardProps

For instance, the box tags use the custom-styled generator which handles shouldForwardProps, I see that the problem is real for components built by the styled function (styled(<component>), but the box ones shouldn't have the same behavior right?

I force the box options to use the "normal" styled.<tag>+system and it seems to be working as expected.

https://codesandbox.io/s/gifted-germain-23xjb?file=/src/App.js

Let me know if it worth a PR

[agriffis]

The problem is that Emotion makes it impossible to do both:

1. Avoid forwarding system props to non-primitive components
2. Avoid forwarding non-HTML props to primitive components

The way that Emotion implements shouldForwardProp makes it impossible to do (1) without sacrificing (2). The details of this are in #231

This was fixed in styled-components in https://github.com/styled-components/styled-components/pull/3436 but there is no equivalent for Emotion yet.

In xstyled we are responsible for system props, so we choose to set shouldForwardProp to catch the props for which we're responsible. This is a compromise but we don't have better options.

I force the box options to use the "normal" styled.<tag>+system and it seems to be working as expected.

In that case you've chosen case (2). I think for trivial applications it probably works out better, but eventually as things get more complex you run into system props leaking from styled wrappers to the components they wrap.

It would be great to get this fixed in Emotion upstream similar to the fix in styled-components.