skipSx should update shouldForwardProps

[realamirhe]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Summary 💡

The skipSx doesn't work as expected when shouldForwardProp default mechanism is overwritten and does not include 'sx' directly

shouldForwardProp: (prop) => !["isEven"].includes(prop as string),
skipSx: true

This will send sx to the underlying component

Examples 🌈

I made a code sandbox using the latest released material

https://codesandbox.io/s/goofy-phoebe-xc99x

Motivation 🔦

making a custom component that only changes in CSS layer duo to props is a common practice
also, sx seems to be a little bit slow, and skipping it may gain some performance tips

so because we are working mostly with the Mui component and they pass the props to the underling dom node, it makes sense to have it as a core feature of the material

---

I seared on skipSx tags in the issue list and could not find an open issue, so posted a new one.

[michaldudak]

@mnajdova correct me if I'm wrong, but I think it works as it's supposed to. skipSx does not evaluate the sx prop in a component but does not control whether it's passed to the underlying components.
I can imagine a use case where you don't want to use the sx implementation provided by styled but still propagate the prop to the underlying component.

If you don't want to ignore the sx completely, set skipSx to true and add sx to the list of excluded props in shouldForwardProp.

[realamirhe]

Hi @michaldudak
My understanding was similarly the same, but there is an issue with this mindset

  shouldForwardProp: (prop) => !["isEven"].includes(prop as string),
  skipSx: true

this should not apply the sx but it does!
could you take a look at an example https://codesandbox.io/s/goofy-phoebe-xc99x

[michaldudak]

This is expected. In this example the sx prop is not handled by the styled wrapper, but by Typography itself. Try wrapping a host component, such as <p> in styled (styled('p', {) instead of Typography to see that the sx prop has no effect if skipSx is set to true.

[realamirhe]

That's okay, I know it prevents the sx to go deeper from the created component (styled one)
But I believe differences happen to exist because of our expectations of shouldForwardProp and skipSx priority, for me skipSx is more prior to shouldForwardProps, and vice versa

Anyway, I post it as a feature request, not a bug, and the discussion was exactly what I was looking for.

[mnajdova]

After reading trough it I think it makes sense for the skipSx prop to determine whether to forward the sx prop or not. I think we should combine this logic with the default shouldForwardProp. But, if a custom shouldForwardProp function is specified, it should be a responsibility of the developer to decide on it. I am not sure if this is very intuitive.

[michaldudak]

It would be a breaking change, though, so we could think about it for the v6 release.