Should Stitches automatically prevent variant props from being forwarded to child component?

[peduarte]

Whilst working on the Modulz design system, I came across this interesting issue.

We have a Separator Component, which extends SeparatorPrimitive via the styled function.

export const Separator = styled(SeparatorPrimitive, {...})

Colm created an orientation variant, to style it accordingly based on whether it’s horizontal or vertical

orientation: {
  horizontal: { height: "1px", width: "50px" },
  vertical: { width: "1px", height: "50px" }
}

Note: our SeparatorPrimitive also accepts an orientation prop with the same values as above.

When using this Component, I was getting the following error:

Warning: Failed prop type: Invalid prop `orientation` of value `undefined` supplied to `Separator`, expected one of:
  - horizontal
  - vertical

Then I realised, in Stitches, variant props are removed and not passed down to the underlying component. This is to avoid forwarding invalid DOM attributes to elements. As a side-effect, it meant that the orientation prop was never reaching the SeparatorPrimitive, which results in the error above.

To work around this error, I’ve dropped the Stitches variants in favour of the data-orientation attribute, added by the Primitives:

'[data-orientation="horizontal"]': { height: "1px", width: "50px" },
'[data-orientation="vertical"]': { width: "1px", height: "50px" }

Not sure I’m fully satisfied with this, because:

• it relies on some implementation detail
• from a user’s perspective, orientation feels like a variant. But the extracted variants (StitchesVariants<typeof Separator>) wont include it

Demo: https://codesandbox.io/s/the-separator-war-2x8t5?file=/src/App.tsx

---

I'd like to open up a discussion about this:

• Should Stitches introduce a way to "opt-out" of removing variant props?
• Should Stitches always forward all props, unless told otherwise?

---

Emotion has a shouldForwardProp function to customise prop forwarding. By default it forwards everything.

[hadihallak]

I think we can control this via config or via a custom opt-out method as you mentioned

Alternitevly, another solution would be to only omit variant props if the styled component wasn't extending other components:

// Here we omit variant props:
const Test = styled.div({variants:  {variant: {red: {}}}})


// Here we  dont omit variant props and omiting variants becomes SomeOtherComponent's  responsibility:
const Test = styled(SomeOtherComponent, {variants:  {variant: {red: {}}}})

[peduarte]

another solution would be to only omit variant props if the styled component wasn't extending other components:

I also thought about this, but not sure it feels a little "random".

I think we can control this via config

what do you have in mind?

[chaance]

'[data-orientation="horizontal"]': { height: "1px", width: "50px" },
'[data-orientation="vertical"]': { width: "1px", height: "50px" }

Not sure I’m fully satisfied with this, because:

• it relies on some implementation detail

Just a point of clarity, this isn’t actually an implementation detail but a part of the public API. data- attributes are generally there so that users can target elements in stylesheets. That’s why we use them instead of relying on aria- attributes. ARIA is definitely an implementation detail and may change over time. data- can be whatever we want.

from a user’s perspective, orientation feels like a variant. But the extracted variants (StitchesVariants<typeof Separator>) wont include it

Definitely agree with this. I feel like Emotion struggled with this for years and never came up with a really great solution. shouldForwardProp feels like a hack to me, but I get that it's probably the best they could do.

@hadihallak I kind of like the idea of handling this via config. I think this issue will pop up from time to time but I can't imagine it being so frequent that the config gets out of hand.

[Kinbaum]

Hi @peduarte, first of all thank you to you and your for this amazing library.

I found this discussion while looking for a solution as to why my props are not being preserved in my component when the prop is also a variant. Looks like they're being stripped as part of sanitizing the html output, removing potentially invalid attributes. And that's really the keyword here, potentially.

I would definitely like to see at least one of the following where we can say:

• forwardVariantProps - this would be part of the styled configuration and work on a component by component basis
• automatically preserve the prop if using a boolean variant like disabled
• pick and choose on a variant by variant basis which prop gets forwarded by setting forwardVariant: true

I have found that on several occasions I want to control things besides the look and feel of my component based on a given variant. For example, I have a TextField which is comprised of a div containing labels, inputs, and tooltips. I may want to apply certain CSS to the component when it's disabled, but I also want to set the disabled attribute on the input using disabled={ props.disabled }, or evaluate props.disabled to do anything else I may need to.

The only way around this it seems is to create a component that wraps a styled component. But this is a pattern that I am not particularly a fan of and I see it as being more of a hassle.

There hasn't been much on this discussion in a while. Any thoughts to the options above?