Skip to content

Fix default props when extending styled components - #90

Merged
mxstbr merged 7 commits into
styled-components:masterfrom
diegohaz:fix/default-props
Oct 15, 2016
Merged

Fix default props when extending styled components#90
mxstbr merged 7 commits into
styled-components:masterfrom
diegohaz:fix/default-props

Conversation

@diegohaz

Copy link
Copy Markdown
Member

WIP

Reference: #88

@mxstbr

mxstbr commented Oct 15, 2016

Copy link
Copy Markdown
Member

Can I tell you that you're one of the best contributors I've ever seen? Provides both a reproduction and a failing test – 👏👏👏 – I'm a fan.

Are you looking into fixing this? I think the correct way would be to use the check if it's a previous styled component and then doing class StyledComponent extends PreviousStyledComponent – but I'm not 100% sure if that works.

EDIT: Also, I'm not sure if that starts breaking down three levels deep (i.e. const A = styled('div') const B = styled(A) const C = styled(B)) – might be good to add a test case for that too?

@diegohaz

diegohaz commented Oct 15, 2016

Copy link
Copy Markdown
Member Author

I think the problem is this line. When you create StyledComponent passing the target.target of the parent styled component, it looses other properties.

if (isStyledComponent) return createStyledComponent(target.target, target.rules.concat(rules))

@mxstbr

mxstbr commented Oct 15, 2016

Copy link
Copy Markdown
Member

Yeah exactly. I think instead of doing that, we should do

let Extended

if (isStyledComponent) {
  Extended = target
} else {
  Extended = React.Component
}

class StyledComponent extends Extended {
  // etc.
}

or something like that maybe?

@geelen

geelen commented Oct 15, 2016

Copy link
Copy Markdown
Member

Gotta merge those RuleSets tho

@diegohaz

Copy link
Copy Markdown
Member Author

The Extended solution makes other tests to fail. :(

I managed to make it work in a simple way, but I'm not very used to these typings so maybe it needs some review.

@mxstbr

mxstbr commented Oct 15, 2016

Copy link
Copy Markdown
Member

I managed to make it work in a simple way, but I'm not very used to these typings so maybe it needs some review.

Don't worry too much about the typings, they really need a comprehensive review. (see #51)

The solution you did works for defaultProps, but what about propTypes and some others? Those should be on there too, which is why I'd prefer if we can make extends work somehow. That would be a more holistic solution. (i.e. work for all cases, not just ones we think of)

@diegohaz

Copy link
Copy Markdown
Member Author

Yeah, I've tried to do that way, but I think I would need to understand better the whole program flow.

First, I've updated the StyledComponent.js file to this, but all "extending" tests fail, even the one I'm trying to fix:

// @flow
import { Component, createElement, PropTypes } from 'react'

import validAttr from '../utils/validAttr'
import { CHANNEL } from './ThemeProvider'

import type { RuleSet, Target } from '../types'

export default (ComponentStyle: any) => {
  const createStyledComponent = (target: Target, rules: RuleSet, parent?: Target) => {
    const Extended = parent || Component
    /* eslint-disable react/prefer-stateless-function */
    class AbstractStyledComponent extends Extended {
      static isPrototypeOf: Function
      state: any
    }

    /* Handle styled(OtherStyledComponent) differently */
    const isStyledComponent = AbstractStyledComponent.isPrototypeOf(target)
    if (isStyledComponent) {
      return createStyledComponent(target.target, target.rules.concat(rules), target)
    }

    const isTag = typeof target === 'string'
    const componentStyle = new ComponentStyle(rules)

    class StyledComponent extends AbstractStyledComponent {
      static rules: RuleSet
      static target: Target
      state: {
        theme: any,
      }
      unsubscribe: Function

      constructor() {
        super()
        this.state = {
          theme: null,
        }
      }

      componentWillMount() {
        // If there is a theme in the context, subscribe to the event emitter. This
        // is necessary due to pure components blocking context updates, this circumvents
        // that by updating when an event is emitted
        if (this.context[CHANNEL]) {
          const subscribe = this.context[CHANNEL]
          this.unsubscribe = subscribe(theme => {
            // This will be called once immediately
            this.setState({ theme })
          })
        }
      }

      componentWillUnmount() {
        if (this.unsubscribe) {
          this.unsubscribe()
        }
      }

      /* eslint-disable react/prop-types */
      render() {
        const { className, children } = this.props
        const theme = this.state.theme || {}
        const executionContext = Object.assign({}, this.props, { theme })

        const generatedClassName = componentStyle.generateAndInjectStyles(executionContext)
        const propsForElement = {}
        /* Don't pass through non HTML tags through to HTML elements */
        Object.keys(this.props)
          .filter(propName => !isTag || validAttr(propName))
          .forEach(propName => {
            propsForElement[propName] = this.props[propName]
          })
        propsForElement.className = [className, generatedClassName].filter(x => x).join(' ')

        return createElement(target, propsForElement, children)
      }
    }

    /* Used for inheritance */
    StyledComponent.rules = rules
    StyledComponent.target = target

    StyledComponent.displayName = isTag ? `styled.${target}` : `Styled(${target.displayName})`
    StyledComponent.contextTypes = {
      [CHANNEL]: PropTypes.func,
    }
    return StyledComponent
  }

  return createStyledComponent
}

The test output is:

1) e2e extending should generate a single class if only parent has styles:

      Error: Expected '.a { color: blue; } .b { }' to equal '.a { color: blue; }'
      + expected - actual

      -.a { color: blue; } .b { }
      +.a { color: blue; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:178:61)

  2) e2e extending should generate a single class if only child has styles:

      Error: Expected '.a { color: blue; } .b { }' to equal '.a { color: blue; }'
      + expected - actual

      -.a { color: blue; } .b { }
      +.a { color: blue; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:188:61)

  3) e2e extending should generate a class for the child with the rules of the parent:

      Error: Expected '.a { color: red; }' to equal '.a { color: blue;color: red; }'
      + expected - actual

      -.a { color: red; }
      +.a { color: blue;color: red; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:197:61)

  4) e2e extending should generate different classes for both parent and child:

      Error: Expected '.a { color: blue; } .b { color: red; }' to equal '.a { color: blue; } .b { color: blue;color: red; }'
      + expected - actual

      -.a { color: blue; } .b { color: red; }
      +.a { color: blue; } .b { color: blue;color: red; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:207:61)

  5) e2e extending should copy nested rules to the child:

      Error: Expected '.a { color: blue; } .a > h1 { font-size: 4rem; } .b { color: red; }' to equal '.a { color: blue; } .a > h1 { font-size: 4rem; } .b { color: blue; color: red; } .b > h1 { font-size: 4rem; }'
      + expected - actual

      -.a { color: blue; } .a > h1 { font-size: 4rem; } .b { color: red; }
      +.a { color: blue; } .a > h1 { font-size: 4rem; } .b { color: blue; color: red; } .b > h1 { font-size: 4rem; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:220:61)

  6) e2e extending should keep default props from parent:

      Error: Expected '.a { color: red; } .b { background-color: green; }' to equal '.a { color: red; } .b { color: red; background-color: green; }'
      + expected - actual

      -.a { color: red; } .b { background-color: green; }
      +.a { color: red; } .b { color: red; background-color: green; }

      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:81:30)
      at Context.<anonymous> (src/test/e2e.test.js:241:61)

Do you have any idea?

@diegohaz

Copy link
Copy Markdown
Member Author

Actually, I think I figured it out.

@mxstbr

mxstbr commented Oct 15, 2016

Copy link
Copy Markdown
Member

I was just about to take this, but if you figured it out feel free to push!

@mxstbr mxstbr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, well done! 👏

Two tiny things then we can ship this:

  • Make the same changes in StyledNativeComponent
  • Add tests to ensure it works even with triple inherited components (const A = styled('div') const B = styled(A) const C = styled(B))

@mxstbr mxstbr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, this looks great! Thanks so much for this very high quality PR, very good job!

@mxstbr
mxstbr merged commit 80164e5 into styled-components:master Oct 15, 2016
@geelen

geelen commented Oct 16, 2016

Copy link
Copy Markdown
Member

@diegohaz you hero!!

@diegohaz
diegohaz deleted the fix/default-props branch January 10, 2017 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants