override shouldForwardProp

[maxmedina05]

Summary

Currently, some props are being passed and written to the DOM. Emotion is handling this by using is-prop-valid which checks if the prop is a valid html attribute before writing. Here

Test plan

[netlify]

Deploy request for xstyled rejected.

Rejected with commit cbdacd6

https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy

[maxmedina05]

Deploy request for xstyled pending review.

Review with commit 95cc553

https://app.netlify.com/sites/xstyled/deploys

how?

[gregberge]

Dependencies are not at the good place, they should be added in the concerned package. Also no package-lock, we use yarn here. And define dependencies with ^.

[agriffis]

I think you might run into trouble with this approach if you wrap another component instead of an HTML element. In that case you want to filter out the system props, but you must pass through other props which aren't necessarily HTML attributes.

For example:

const Component = ({foo}) => <div>{foo}</div>

const StyledComponent = styled(Component)`
  color: pink;
`

<StyledComponent foo="Hello!" />

You can't apply isPropValid in StyledComponent because it will remove foo from the props.

The problem gets trickier if you use as={Component} because now it needs to be dynamic at run-time instead of using knowledge of the wrapped component at code-time.

And most unfortunately, solving this might require a change in emotion upstream. The problem is that Emotion doesn't pass the dynamic element to be created to shouldForwardProp and so you can't tell how to filter! The best you can do is to filter system props and let the rest through.

Here you can see it... For its internal determination, Emotion uses getShouldForwardProp(finalTag) but the user-specified shouldForwardProp doesn't get the same privilege:

https://github.com/emotion-js/emotion/blob/73de88e70d6c4325d3873be0b7c0224675697e26/packages/primitives-core/src/styled.js#L60-L63

I recently confronted this in styled-components and was able to make the change upstream, see styled-components/styled-components#3436 (for v6 in development) and styled-components/styled-components#3451 (for v5, merged but not released yet)

I'm not an expert on Emotion so I'd suggest you check everything I claim here, but this PR caught my eye because of my recent related work on styled-components.

[gregberge]

@agriffis nice catch, so you think it is not possible to do it. But a great question is, why would you pass unallowed props?

[gregberge]

@maxmedina05 I am sorry but I can't merge it, it would break all as usage. You should control what props are passing. How about wrapping x.* by another component that filters props if you need it.

[agriffis]

@gregberge I don't fully understand your question. When you use a styled component, the props are a mixture of three things:

1. styled component props (including system props) which affect the generated style
2. props that should be consumed by a wrapped component
3. props that should appear as attrs on the eventual HTML element

With this mix of props, you can reliably apply shouldForwardProp to strip the known props from (1). However this requires a deliberately created shouldForwardProp since it needs to know what props to strip.

If the wrapped component is already an HTML element, then you can also strip any non-HTML attrs. However this depends on being able to detect as={Component} which is not currently possible in Emotion's shouldForwardProp

The only full solution I can think of is to declare StyledComponent's prop-types ahead of the component, and then use them in a custom shouldForwardProp. I'll use styled-components for example because I don't know Emotion well enough:

/**
 * Factory for shouldForwardProp that takes into account the component's
 * propTypes (which should always be stripped, even if they are valid HTML attrs)
 * and also strips non-HTML attrs, if the wrapped component is an HTML element.
 */
const makeShouldForwardProp = propTypes => (prop, validAttr, el) =>
  !propTypes.hasOwnProperty(prop) && (typeof el !== 'string' || validAttr(prop))

const propTypes = {disabled: PropTypes.bool, ...getSystemPropTypes(system)}

const StyledComponent = styled.div.withConfig({shouldForwardProp: makeShouldForwardProp(propTypes)})(
  ({disabled}) => css`
    ${disabled && css`color: red;`}
    ${system}
  `)

StyledComponent.propTypes = propTypes

So we have a component that will:

1. strip disabled because it is expressly handled by the component, even though it is an HTML attr
2. strip system props since they are handled by the component
3. strip invalid HTML props IF the wrapped component is an HTML element (string)

I have been pondering how to make this more automatic in xstyled, as the OP of this issue desires, but I haven't solved it yet.

[agriffis]

I suppose I'm thinking something like this:

styled.div.withPropTypes(propTypes) // shorthand for .withConfig() PLUS attach propTypes
styled.divBox.withPropTypes(propTypes) // PLUS getSystemPropTypes() PLUS append ${system}

Oh, is x.div already broken for as={Component} in this regard? I think it needs this:

diff --git a/packages/styled-components/src/createX.ts b/packages/styled-components/src/createX.ts
index 4d3b72f..ef0939a 100644
--- a/packages/styled-components/src/createX.ts
+++ b/packages/styled-components/src/createX.ts
@@ -32,10 +32,10 @@ export const createX = <TProps extends object>(generator: StyleGenerator) => {
   tags.forEach((tag) => {
     // @ts-ignore
     x[tag] = styled(tag).withConfig({
-      shouldForwardProp: (prop, defaultValidatorFn) => {
+      shouldForwardProp: (prop, defaultValidatorFn, elementToBeCreated) => {
         if (typeof prop === 'string' && generator.meta.props.includes(prop))
           return false
-        return defaultValidatorFn(prop)
+        return elementToBeCreated !== 'string' || defaultValidatorFn(prop)
       },
     })<TProps>(() => [`&&{`, generator, `}`])
   })

This is kind of a bug in styled-components that the defaultValidatorFn it passes isn't really representative of the internal version. 😒 I'll see about making a PR for styled-components v6 to fix this, but it will be a breaking change.

[gregberge]

• For x.* you don't define the style of the component, so it does not make sense to pass any property that is not system or HTML.
• For custom styled components, it is already possible to use shouldForwardProp

So there is nothing to do in xstyled, everything's good.

[agriffis]

@gregberge x.* is broken with as={Component}

You described this approach here: #191 (comment)

but it will drop any non-HTML props, for example see https://codesandbox.io/s/cool-poincare-rzyyt?file=/src/App.js

I've created a PR #236 to fix this.

[agriffis]

@maxmedina05 You created this PR, I raised a problem with it, and now I'm sure it feels like it ran away from you. 😬

You said at the beginning:

Currently, some props are being passed and written to the DOM

In theory you shouldn't be passing props to <x.div> and friends that are not either (1) system props, or (2) HTML attrs. So if there are props winding up in the DOM that you don't expect, it is either a bug in xstyled or a misunderstanding in how you're using the utility.

If you are still seeing a problem, perhaps you could open an issue with an example.

[maxmedina05]

@agriffis nice catch, so you think it is not possible to do it. But a great question is, why would you pass unallowed props?

I've been trying to upgrade to v2 an internal library that was built using xstyled v1. After I upgraded, I get the following error for some box components that are using props for doing conditional styling. So far, this error is only happening for styled components using styled.box.

For example:

const StyledContainer = styled.box(
  {
    backgroundColor: "yellow",
    padding: "1rem"
  },
  ({ dark }) =>
    dark
      ? {
          backgroundColor: "black",
          color: "white"
        }
      : {}
);

The error:

I made this example, in Stackblitz: https://stackblitz.com/edit/react-lcxvdr?file=src/Example.js

[agriffis]

@maxmedina05 For now you'll need to use your own shouldForwardProp until Emotion upstream enhances their call to include elementToBeCreated, then we could consider an enhancement to xstyled to do it automatically.