Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shouldForwardProp in styled component #900

Closed
timomeh opened this issue Oct 8, 2018 · 9 comments
Closed

shouldForwardProp in styled component #900

timomeh opened this issue Oct 8, 2018 · 9 comments

Comments

@timomeh
Copy link

timomeh commented Oct 8, 2018

  • emotion version: 9.2.9
  • react version: 16.5.0

Relevant code.

import styled from '@emotion/native'
import isPropValid from '@emotion/is-prop-valid'

const Text = styled.Text(props => ({
  fontFamily: "Lato"
}))

const Headline = styled(Text, { shouldForwardProp: isPropValid })(props => ({
  color: props.isColored ? props.theme.baseColor : props.theme.textColor,
  fontWeight: '700'
}))

<Headline isColored>Hello, World!</Headline>

What you did:

I'm using react-native and react-testing-library together with react-native-web to test my components.

I added { shouldForwardProp: isPropValid } to my styled components in react native to prevent the prop isColored from being forwarded.

What happened:

Using the setup described above, I noticed that the prop isColored is being forwarded. The following error message will be displayed:

Warning: React does not recognize the isColored prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase iscolored instead. If you accidentally passed it from a parent component, remove it from the DOM element.

Problem description:

It seems like shouldForwardProp isn't being called in @emotion/native. I tried passing a console.warn to see if shouldForwardProp will be called, but nothing will be logged.

const shouldForwardProp = () => {
  console.warn('shouldForwardProp called')
}

const Headline = styled(Text, { shouldForwardProp  })(...))

Suggested solution:

@emotion/native uses createStyled from @emotion/primitives-core:

import { StyleSheet } from 'react-native'
import { createStyled } from '@emotion/primitives-core'
/**
* a function that returns a styled component which render styles in React Native
*/
let styled = createStyled(StyleSheet)

primitives-core has a default implementation for shouldForwardProp:

let defaultPickTest = prop => prop !== 'theme' && prop !== 'innerRef'
type options = {
getShouldForwardProp: (cmp: React.ElementType) => (prop: string) => boolean
}
export function createStyled(
StyleSheet: Object,
{ getShouldForwardProp = () => defaultPickTest }: options = {}
) {

It seems like @emotion/native doesn't override this default implementation. It should provide an API to pass shouldForwardProp as option.

Or maybe there is an API for it which isn't documented, or I can't find the documentation, or I can't find a way to do it in the emotion package code.

There is a discussion regarding this in the PR for @emotion/native: https://github.com/emotion-js/emotion/pull/759/files#r204662931
But to be totally honest, I don't understand why the API is intentionally different in comparison to @emotion/styled-base:

let createStyled: CreateStyled = (tag: any, options?: StyledOptions) => {
if (process.env.NODE_ENV !== 'production') {
if (tag === undefined) {
throw new Error(
'You are trying to create a styled element with an undefined component.\nYou may have forgotten to import it.'
)
}
}
let identifierName
let shouldForwardProp
let targetClassName
if (options !== undefined) {
identifierName = options.label
targetClassName = options.target
shouldForwardProp =

@Andarist
Copy link
Member

Andarist commented Oct 8, 2018

I think it isn't intentionally different, just native version of emotion is much newer than the version for the web and some differences might still be there, they should be unified though - as much as it's possible.

We are happy to accept a PR handling custom shouldForwardProp.

@timomeh
Copy link
Author

timomeh commented Oct 8, 2018

Sure, I can take a look at it. Maybe this would also be a good opportunity to unify the shouldForwardProp API and add documentation for it, if the implementation in @emotion/native works.

@Andarist
Copy link
Member

Andarist commented Oct 8, 2018

The documentation is already added on master, it might not be released yet though as master contains v10-beta code currently.

@timomeh
Copy link
Author

timomeh commented Oct 8, 2018

Could you post a link to the documentation for shouldForwardProp? I can't find it.

@Andarist
Copy link
Member

Andarist commented Oct 8, 2018

Oh, it's actually not merged into the master yet. Sorry for the confusion. You can check it out here https://github.com/emotion-js/emotion/pull/799/files#diff-e8ee253d6b88874fa7b359247bf59aa1R158

@timomeh
Copy link
Author

timomeh commented Oct 8, 2018

Thank you!

@dimaip
Copy link

dimaip commented Oct 3, 2019

This issues makes @emotion/primitives unusable with the styled syntax, I have to fallback to using css props :(

@Andarist Andarist added this to the v10-todo milestone Oct 27, 2019
@Andarist
Copy link
Member

Andarist commented Nov 4, 2019

Implementing this is not overly complex - I encourage anyone interested in this feature to prepare a PR for this.

@Andarist
Copy link
Member

I've implemented this in #1642 and it's going to be released in v11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants