feat(AutoControlledComponent): Default values

[jeffcarbs]

Fixes #764

I noticed a bug using AutoControlledComponent where initial state for non-AutoControlled props was getting clobbered in the componentWillMount of the ACC. This is because ACC uses this.state = ... instead of this.setState to set the state.

Example of the bug:

class MyComponent extends AutoControlledComponent {
  state = { someField: 'default' }

  render () => <div />
}

// Actual
shallow(<MyComponent />).state() === {}

// Expected
shallow(<MyComponent />).state() === { someField: 'default' }

In fixing that bug, I came across the #764 issue which I realized could be solved simultaneously. The initial state is set within the constructor which means it's available in the ACC's componentWillMount. I updated the componentWillMount to use the initial state if the value of the prop was not defined via props.

For example:

class MyComponent extends AutoControlledComponent {
  state = { someField: 'default' }

  static autoControlledProps = ['someField']

  render () => <div />
}

shallow(<MyComponent />).state() === { someField: 'default' }
shallow(<MyComponent defaultSomeField='propDefault' />).state() === { someField: 'propDefault' }
shallow(<MyComponent someField='prop' />).state() === { someField: 'prop' }

This seems pretty intuitive to me. The props are being passed directly to state, so to specify a "global default" outside of props, you set the state yourself in the constructor (or use the es7 property initializer, which is basically just sugar on the same thing).

---

Edit As an aside, I've been using AutoControlledComponent as a base for some of my own components via:

import AutoControlledComponent from 'semantic-ui-react/dist/commonjs/lib/AutoControlledComponent'

I think it's a really cool abstraction and lets you write super flexible/reusable components that either do all the work (control the state) or let the user do it from higher up.

@levithomason - Thoughts on open-sourcing it as a separate package? Its only dependency is lodash (which you could probably even drop) so would probably be pretty simple.

[codecov-io]

Current coverage is 95.80% (diff: 100%)

Merging #1066 into master will decrease coverage by 0.02%

@@             master      #1066   diff @@
==========================================
  Files           873        866     -7   
  Lines          4813       4790    -23   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           4612       4589    -23   
  Misses          201        201          
  Partials          0          0          

Powered by Codecov. Last update ae21628...affe19f

[levithomason]

I think this sounds sane, using the initial state as the "default" AC prop values. Another way of thinking about this is that it is the "default state", since, the props are copied to state immediately.

I'm gonna update this PR to undo the Accordion hacks as well just to ensure it is working correctly.

[levithomason]

I updated the Accordion to use the new initial state pattern in the constructor. I consolidated the "default state" logic into the getAutoControlledStateValue function as well. Finally, I added another test to cover an edge case.

LMK if anything looks out of place.

[levithomason]

I completed this todo, using the new proposed standard of setting initial state in the constructor.

[levithomason]

Updated this function to handle initial state. Since this is the SSOT when it comes to how we derive AC state I think it makes sense here. It also prevents an extra loop of state in CWM.

[levithomason]

Extra test to catch the edge case I ran into. If you have state = undefined React will initialize state as null. This then throws as we look for keys on null. I added a comment about this and a check for it in getAutoControlledStateValue.

[jeffcarbs]

I'd maybe do something like:

if (state) {
  const initialState = state[propName]
  if (initialState !== undefined) return initialState
}

• state will only every be: an object, null, or undefined. I think this is a more straightforward check than the toString check.
• if (initialState) return initialState wouldn't let null through, which should be a valid default value.
• It makes it the same style as the propValue and defaultProp returns.

[jeffcarbs]

Left a comment but otherwise I think this is g2g 👍

[levithomason]

I try to avoid extra nesting / branch logic when possible. Perhaps went too far in that case, updated :)

[levithomason]

Released in [email protected].