fix(factories): remove circular references

[levithomason]

The Input PR #281 revealed a circular reference in factories. Button imports factories (to use icon/label). Input imports factories for icon, but also imports Button (for actions).

To avoid circular references and the undefined imports they create, factories for our components have moved to a static create method:

Button.create()
Icon.create()
Image.create()
Label.create()

Factories for HTML components do not have circular reference issues since they do not need to import anything. These remain in factories.js:

createHTMLImage()
createHTMLInput()
createShorthand()
createShorthandFactory()

TODO

/cc @jcarbo @layershifter, I may do these as well in this PR. Feedback?

• 1. Almost every createShorthand() call can be turned into a static create() method on the component it is creating. This would further standardize things for us.
• 2. Now that there are no circular references, factories can be part of src/lib. This would further simplify dev "everything lives in /lib"
• 3. We could also expose mapValueToProps on each component. This would make testing easier by being able to reference a single function as the SSOT for how a component chooses to map a value to it's props. Currently, we repeat the mapValueToProps function when we write a test for a shorthand prop. Common tests for Icon/Label/etc also repeat those mapValueToProps functions.

[codecov-io]

Current coverage is 98.86% (diff: 100%)

Merging #550 into master will increase coverage by <.01%

@@             master       #550   diff @@
==========================================
  Files           108        108          
  Lines          1762       1766     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1742       1746     +4   
  Misses           20         20          
  Partials          0          0          

Powered by Codecov. Last update 9ea6ba9...496c2c9

[levithomason]

Example static shorthand create method for the Icon. I think this cohesion makes more sense anyhow. Keeping the mapValueToProps method in the same file as the component that it is creating.

[levithomason]

Example definition of a static shorthand create() method for our components.

This is where we could pull out the mapValueToProps method and also make it a static on the Button (see TODO 3 in PR description):

Button.mapValueToProps = value => ({ content: value })
Button.create = createShorthandFactory(Button, Button.mapValueToProps)

[levithomason]

@jcarbo I did in fact have to reverse the props for the string/number literal case only. Otherwise, the default type could not be overridden for createHTMLInput.

createHTMLInput(this.props.input, { type: 'password' })

In this case, if the user does not provide an input prop, then we want type='password'. However, if the mapValueToProps argument always wins, then the type: 'password' will always be overridden with type: undefined.

This is true for only for the literal value usage. The mapValueToProps is really like a baseProps, and is only invoked for the literal value case. It should also have less priority than the defaultProps, which then have less priority than the user's prop object/element props. Though, in this condition there are no user props, just a value.

[jeffcarbs]

Would be nice to get some test coverage on these functions. I think that would help make it more clear

I still don't see why we'd need to switch the order in this case. Using your example:

// Given this definition taken from the Input-v1 PR
const createHTMLInput = createShorthandFactory('input', value => ({ type: value }))

// and this usage within the stardust repo
createHTMLInput(this.props.input, { type: 'password' })

// Usage by a consumer of this lib:
<Input input='text' />

// The shorthand would always yield
<input type='password' />

I think about it this way, you can pass props as an object or you can pass a literal that's turned into an object. Both of those cases should be treated the same:

• as an object: { type: 'text' }
• as a literal: text is turned into {type: 'text'}

I can't how those should be treated differently in terms of overwriting defaultProps

[levithomason]

Indeed I have a branch adding tests currently, will push soon. This came up from the Input test for spreads input type failing since it could no longer override the type. Again, tests are very much needed and will no doubt solve all this.

[jeffcarbs]

To clarify my point further, you should be able to rewrite this:

export function createShorthand(Component, mapValueToProps, val, defaultProps = {}) {
  // Clone ReactElements
  if (isValidElement(val)) {
    return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
  }

  // Create ReactElements from props objects
  if (_.isPlainObject(val)) {
    return <Component {...mergePropsAndClassName(defaultProps, val)} />
  }

  // Map values to props and create a ReactElement
  if (_.isString(val) || _.isNumber(val)) {
    return <Component {...mergePropsAndClassName(defaultProps, mapValueToProps(val))} />
  }

  // Otherwise null
  return null
}

...as this:

export function createShorthand(Component, mapValueToProps, val, defaultProps = {}) {
  // Clone ReactElements
  if (isValidElement(val)) {
    return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
  }

  // Map values to props if it's a literal
  if (_.isString(val) || _.isNumber(val)) {
    val = mapValueToProps(val)
  }

  // Create ReactElements from props objects
  if (_.isPlainObject(val)) {
    return <Component {...mergePropsAndClassName(defaultProps, val)} />
  }

  // Otherwise null
  return null
}

In other words, mapValueToProps is just a convenience that converts a literal to a props object on the end-user's behalf. So when instantiating the component it makes no difference whether the user passed their props as an object or as a literal, they should always win.