Merge pull request #26 from istarkov/master

Themr breaks shouldComponentUpdate shallow equal optimization

Author: javivelasco

package.json

@@ -39,11 +39,14 @@
     "eslint-plugin-babel": "^3.2.0",
     "eslint-plugin-react": "^5.0.1",
     "expect": "^1.18.0",
+    "fbjs": "^0.8.4",
     "jsdom": "^8.4.0",
     "mocha": "^2.4.5",
     "react": "^15.0.1",
     "react-addons-test-utils": "^15.0.1",
-    "rimraf": "^2.5.2"
+    "react-dom": "^15.3.2",
+    "rimraf": "^2.5.2",
+    "sinon": "^1.17.6"
   },
   "files": [
     "lib",

src/components/themr.js

@@ -48,6 +48,11 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
       composeTheme: optionComposeTheme
     }
 
+    constructor(...args) {
+      super(...args)
+      this.theme_ = this.calcTheme(this.props)
+    }
+
     getWrappedInstance() {
       invariant(optionWithRef,
         'To access the wrapped instance, you need to specify ' +
@@ -57,8 +62,8 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
       return this.refs.wrappedInstance
     }
 
-    getNamespacedTheme() {
-      const { themeNamespace, theme } = this.props
+    getNamespacedTheme(props) {
+      const { themeNamespace, theme } = props
       if (!themeNamespace) return theme
       if (themeNamespace &&  !theme) throw new Error('Invalid themeNamespace use in react-css-themr. ' +
         'themeNamespace prop should be used only with theme prop.')
@@ -68,8 +73,8 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
         .reduce((result, key) => ({ ...result, [removeNamespace(key, themeNamespace)]:  theme[key] }), {})
     }
 
-    getThemeNotComposed() {
-      if (this.props.theme) return this.getNamespacedTheme()
+    getThemeNotComposed(props) {
+      if (props.theme) return this.getNamespacedTheme(props)
       if (config.localTheme) return config.localTheme
       return this.getContextTheme()
     }
@@ -80,30 +85,49 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
         : {}
     }
 
-    getTheme() {
-      return this.props.composeTheme === COMPOSE_SOFTLY
-        ? { ...this.getContextTheme(), ...config.localTheme, ...this.getNamespacedTheme() }
-        : themeable(themeable(this.getContextTheme(), config.localTheme), this.getNamespacedTheme())
+    getTheme(props) {
+      return props.composeTheme === COMPOSE_SOFTLY
+        ? {
+          ...this.getContextTheme(),
+          ...config.localTheme,
+          ...this.getNamespacedTheme(props)
+        }
+        : themeable(
+          themeable(this.getContextTheme(), config.localTheme),
+          this.getNamespacedTheme(props)
+        )
+    }
+
+    calcTheme(props) {
+      const { composeTheme } = props
+      return composeTheme
+        ? this.getTheme(props)
+        : this.getThemeNotComposed(props)
+    }
+
+    componentWillReceiveProps(nextProps) {
+      if (
+        nextProps.composeTheme !== this.props.composeTheme ||
+        nextProps.theme !== this.props.theme ||
+        nextProps.themeNamespace !== this.props.themeNamespace
+      ) {
+        this.theme_ = this.calcTheme(nextProps)
+      }
     }
 
     render() {
-      const { composeTheme, ...rest } = this.props
       let renderedElement
 
       if (optionWithRef) {
         renderedElement = React.createElement(ThemedComponent, {
-          ...rest,
+          ...this.props,
           ref: 'wrappedInstance',
-          theme: composeTheme
-            ? this.getTheme()
-            : this.getThemeNotComposed()
+          theme: this.theme_
         })
       } else {
         renderedElement = React.createElement(ThemedComponent, {
-          ...rest,
-          theme: composeTheme
-            ? this.getTheme()
-            : this.getThemeNotComposed()
+          ...this.props,
+          theme: this.theme_
         })
       }
 

test/components/ThemeProvider.spec.js

@@ -31,14 +31,14 @@ describe('ThemeProvider', () => {
       expect(() => TestUtils.renderIntoDocument(
         <ThemeProvider theme={theme}>
         </ThemeProvider>
-      )).toThrow(/exactly one child/)
+      )).toThrow(/expected to receive a single React element child/)
 
       expect(() => TestUtils.renderIntoDocument(
         <ThemeProvider theme={theme}>
           <div />
           <div />
         </ThemeProvider>
-      )).toThrow(/exactly one child/)
+      )).toThrow(/expected to receive a single React element child/)
     } finally {
       ThemeProvider.propTypes = propTypes
     }

test/components/themr.spec.js

@@ -1,6 +1,9 @@
 import expect from 'expect'
 import React, { Children, PropTypes, Component } from 'react'
 import TestUtils from 'react-addons-test-utils'
+import sinon from 'sinon'
+import { render } from 'react-dom'
+import shallowEqual from 'fbjs/lib/shallowEqual'
 import { themr } from '../../src/index'
 
 describe('Themr decorator function', () => {
@@ -408,4 +411,88 @@ describe('Themr decorator function', () => {
       ...bar
     })
   })
+
+  it('should not update theme prop on rerender if nothing changed', () => {
+    const spy = sinon.stub().returns(<div />)
+    const div = document.createElement('div')
+
+    @themr('Container')
+    class Container extends Component {
+      shouldComponentUpdate(nextProps) {
+        return !shallowEqual(nextProps, this.props)
+      }
+
+      render() {
+        return spy()
+      }
+    }
+
+    render(
+      <Container />,
+      div
+    )
+
+    render(
+      <Container />,
+      div
+    )
+
+    expect(spy.calledOnce).toBe(true)
+  })
+
+  it(
+    'should update theme prop on rerender if theme or themeNamespace or composeTheme changed',
+    () => {
+      const spy = sinon.stub().returns(<div />)
+      const div = document.createElement('div')
+
+      @themr('Container')
+      class Container extends Component {
+        shouldComponentUpdate(nextProps) {
+          return !shallowEqual(nextProps, this.props)
+        }
+
+        render() {
+          return spy()
+        }
+      }
+      const themeA = {}
+      const themeB = {}
+      const themeNamespace = 'nsA'
+
+      render(
+        <Container theme={themeA} />,
+        div
+      )
+
+      render(
+        <Container theme={themeB} />,
+        div
+      )
+
+      expect(spy.calledTwice).toBe(true)
+
+      render(
+        <Container theme={themeB} themeNamespace={themeNamespace} />,
+        div
+      )
+
+      expect(spy.calledThrice).toBe(true)
+
+
+      render(
+        <Container theme={themeB} themeNamespace={themeNamespace} composeTheme={'deeply'} />,
+        div
+      )
+
+      expect(spy.calledThrice).toBe(true)
+
+      render(
+        <Container theme={themeB} themeNamespace={themeNamespace} composeTheme={'softly'} />,
+        div
+      )
+
+      expect(spy.callCount === 4).toBe(true)
+    }
+  )
 })