Add transient Props to @xstyled/emotion

[maxmedina05]

Summary

Emotion allows its consumer apps to change styles based on props. See here. By default, emotion would prevent invalid props from being rendered to the DOM element but since xstyled is overriding the "shouldForwardProp" this functionally is being removed.

Styled-components handle this by prefixing the prop name with a dollar sign $. Transient Props

Do you see any downside or conflict by doing something this? @agriffis @gregberge

Test plan

My previous PR was causing some issues with the "as" so this is why I just remove the ones with the $.

[netlify[bot]]

👷 Deploy request for xstyled accepted.

Name	Link
🔨 Latest commit	0063ca66b78360fdec393ab064c6519ee68228cb
🔍 Latest deploy log	https://app.netlify.com/sites/xstyled/deploys/6087f0f5535ce500072b5173

[agriffis]

Thanks Max!

For reference, styled-components checks for transient props before calling shouldForwardProp:

https://github.com/styled-components/styled-components/blob/23ec638a5e0802b9b17b0ffde025e6ba1ea650c9/packages/styled-components/src/models/StyledComponent.ts#L135-L150

I think we should move the check before checking generator.meta.props (fast check before slow check)

I'm also suspicious that we should check for prop === 'theme' since I think I've seen that sneak through sometimes.

[maxmedina05]

Thank you for your quick response. @agriffis

Base on your last comment:

I'm also suspicious that we should check for prop === 'theme' since I think I've seen that sneak through sometimes.

I don't see the the theme being forwarded to the DOM from the theme provider. But if I explicitly add a "theme" prop to the component it would be render in the DOM. In that case, I don't understand why it should have a special treatment.

I reordered the check.

  it("doesn't forward theme", () => {
    const Dummy = styled.box``
    const { container } = render(
      <SpaceTheme>
        <Dummy />
      </SpaceTheme>,
    )
    expect(container.firstChild).not.toHaveAttribute('theme')
  })

[agriffis]

I appreciate your willingness to add an extra test, but I think we should leave that one out for now, since it's otherwise unrelated to this PR.

Otherwise this looks good to me. Do you have any concerns or thoughts, @gregberge ?

[maxmedina05]

I appreciate your willingness to add an extra test, but I think we should leave that one out for now, since it's otherwise unrelated to this PR.

Otherwise this looks good to me. Do you have any concerns or thoughts, @gregberge ?

@agriffis I removed the test. Thank you for the support.

[gregberge]

Hello @maxmedina05, thanks, it looks good to me.

[maxmedina05]

@agriffis @gregberge How does the pipeline works? Would this be automatically published? like 2.4.2? or do I need to trigger something?

[agriffis]

For now, Greg is the pipeline :stuck_out_tongue_winking_eye:

[maxmedina05]

Ey @gregberge

Thank you for letting merge this. With this I can finally finish upgrading an internal lib to v2.

could this be release as version 2.4.2?

[gregberge]

@maxmedina05 done