feat: add generateKey option to factories

specifications/shorthand-api.md

@@ -5,17 +5,39 @@
 
 
 - [Why?](#why)
+  - [DOM Structure](#dom-structure)
+    - [A Moving Target](#a-moving-target)
+    - [Ownership](#ownership)
+  - [Intuition & Effort](#intuition--effort)
 - [Proposals](#proposals)
-  - [[Goal]](#goal)
+  - [Goal](#goal)
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
 ## Why?
 
-## Proposals
+### DOM Structure
+
+In traditional UI libraries, developers are required to memorize or copy and paste specific various brittle HTML structures to use components. The developer should be focused on a higher level of describing features.
+
+#### A Moving Target
+
+These structures are often required for the component to work correctly. This makes them brittle and a common source of bugs. This is only exacerbated by the fact that different variations of the same component often require slightly different structures.
+
+The component should own and isolate the brittle nature of required markup.
+
+#### Ownership
 
-### [Goal]
+Developers require components to have certain styles and behaviors. Components may require a specific DOM structure to achieve those styles and behaviors. Therefore, component DOM structure is the concern and responsibility of the component, not the user.
+
+### Intuition & Effort
+
+When describing a component to another human we use natural language, we don't speak HTML. When defining a component via an API we should strive to use the same natural language. This frees the developer's mind to spend its effort creating actual features opposed to creating implementations of features.
+
+Providing a high level API of natural language allows developers to use intuition and minimal effort to create features.
+
+## Proposals
 
-[Description]
+See the [docs][1].
 
-[Implementation]
+[1]: https://stardust-ui.github.io/react

src/components/Accordion/Accordion.tsx

@@ -124,7 +124,7 @@ class Accordion extends AutoControlledComponent<any, any> {
 
       children.push(
         AccordionTitle.create(title, {
-          autoGenerateKey: true,
+          generateKey: true,
           defaultProps: { active, index },
           overrideProps: this.handleTitleOverrides,
         }),
@@ -133,7 +133,7 @@ class Accordion extends AutoControlledComponent<any, any> {
         AccordionContent.create(
           { content },
           {
-            autoGenerateKey: true,
+            generateKey: true,
             defaultProps: { active },
           },
         ),

src/components/Header/Header.tsx

@@ -53,7 +53,7 @@ class Header extends UIComponent<any, any> {
       )
     }
 
-    const subheaderElement = HeaderSubheader.create(subheader, { autoGenerateKey: false })
+    const subheaderElement = HeaderSubheader.create(subheader, { generateKey: false })
 
     return (
       <ElementType {...rest} className={classes.root}>

src/lib/factories.tsx

@@ -2,48 +2,64 @@ import _ from 'lodash'
 import cx from 'classnames'
 import React, { cloneElement, isValidElement } from 'react'
 
+interface IProps {
+  [key: string]: any
+}
+
+type ShorthandFunction = (
+  Component: React.ReactType,
+  props: IProps,
+  children: any,
+) => React.ReactElement<IProps>
+type ShorthandValue = string & number & IProps & React.ReactElement<IProps> & ShorthandFunction
+type MapValueToProps = (value: ShorthandValue) => IProps
+
+interface ICreateShorthandOptions {
+  /** Default props object */
+  defaultProps?: IProps
+
+  /** Override props object or function (called with regular props) */
+  overrideProps?: IProps | ((props: IProps) => IProps)
+
+  /** Whether or not automatic key generation is allowed */
+  generateKey?: boolean
+}
+const CREATE_SHORTHAND_DEFAULT_OPTIONS: ICreateShorthandOptions = {
+  defaultProps: {},
+  overrideProps: {},
+  generateKey: true,
+}
+
 // ============================================================
 // Factories
 // ============================================================
 
-/**
- * A more robust React.createElement. It can create elements from primitive values.
- *
- * @param {function|string} Component A ReactClass or string
- * @param {function} mapValueToProps A function that maps a primitive value to the Component props
- * @param {string|object|function} val The value to create a ReactElement from
- * @param {Object} [options={}]
- * @param {object} [options.defaultProps={}] Default props object
- * @param {object|function} [options.overrideProps={}] Override props object or function (called with regular props)
- * @returns {object|null}
- */
+/** A more robust React.createElement. It can create elements from primitive values. */
 export function createShorthand(
-  Component: any,
-  mapValueToProps: any,
-  val?: any,
-  options: any = {},
-) {
+  Component: React.ReactType,
+  mapValueToProps: MapValueToProps,
+  value?: ShorthandValue,
+  options: ICreateShorthandOptions = CREATE_SHORTHAND_DEFAULT_OPTIONS,
+): React.ReactElement<IProps> | null {
   if (typeof Component !== 'function' && typeof Component !== 'string') {
     throw new Error('createShorthand() Component must be a string or function.')
   }
   // short circuit noop values
-  if (_.isNil(val) || _.isBoolean(val)) return null
+  if (_.isNil(value) || typeof value === 'boolean') return null
 
-  const valIsString = _.isString(val)
-  const valIsNumber = _.isNumber(val)
-  const valIsFunction = _.isFunction(val)
-  const valIsReactElement = isValidElement(val)
-  const valIsPropsObject = _.isPlainObject(val)
-  const valIsPrimitiveValue = valIsString || valIsNumber || _.isArray(val)
+  const valIsPrimitive = typeof value === 'string' || typeof value === 'number'
+  const valIsPropsObject = _.isPlainObject(value)
+  const valIsReactElement = isValidElement(value)
+  const valIsFunction = typeof value === 'function'
 
   // unhandled type return null
-  if (!valIsFunction && !valIsReactElement && !valIsPropsObject && !valIsPrimitiveValue) {
+  if (!valIsPrimitive && !valIsPropsObject && !valIsReactElement && !valIsFunction) {
     if (process.env.NODE_ENV !== 'production') {
       console.error(
         [
-          'Shorthand value must be a string|number|array|object|ReactElement|function.',
-          ' Use null|undefined|boolean for none',
-          ` Received ${typeof val}.`,
+          'Shorthand value must be a string|number|object|ReactElement|function.',
+          ' Use null|undefined|boolean for none.',
+          ` Received: ${value}`,
         ].join(''),
       )
     }
@@ -57,15 +73,17 @@ export function createShorthand(
 
   // User's props
   const usersProps =
-    (valIsReactElement && val.props) ||
-    (valIsPropsObject && val) ||
-    (valIsPrimitiveValue && mapValueToProps(val))
+    (valIsReactElement && value.props) ||
+    (valIsPropsObject && value) ||
+    (valIsPrimitive && mapValueToProps(value)) ||
+    {}
 
   // Override props
-  let { overrideProps = {} } = options
-  overrideProps = _.isFunction(overrideProps)
-    ? overrideProps({ ...defaultProps, ...usersProps })
-    : overrideProps
+  let { overrideProps } = options
+  overrideProps =
+    typeof overrideProps === 'function'
+      ? overrideProps({ ...defaultProps, ...usersProps })
+      : overrideProps || {}
 
   // Merge props
   const props = { ...defaultProps, ...usersProps, ...overrideProps }
@@ -88,25 +106,26 @@ export function createShorthand(
   // ----------------------------------------
   // Get key
   // ----------------------------------------
+  const { generateKey = true } = options
 
   // Use key or generate key
-  if (_.isNil(props.key) && (valIsString || valIsNumber)) {
+  if (generateKey && _.isNil(props.key) && valIsPrimitive) {
     // use string/number shorthand values as the key
-    props.key = val
+    props.key = value
   }
 
   // ----------------------------------------
   // Create Element
   // ----------------------------------------
 
   // Clone ReactElements
-  if (valIsReactElement) return cloneElement(val, props)
+  if (valIsReactElement) return cloneElement(value, props)
 
   // Create ReactElements from built up props
-  if (valIsPrimitiveValue || valIsPropsObject) return <Component {...props} />
+  if (valIsPrimitive || valIsPropsObject) return <Component {...props} />
 
   // Call functions with args similar to createElement()
-  if (valIsFunction) return val(Component, props, props.children)
+  if (valIsFunction) return value(Component, props, props.children)
 }
 
 // ============================================================

src/lib/renderComponent.tsx

@@ -8,7 +8,7 @@ import getUnhandledProps from './getUnhandledProps'
 import callable from './callable'
 
 export interface IRenderResultConfig<P> {
-  ElementType: React.ComponentType<P>
+  ElementType: React.ReactType<P>
   rest: { [key: string]: any }
   classes: { [key: string]: string }
 }

test/specs/commonTests/implementsShorthandProp.tsx

@@ -25,6 +25,7 @@ const shorthandComponentName = ShorthandComponent => {
  * @param {string|function} options.ShorthandComponent The component that should be rendered from the shorthand value.
  * @param {boolean} [options.alwaysPresent] Whether or not the shorthand exists by default.
  * @param {boolean} [options.assertExactMatch] Selects an assertion method, `contain` will be used if true.
+ * @param {boolean} [options.generateKey=false] Whether or not automatic key generation is allowed for the shorthand component.
  * @param {function} options.mapValueToProps A function that maps a primitive value to the Component props.
  * @param {Object} [options.requiredProps={}] Props required to render the component.
  * @param {Object} [options.shorthandDefaultProps] Default props for the shorthand component.
@@ -34,6 +35,7 @@ export default (Component, options: any = {}) => {
   const {
     alwaysPresent,
     assertExactMatch = true,
+    generateKey = false,
     mapValueToProps,
     propKey,
     ShorthandComponent,
@@ -52,13 +54,21 @@ export default (Component, options: any = {}) => {
 
     const name = shorthandComponentName(ShorthandComponent)
     const assertValidShorthand = value => {
-      const shorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, {
+      const expectedShorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, {
         defaultProps: shorthandDefaultProps,
         overrideProps: shorthandOverrideProps,
+        generateKey,
       })
       const element = createElement(Component, { ...requiredProps, [propKey]: value })
+      const wrapper = shallow(element)
 
-      shallow(element).should[assertMethod](shorthandElement)
+      wrapper.should[assertMethod](expectedShorthandElement)
+
+      // Enzyme's .key() method is not consistent with React for elements with
+      // no key (`undefined` vs `null`), so use the underlying element instead
+      // Will fail if more than one element of this type is found
+      const shorthandElement = wrapper.find(ShorthandComponent).getElement()
+      expect(shorthandElement.key).to.equal(expectedShorthandElement.key, "key doesn't match")
     }
 
     if (alwaysPresent || (Component.defaultProps && Component.defaultProps[propKey])) {

test/specs/lib/factories-test.tsx

@@ -15,21 +15,29 @@ const getShorthand = ({
   defaultProps,
   mapValueToProps = () => ({}),
   overrideProps,
+  generateKey,
   value,
-}: any) => createShorthand(Component, mapValueToProps, value, { defaultProps, overrideProps })
+}: any) =>
+  createShorthand(Component, mapValueToProps, value, {
+    defaultProps,
+    overrideProps,
+    generateKey,
+  })
 
 // ----------------------------------------
 // Common tests
 // ----------------------------------------
 
 const itReturnsNull = value => {
   test('returns null', () => {
+    consoleUtil.disableOnce()
     expect(getShorthand({ value })).toBe(null)
   })
 }
 
 const itReturnsNullGivenDefaultProps = value => {
   test('returns null given defaultProps object', () => {
+    consoleUtil.disableOnce()
     expect(getShorthand({ value, defaultProps: { 'data-foo': 'foo' } })).toBe(null)
   })
 }
@@ -120,6 +128,7 @@ describe('factories', () => {
     })
 
     test('throw if passed Component that is not a string nor function', () => {
+      consoleUtil.disableOnce()
       const badComponents = [undefined, null, true, false, [], {}, 123]
 
       _.each(badComponents, badComponent => {
@@ -148,6 +157,7 @@ describe('factories', () => {
     })
 
     test('throw if passed Component that is not a string nor function', () => {
+      consoleUtil.disableOnce()
       const badComponents = [undefined, null, true, false, [], {}, 123]
 
       _.each(badComponents, badComponent => {
@@ -209,6 +219,26 @@ describe('factories', () => {
           expect(getShorthand({ value: { key: '' } })).toHaveProperty('key', '')
         })
       })
+
+      describe('when value is a string', () => {
+        test('is generated from the value', () => {
+          expect(getShorthand({ value: 'foo' })).toHaveProperty('key', 'foo')
+        })
+
+        test('is not generated if generateKey is false', () => {
+          expect(getShorthand({ value: 'foo', generateKey: false })).toHaveProperty('key', null)
+        })
+      })
+
+      describe('when value is a number', () => {
+        test('is generated from the value', () => {
+          expect(getShorthand({ value: 123 })).toHaveProperty('key', '123')
+        })
+
+        test('is not generated if generateKey is false', () => {
+          expect(getShorthand({ value: 123, generateKey: false })).toHaveProperty('key', null)
+        })
+      })
     })
 
     describe('overrideProps', () => {
@@ -390,36 +420,8 @@ describe('factories', () => {
     })
 
     describe('from an array', () => {
-      itReturnsAValidElement(['foo'])
-      itAppliesDefaultProps(['foo'])
-      itMergesClassNames('mapValueToProps', 'mapped', {
-        value: ['foo'],
-        mapValueToProps: () => ({ className: 'mapped' }),
-      })
-
-      itAppliesProps(
-        'mapValueToProps',
-        { 'data-prop': 'present' },
-        {
-          value: ['foo'],
-          mapValueToProps: () => ({ 'data-prop': 'present' }),
-        },
-      )
-
-      itOverridesDefaultProps(
-        'mapValueToProps',
-        { some: 'defaults', overridden: false },
-        { some: 'defaults', overridden: true },
-        {
-          value: ['an array'],
-          mapValueToProps: () => ({ overridden: true }),
-        },
-      )
-
-      itOverridesDefaultPropsWithFalseyProps('mapValueToProps', {
-        value: ['an array'],
-        mapValueToProps: () => ({ undef: undefined, nil: null, zero: 0, empty: '' }),
-      })
+      itReturnsNull(['foo'])
+      itReturnsNullGivenDefaultProps(['foo'])
     })
 
     describe('style', () => {