refactoring

Author: raveclassic

src/components/themr.js

@@ -84,7 +84,7 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
     getNamespacedTheme(props) {
       const { themeNamespace, theme } = props
       if (!themeNamespace) return theme
-      if (themeNamespace &&  !theme) throw new Error('Invalid themeNamespace use in react-css-themr. ' +
+      if (themeNamespace && !theme) throw new Error('Invalid themeNamespace use in react-css-themr. ' +
         'themeNamespace prop should be used only with theme prop.')
 
       return Object.keys(theme)
@@ -152,58 +152,85 @@ export default (componentName, localTheme, options = {}) => (ThemedComponent) =>
   return Themed
 }
 
-/**
- * Merges two themes by concatenating values with the same keys
- * @param {TReactCSSThemrTheme} [original] - Original theme object
- * @param {TReactCSSThemrTheme} [mixin] - Mixing theme object
- * @returns {TReactCSSThemrTheme} - Merged resulting theme
- */
-export function themeable(original = {}, mixin) {
-  //don't merge if no mixin is passed
-  if (!mixin) return original
-
-  //merge themes by concatenating values with the same keys
-  return Object.keys(mixin).reduce(
-
-    //merging reducer
-    (result, key) => {
-
-      const originalValue = typeof original[key] !== 'function'
-        ? (original[key] || '')
-        : ''
-      const mixinValue = typeof mixin[key] !== 'function'
-        ? (mixin[key] || '')
-        : ''
-
-      let newValue
-
-      //when you are mixing an string with a object it should fail
-      invariant(!(typeof originalValue === 'string' && typeof mixinValue === 'object'),
-        `You are merging a string "${originalValue}" with an Object,` +
-        'Make sure you are passing the proper theme descriptors.'
-      )
+export function themeable(original = {}, mixin = {}) {
+  //make a copy to avoid mutations of nested objects
+  //also strip all functions injected by isomorphic-style-loader
+  const result = Object.keys(original).reduce((acc, key) => {
+    const value = original[key]
+    if (typeof value !== 'function') {
+      acc[key] = value
+    }
+    return acc
+  }, {})
+
+  //traverse mixin keys and merge them to resulting theme
+  Object.keys(mixin).forEach(key => {
+    //there's no need to set any defaults here
+    const originalValue = result[key]
+    const mixinValue = mixin[key]
+
+    switch (typeof mixinValue) {
+      case 'object': {
+        //possibly nested theme object
+        switch (typeof originalValue) {
+          case 'object': {
+            //exactly nested theme object - go recursive
+            result[key] = themeable(originalValue, mixinValue)
+            break
+          }
+
+          case 'undefined': {
+            //original does not contain this nested key - just take it as is
+            result[key] = mixinValue
+            break
+          }
+
+          default: {
+            //can't merge an object with a non-object
+            throw new Error(`You are merging object ${key} with a non-object ${originalValue}`)
+          }
+        }
+        break
+      }
 
-      //check if values are nested objects
-      if (typeof originalValue === 'object' && typeof mixinValue === 'object') {
-        //go recursive
-        newValue = themeable(originalValue, mixinValue)
-      } else {
-        //either concat or take mixin value
-        newValue = originalValue.split(' ')
-          .concat(mixinValue.split(' '))
-          .filter((item, pos, self) => self.indexOf(item) === pos && item !== '')
-          .join(' ')
+      case 'function': {
+        //this handles issue when isomorphic-style-loader addes helper functions to css-module
+        break //just skip
       }
 
-      return {
-        ...result,
-        [key]: newValue
+      default: {
+        //plain values
+        switch (typeof originalValue) {
+          case 'object': {
+            //can't merge a non-object with an object
+            throw new Error(`You are merging non-object ${mixinValue} with an object ${key}`)
+          }
+
+          case 'undefined': {
+            //mixin key is new to original theme - take it as is
+            result[key] = mixinValue
+            break
+          }
+          case 'function': {
+            //this handles issue when isomorphic-style-loader addes helper functions to css-module
+            break //just skip
+          }
+
+          default: {
+            //finally we can merge
+            result[key] = originalValue.split(' ')
+              .concat(mixinValue.split(' '))
+              .filter((item, pos, self) => self.indexOf(item) === pos && item !== '')
+              .join(' ')
+            break
+          }
+        }
+        break
       }
-    },
+    }
+  })
 
-    //use original theme as an acc
-    original
-  )
+  return result
 }
 
 /**

test/components/themr.spec.js

@@ -510,17 +510,82 @@ describe('themeable function', () => {
     expect(result).toEqual(expected)
   })
 
-  it('should skip dupplicated keys classNames', () => {
+  it('should skip duplicated keys classNames', () => {
     const themeA = { test: 'test' }
     const themeB = { test: 'test test2' }
     const expected = { test: 'test test2' }
     const result = themeable(themeA, themeB)
     expect(result).toEqual(expected)
   })
 
-  it('throws an exception when its called mixing a string with an object', () => {
-    expect(() => {
-      themeable('fail', { test: { foo: 'baz' } })
-    }).toThrow(/sure you are passing the proper theme descriptors/)
+  it('should take mixin value if original does not contain one', () => {
+    const themeA = {}
+    const themeB = {
+      test: 'test',
+      nested: {
+        bar: 'bar'
+      }
+    }
+    const expected = themeB
+    const result = themeable(themeA, themeB)
+    expect(result).toEqual(expected)
+  })
+
+  it('should take original value if mixin does not contain one', () => {
+    const themeA = {
+      test: 'test',
+      nested: {
+        bar: 'bar'
+      }
+    }
+    const themeB = {}
+    const expected = themeA
+    const result = themeable(themeA, themeB)
+    expect(result).toEqual(expected)
+  })
+
+  it('should skip function values for usage with isomorphic-style-loader', () => {
+    const themeA = {
+      test: 'test',
+      foo() {
+      }
+    }
+
+    const themeB = {
+      test: 'test2',
+      bar() {
+      }
+    }
+
+    const expected = {
+      test: [
+        themeA.test, themeB.test
+      ].join(' ')
+    }
+
+    const result = themeable(themeA, themeB)
+    expect(result).toEqual(expected)
+  })
+
+  it('should throw when merging objects with non-objects', () => {
+    const themeA = {
+      test: 'test'
+    }
+    const themeB = {
+      test: {
+      }
+    }
+    expect(() => themeable(themeA, themeB)).toThrow()
+  })
+
+  it('should throw when merging non-objects with objects', () => {
+    const themeA = {
+      test: {
+      }
+    }
+    const themeB = {
+      test: 'test'
+    }
+    expect(() => themeable(themeA, themeB)).toThrow()
   })
 })