RFC filtering forwarded props

[lcswillems]

I am fan of styled-components, but there is a big pain point for me: filtering forwarded props (that pushed me to consider other librairies like emotion).

For example, if I want a div to have a controlable height and width, I will have to do this in order to ensure my div doesn't end up with height and width attributes:

const StyledDiv = styled(({height, width, ...props}) => <div {...props} />)`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`

This is painful to write and makes the code unreadable. It seems to be a big issue with the library if we look at the number of issues opened for this (#135, #2467, #411, #439), the number of articles/spectrum questions about it, the aborted attempt of transient prop with v5 (#2093), etc...

Hence, I would like to propose a new solution, that I have already implemented and works well for me.

Preliminary remark

A lot of people use styled components this way (because it is the only way described by the doc):

const StyledDiv = styled.div`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`

But this quickly leads to long properties that are hard to understand. I usually use it this way, which can also fully benefit from props destructuring:

const StyledDiv = styled.div(({ width, height }) => `
    width: ${width}px;
    height: ${height}px;
`)

Remark: These codes doesn't handle the issue with props forwarding.

The base solution

Currently, styled.div takes a function that transforms props into css. The idea is to rather take a function that transform props into props, where the css prop will represent the styling:

const StyledDiv = styled.div(({ width, height, ...props }) => ({
    css: `
        width: ${width}px;
        height: ${height}px;
    `,
    ...props
})

You can notice that this code handles the issue with props forwarding! width and height will not be forwarded to the div. If you want to forward width but not height, you can do:

const StyledDiv = styled.div(({ width, height, ...props }) => ({
    css: `
        width: ${width}px;
        height: ${height}px;
    `,
    width,
    ...props
})

If you want to forward all the props, you can just use the current solution:

const StyledDiv = styled.div(({ width, height }) => `
    width: ${width}px;
    height: ${height}px;
`)

etc...

Variants

The solution I have just proposed illustrate perfectly the design shift. However, it may not be a good solution in practice because painful to use. Hence, it may be a good idea to also implement several alternatives for convenience:

Variant 1 (css string and other props)

Instead of returning the props, a better solution could be to return an array containing the css and then the other props we want to forward. For example, if we only want to forward width, it would be:

const StyledDiv = styled.div(({ width, height, ...props }) => [`
    width: ${width}px;
    height: ${height}px;
`, {width, ...props}])

Variant 2 (tagged template litteral)

The previous solutions don't use tagged template litteral, what styled-components heavily use. Alternative 1 can be adapted to use them:

const StyledDiv = styled.div([`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`, ({ width, height, ...props }) => props ])

But, now, the [ ] doesn't seem useful anymore. This leads to third and last variant:

Variant 3 (simplified tagged template litteral)

const StyledDiv = styled.div(`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`, ({ width, height, ...props }) => props)

To sum up

The current solution is:

const StyledDiv = styled(({height, width, ...props}) => <div {...props} />)`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`

which is painful to write and unreadable (what kind of component is it?).

The solution I propose is to add a second argument for filtering props:

const StyledDiv = styled.div(`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`, ({width, height, ...props}) => props);

It is easy to write and to understand, and props filtering (which is not the important part of the definition) is sent at the end of the definition.

For people currently writing components like this:

const StyledDiv = styled.div(({ width, height }) => `
    width: ${width}px;
    height: ${height}px;
`)

and wanting to filter props, the solution would be:

const StyledDiv = styled.div(({ width, height, ...props }) => [`
    width: ${width}px;
    height: ${height}px;
`, {width, ...props}])

[diondiondion]

This is a great proposal, and so needed. 👏

Personally I'm not a big fan of the array notation in your very last example, and would instead keep things consistent between usage styles (template literal vs. function returning one). I.e., like this:

const StyledDiv = styled.div(({width, height}) => `
    width: ${width}px;
    height: ${height}px;
`, ({width, height, ...props}) => props))

It would mean you'd have to destructure twice, but unless there was a performance penalty, that feels like a cleaner solution to me. Especially if you don't want to destructure initially:

const StyledDiv = styled.div(p => `
    width: ${p.width}px;
    height: ${p.height}px;
`, ({width, height, ...props}) => props))

---

Something else I'm wondering is how ref and as would interact with filtering in the proposed API?

One big reason the current solution (styled(({height, width, ...props}) => <div {...props} />)) is so unergonomic is that it breaks completely when using either refs or the as prop, so I'm curious about how you imagine these cases to be handled. I imagine that ref will work transparently since no JSX calls are involved, so that's good. However, we still need a way to indicate that we want the as prop to be forwarded to the wrapped component (statically, without having to use the forwardedAs prop).
Can you think of a good way to integrate such an API with your proposal? I don't mean to derail your proposal, but I think it's worth thinking about these cases as they're quite linked in how they both deal with how props are forwarded (or not).

The only API I can come up with would be an optional third parameter (which should probably be an object for future proofing).

const StyledDiv = styled(Button)(`
    width: ${props => props.width}px;
    height: ${props => props.height}px;
`, ({width, height, ...props}) => props, {forwardAs: true});

Setting the forwardAs flag would tell styled-components to forward the as prop to the wrapped Button component, instead of replacing it. But maybe you can think of a more elegant/unified approach.

[kachkaev]

The problem with any variant of ({ a, b, ...props}) => props is that this will produce an ESLint error in many projects.

'a' is defined but never used. eslint(no-unused-vars)

or

'a' is defined but never used. eslint(@typescript-eslint/no-unused-vars)

I don't see reasons not to use something like shouldForwardProp in Emotion. One function to rule'em all 🤷‍♂️

It does not have to be an argument of the styled function:

const MyComponent = styled(AnotherComponent)`...`;
MyComponent.shouldForwardProp = prop => !['a', 'b'].includes(prop)

UPD:

1. My solution is probably harder to make it type-safe
2. linting errors can be mitigated by setting { ignoreRestSiblings: true }

---

This lack of a first-class props filter is a big pain for our team as well.

[lcswillems]

@diondiondion You are right. I also prefer:

const StyledDiv = styled.div(p => `
    width: ${p.width}px;
    height: ${p.height}px;
`, ({width, height, ...props}) => props))

@kachkaev The solution of Emotion is not type safe. It is the first issue I faced with their solution. But I didn't know that the one I proposed would raise this linting error. So, it has to be taken into consideration too.

[k15a]

I don't think this will work as styled.div() is not a tagged template literal anymore and therefore we can't process interpolations like ${props => props.width} anymore.

[k15a]

const StyledDiv = styled.div(p => `
    width: ${p.width}px;
    height: ${p.height}px;
`, ({width, height, ...props}) => props))

This would not work as well as you can't pass shared styles created by the css function anymore.

[mxstbr]

Let's be honest, all of those options make the styled API worse developer experience-wise (personal opinion, not speaking on behalf of anybody else).

Also, as @k15a correctly said, the styled.div function needs to be an actual tagged template literal for function interpolations to work.

We hear you that this is a pain point many people experience, but we haven't found a nice way to handle this yet that doesn't have issues of some sort or another 😕

[diondiondion]

Let's be honest, all of those options make the styled API worse developer experience-wise

What these options are competing with is this:

const StyledDiv = styled(({height, width, ...props}) => <div {...props} />)`...

Throw in some forwardRef nesting and aliases for the as prop, and your statement just doesn't hold true anymore.

We hear you

Yet it's been almost three years with no progress, not even an optional API only to be used in the nastiest of edge cases. It's massively disheartening. 🤷‍♂

[mxstbr]

That looks bad, agreed, but a tiny abstraction makes it fine:

import { filter } from 'some-utils';

const MyDesignSystemComponent = styled(filter('div', props => { ...props, width: undefined }))`
  width: ${props => props.width}px;
`;

Or even further to avoid the inline fn:

const divWithoutWidth = filter('div', props => { ...props, width: undefined })

const MyDesignSystemComponent = styled(divWithoutWidth)`
  width: ${props => props.width}px;
`;

You can also create your own API if you don't like the fn, e.g.:

// Filter "width"
const divWithoutWidth = filter('div', { width: false })

const MyDesignSystemComponent = styled(divWithoutWidth)`
  width: ${props => props.width}px;
`;

Or maybe even with currying if you need different DOM elements:

const withoutWidth = filter({ width: false })

const MyDesignSystemComponent = styled(withoutWidth('div'))`
  width: ${props => props.width}px;
`;

All of these options are nicer that the suggested changes to the styled API in this RFC, no? We could even have that as a sub-import of the styled-components package, something like:

import { filter } from 'styled-components/utils';
// or maybe
import { filterProps } from 'styled-components/utils';

We could bikeshed that API and make it really nice and intuitive and I think that would be nicer than the suggested API here.

(again, this is me personally speaking, no clue what the other s-c maintainers think about this)

[lcswillems]

All of these options are nicer that the suggested changes to the styled API in this RFC, no?

If my suggestion could work, I think it would be nicer because props filtering would be sent to the end of the definition. But my suggestion doesn't work.

Anyway, if I opened this RFC, it is not because I wanted my suggestion to be implement, but rather to open a new discussion about a new solution, and to hope converging to a solution for the library.

@mxstbr I like the solution you propose! It makes it more readable.

[mxstbr]

Anyway, if I opened this RFC, it is not because I wanted my suggestion to be implement, but rather to open a new discussion about a new solution, and to hope converging to a solution for the library.

Sorry if I came across as too harsh, thank you for taking the time to write up a RFC! 👍 Even if this specific API does not work, we appreciate any and all input into making the library better.

[diondiondion]

Thanks for the ideas @mxstbr. Like @lcswillems, I'm happy that there's a discussion at all; it's not about any one suggestion to "win".

a tiny abstraction makes it fine

This and similar approaches wouldn't survive usage with the as prop, though, would they? Personally I really think there needs to be something robust that transparently filters props without messing up the usage ergonomics of a "simple" styled component (ie. one that has no custom props).

If my suggestion could work, I think it would be nicer because props filtering would be sent to the end of the definition. But my suggestion doesn't work.

Could it be made to work by attaching the prop filter as a static field?

const StyledDiv = styled.div`
    width: ${p => p.width}px;
    height: ${p => p.height}px;
`
StyledDiv.filterProps = ({width, height, ...props}) => props

It's more verbose, but still very clear and "out of the way" of the component setup.

---

And of course then there'd be emotion's way of adding an optional second parameter to the styled function that can be used to pass an object with options. It's quite ugly IMO, but worth mentioning for the sake of completeness.

const StyledDiv = styled('div', {
   filterProps: ({width, height, ...props}) => props
})`
    width: ${p => p.width}px;
    height: ${p => p.height}px;
`

[jukkahuuskonen]

Hi mxstbr,
Thanks for the filtering HOC suggestion. Since we are using quite a few material ui components with styled-components I had a problem with forwarding refs together with filtering props from DOM.
According to your suggestion I wrote the filterProps-HOC to filter the props and handle forwarding refs baked in. If someone is interested, here is my solution:

import React, { forwardRef, createElement } from 'react';

function filterProps(Component, propsToFilter = []) {
  // eslint-disable-next-line react/display-name
  return forwardRef((props, ref) => {
    const filteredProps = { ref, ...props };
    propsToFilter.forEach(prop => delete filteredProps[prop]);
    if (typeof Component === 'string') {
      return createElement(Component, filteredProps);
    }
    return <Component {...filteredProps} />;
  });
}

export default filterProps;

And usage example:

import styled from 'styled-components';
import PropTypes from 'prop-types';
import MuiButton from '@material-ui/core/Button';
import { VARIANT_NAMES } from './themeConstants';
import filterProps from './filterProps';
import { buttonColorVariant, buttonHover } from './cssFunctions/cssFunctions';

const filter = ['nsVariant', 'overrideColor'];

const Button = styled(filterProps(MuiButton, filter))`
  ${buttonColorVariant}
  margin: 1px;
  padding: 2px 7px;
  & span {
    font-size: smaller;
  }
  ${buttonHover}
`;

Button.propTypes = {
  nsVariant: PropTypes.oneOf(VARIANT_NAMES),
  overrideColor: PropTypes.string,
};

Button.defaultProps = {
  nsVariant: null,
  overrideColor: null,
};

export default Button;

I'm still having a problem with eslint complaining about display-name, if anyone can help with that, I'd apreciate it.
feel free to comment on this.

-Jukka

[karlhorky]

Coming here to offer my 2¢.

The filterProps method suggestion by @diondiondion feels to me like the most clear, least tedious option out of everything proposed above (including all of the options from @mxstbr):

const StyledDiv = styled.div`
    width: ${p.width}px;
    height: ${p.height}px;
`
StyledDiv.filterProps = ({width, height, ...props}) => props

Why? 👇

Most clear

The function is named, and is applied to one component - as opposed to the separate step with intermediary variables (or complex nesting, if you want to do it without intermediary variables) that is required for any of the function-calling approaches.

The code is easier and faster to visually and mentally parse than any other of the options (this being said as a professed "functional programming" person).

Less tedious

It's built in to styled-components. No need to import anything else, just write an extra method. DX ♥️

This approach with the method is also easier for those not as versed with styled components, as there is less that you need to remember to do.

Simple lib change

styled-components calls the method if it's set, otherwise it's a no-op.

More React-y

It also feels more React-y, aligning nicely with other similar component properties / methods, such as propTypes and Next.js' getInitialProps.

[karlhorky]

One thing that just came to mind is, maybe there's an even better way with static analysis + tooling that nobody has brought up yet:

Using the existing styled-components Babel plugin to automatically do the right thing? 🤔

Not sure if I'm maybe missing a critical part of the discussion, but hear my assumptions out:

• Babel knows which components are styled components
• Babel knows which of the props have been used ("consumed") in the styles
• Babel can write this extra code required to filter these props out

Could this be a solution to the issue?

Caveats:

This would not pass along props for the cases where the author intended them to be passed along (which I would argue is a minority of cases). In this case, I argue having the author do extra work to manually pass along the props is an acceptable tradeoff.

[tommedema]

Just want to ask that whatever solution is chosen (this is indeed a huge problem), keep in mind that it needs to be typesafe too. Currently it is super verbose to go from:

import styled from 'styled-components'

interface INavProps {
  height: number
  renderBubbleSelect: boolean,
  renderSavingLabel: boolean,
  renderShareButtons: boolean,
  renderInstallButton: boolean,
  renderDeactivateButton: boolean,
}

const Nav = styled.nav<INavProps>`
  height: ${props => props.height}px;
  // ....
`

to (typesafe):

import styled from 'styled-components'

interface INavProps {
  height: number,
  renderBubbleSelect: boolean,
  renderSavingLabel: boolean,
  renderShareButtons: boolean,
  renderInstallButton: boolean,
  renderDeactivateButton: boolean,
}

const Nav = styled<React.FC<INavProps>>(({
  height,
  renderBubbleSelect,
  renderSavingLabel,
  renderShareButtons,
  renderInstallButton,
  renderDeactivateButton,
  ...nativeProps
}) => <nav {...nativeProps}></nav>)`
  height: ${props => props.height}px;
  // ...
`

[Martinnord]

For newcomers to this discussion, like myself. What's the status of this problem? I also really like SC, but this issue makes me want to switch to other libs, especially when I've started using styled-system.

Thanks.

[franciscop]

Proposal: extend the functionality of .attrs() to allow to post-process the props after the template literal has been evaluated.

Currently if you remove an attribute, it's removed before evaluating the CSS:

const Card = styled.div.attrs({ width: null })`
  width: ${p => p.width + 20}px;  // Broken, `width: px` since it's evaluated before
  padding: 10px;
`;

export default () => <Card width={100}>Hello!</Card>;
// No `width` attribute though 🎉

I think the focus of the discussion (been reading the threads) has been about how to filter out the props, but since filtering out (for DOM purposes) is the same as null-ifying or making them undefined, the .attrs() is very closely related.

.attrs() second argument

Add a second argument to the function like .attrs(before, after) transformations. The before keeps working as it is now, and the after is a transformation that modifies the attributes after the template literal has been evaluated:

const Card = styled.div.attrs({}, { width: null })`
  width: ${p => p.width + 20}px;  // We can read `width` here 🎉
  padding: 10px;
`;

export default () => <Card width={100}>Hello!</Card>;
// Also no `width` attribute 🎉

Note: for all practical purposes of this discussion, an attribute set to null behaves the same as it being set to undefined or not being set relative to the DOM rendering.

[mikkelwf]

Any news regarding this issue..?

[karlhorky]

Looks like @stevesims reopened his shouldForwardProp proposal 👍

#3006

Just adopting the technique from Emotion seems pragmatic and quite acceptable.