Forward .defaultProps when reusing __emotion_base

[Andarist]

fixes #654

I feel though that we should figure out more holistic approach as .defaultProps is only part of the problem - any other config (i.e. shouldForwardProp) will get lost too when reusing __emotion_base. This should act like .withComponent in this regard.

Thoughts?

[Andarist]

I've looked into the code and it seems to me that for isReal components we could return this:

return tag.__emotion_real.withComponent(tag.__emotion_base, options)

to fix shouldForwardProp situation, that wouldn't help with .defaultProps though. Also not sure if that would be totally safe, not sure how things like staticClassName, identifierName, stableClassName are shared for both of those cases.

[emmatown]

I think for shouldForwardProp we could set it as a static property on Styled and change

let shouldForwardProp

to

let shouldForwardProp = tag.__emotion_shouldForwardProp

so it uses the shouldForwardProp on the component that's passed in if it exists and passing a shouldForwardProp with a styled component that already has a shouldForwardProp will override the original shouldForwardProp.

For defaultProps, I think the only way to solve all the edge cases for it would be to opt out of the component flattening if defaultProps is defined since this solution wouldn't account for setting defaultProps on a styled component, passing it to styled again and setting defaultProps on that component as well.

[Andarist]

I think for shouldForwardProp we could set it as a static property on Styled

Problem with that approach is that we have to think about it & add those static properties whenever we introduce any new option. Could you explain in short (or link me to some resources) on how exactly staticClassName, identifierName, stableClassName are used? Do they suffer from the same problem? Or they simply shouldn't get forwarded?

For defaultProps, I think the only way to solve all the edge cases for it would be to opt out of the component flattening if defaultProps is defined since this solution wouldn't account for setting defaultProps on a styled component, passing it to styled again and setting defaultProps on that component as well.

Developer can (although it's a little bit quirky) extend defaultProps on his/her own.

[emmatown]

We don't do component flattening with extractStatic so staticClassName doesn't have to be forwarded.

identifierName is used for the displayName of components and adding appending that name to the class name via adding the label property. The label property is already in the styles that are stored on the static property and there wouldn't be a way to use displayName since the component would be flattened so there's no point in forwarding it.

stableClassName is the class name used for component selector targeting. This is the only one other than shouldForwardProp that we might want to forward because currently in the example below AnotherComponent isn't targeted by targeting SomeComponent but we might want it to.

const SomeComponent = styled.div`
  color: hotpink;
`

const AnotherComponent = styled(SomeComponent)`
  color: green;
`

render(
  <div
    css={css`
      ${SomeComponent} {
        color: darkcyan;
      }
    `}
  >
    <AnotherComponent />
  </div>
)

[Andarist]

Thanks for the explanation. The only thing I'm worrying about is that in the future we might want to "passthrough" more options and static properties approach doesnt scale that well.

Also as to shouldForwardProp - it seems to me that actually it should not just override (although it's easier code-wise), but should reuse existing shouldForwardProp. So it would work like .every. Thoughts?

As to defaultProps, do u want me to adjust the PR to opt out of component flattening?

[codecov]

Codecov Report

Merging #659 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/create-emotion-styled/src/index.js	100% <100%> (ø)	⬆️

[emmatown]

I think that using something that doesn't scale well for options is good since it encourages us to not add options because adding options means users have to learn more specifics about the API and IMO that's not something we want.

For reusing shouldForwardProp, I can't think of any specific use case where you would want it to be reused. Also, if shouldForwardProp is reused then we have to make assumptions about which function should take precedence. As an example, what should happen here? should it pass down someProp or not? since if we did firstShouldForwardProp(prop) || secondShouldForwardProp(prop) someProp would get passed down but I can't see how you would want someProp to be passed down here.

const SomeComponent = styled('div', {
  shouldForwardProp: prop => {
    switch (prop) {
      case 'someProp':
        return false
      default:
        return true
    }
  }
})`
  color: hotpink;
`

const AnotherComponent = styled(SomeComponent, {
  shouldForwardProp: prop => {
    return true
  }
})`
  background-color: green;
`

I'm just gonna merge this as is because the edge case I presented is going to be ridiculously rare so if someone opens an issue about it then let's fix it but right now, let's leave it like this.

[Andarist]

I've proposed it working like .every, so it wouldn't actually work like firstShouldForwardProp(prop) || secondShouldForwardProp(prop) but rather as firstShouldForwardProp(prop) && secondShouldForwardProp(prop).

As you have mentioned it doesnt make sense to forward someProp in this example, but it feels to me that if we have component that omits someProp from being passed down and we wrap it with styled again and omit any other prop too with new shouldForwardProp then both someProp and anotherProp should automatically not get forwarded downwards.

If we just replace shouldForwardProp with next one this would suddenly allow passing someProp which was not the original intention of SomeComponent.

[emmatown]

@Andarist yep, makes sense

see my comment in #670