Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

breaking(AutoControlledProps): Ignore undefined #788

Merged
merged 7 commits into from
Nov 16, 2016
Merged

breaking(AutoControlledProps): Ignore undefined #788

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c3248be
breaking(AutoControlledProps): Ignore undefined
jeffcarbs Nov 3, 2016
6aea3f0
feat(AutoControlledComponent): reinit state on cwrp
levithomason Nov 14, 2016
09c6b4b
feat(Popup): add debug logs
levithomason Nov 14, 2016
0791cb9
refactor(Popup): use if/else instead of switch
levithomason Nov 14, 2016
d23db73
fix(AutoControlledComponent): properly handle defaultProps
levithomason Nov 16, 2016
5b3e939
fix(AutoControlledComponent): only set state if changed
levithomason Nov 16, 2016
d9a9e66
perf(AutoControlledComponent): _.transform to reduce()
levithomason Nov 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 50 additions & 24 deletions src/lib/AutoControlledComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,33 @@ import { Component } from 'react'

const getDefaultPropName = (prop) => `default${prop[0].toUpperCase() + prop.slice(1)}`

/**
* Return the auto controlled state value for a give prop. The initial value is chosen in this order:
* - default props
* - then, regular props
* - then, `checked` defaults to false
* - then, `value` defaults to '' or [] if props.multiple
* - else, undefined
*
* @param {object} props A props object
* @param {string} propName A prop name
* @param {boolean} [includeDefaultProps=false] Whether or not to heed the default prop value
*/
export const getAutoControlledStateValue = (props, propName, includeDefaultProps = false) => {
const defaultPropName = getDefaultPropName(propName)

// defaultProps & props
if (includeDefaultProps && _.has(props, defaultPropName)) return props[defaultPropName]
if (props[propName] !== undefined) return props[propName]

// React doesn't allow changing from uncontrolled to controlled components,
// default checked/value if they were not present.
if (propName === 'checked') return false
if (propName === 'value') return props.multiple ? [] : ''

// otherwise, undefined
}

export default class AutoControlledComponent extends Component {
componentWillMount() {
if (super.componentWillMount) super.componentWillMount()
Expand All @@ -38,26 +65,10 @@ export default class AutoControlledComponent extends Component {
// Also look for the default prop for any auto controlled props (foo => defaultFoo)
// so we can set initial values from defaults.
this.state = _.transform(autoControlledProps, (res, prop) => {
const defaultPropName = getDefaultPropName(prop)

// try to set initial state in this order:
// - default props
// - then, regular props
// - then, `checked` defaults to false
// - then, `value` defaults to null
// React doesn't allow changing from uncontrolled to controlled components
// this is why we default checked/value if they are not present.
if (_.has(this.props, defaultPropName)) {
res[prop] = this.props[defaultPropName]
} else if (_.has(this.props, prop)) {
res[prop] = this.props[prop]
} else if (prop === 'checked') {
res[prop] = false
} else if (prop === 'value') {
res[prop] = this.props.multiple ? [] : '' // eslint-disable-line react/prop-types
}
res[prop] = getAutoControlledStateValue(this.props, prop, true)

if (process.env.NODE_ENV !== 'production') {
const defaultPropName = getDefaultPropName(prop)
const { name } = this.constructor
// prevent defaultFoo={} along side foo={}
if (defaultPropName in this.props && prop in this.props) {
Expand Down Expand Up @@ -120,10 +131,24 @@ export default class AutoControlledComponent extends Component {

componentWillReceiveProps(nextProps) {
if (super.componentWillReceiveProps) super.componentWillReceiveProps(nextProps)
const { autoControlledProps } = this.constructor

// props always win, update state with all auto controlled prop
const newState = _.pick(nextProps, this.constructor.autoControlledProps)
if (!_.isEmpty(newState)) this.setState(newState)
// Solve the next state for autoControlledProps
const newState = _.transform(autoControlledProps, (acc, prop) => {
const isNextUndefined = _.isUndefined(nextProps[prop])
const propWasRemoved = _.has(this.props, prop) && isNextUndefined
Copy link
Member Author

@jeffcarbs jeffcarbs Nov 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be:

const propWasRemoved = !_.isUndefined(this.props[prop]) && isNextUndefined

If you explicitly pass undefined then _.has(this.props, prop) will always return true because _.has just checks if the path exists, not that the path has a defined value. This means we'd continually reinitialize to the default value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I've updated this and similar issues in getAutoControlledStateValue(). I've also added several tests to ensure defaults are properly handled in both instances:

initial state
  ✔ uses the default prop is the regular prop is undefined
  ✔ uses the regular prop when a default is also defined

changing props
  ✔ does not return state to default props when setting props undefined


// if next is defined then use its value
if (!isNextUndefined) acc[prop] = nextProps[prop]

// reinitialize state for props just removed / set undefined
else if (propWasRemoved) acc[prop] = getAutoControlledStateValue(nextProps, prop)

// otherwise, continue persisting the auto controlled state value
else acc[prop] = this.state[prop]
}, {})

this.setState(newState)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth checking if state has actually changed so we can avoid a re-render if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final else branch here deferred to copying the old state to persist it. This meant new state was no longer ever empty. This was unnecessary since setting state for foo does not remove state for bar.

I've removed the unnecessary state copying and added a check to only set state if newState is not empty.

}

/**
Expand All @@ -147,9 +172,10 @@ export default class AutoControlledComponent extends Component {
}
}

// pick auto controlled props
// omit props from parent
let newState = _.omit(_.pick(maybeState, autoControlledProps), _.keys(this.props))
const parentDefinedProps = _.omitBy(this.props, _.isUndefined)

// Pick auto controlled props, omitting props defined by the parent.
let newState = _.omit(_.pick(maybeState, autoControlledProps), _.keys(parentDefinedProps))

if (state) newState = { ...newState, ...state }

Expand Down
50 changes: 27 additions & 23 deletions src/modules/Popup/Popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
getElementType,
getUnhandledProps,
isBrowser,
makeDebugger,
META,
SUI,
useKeyOnly,
Expand All @@ -14,6 +15,8 @@ import Portal from '../../addons/Portal'
import PopupContent from './PopupContent'
import PopupHeader from './PopupHeader'

const debug = makeDebugger('popup')

const _meta = {
name: 'Popup',
type: META.TYPES.MODULE,
Expand Down Expand Up @@ -222,32 +225,26 @@ export default class Popup extends Component {

const { on, hoverable } = this.props

switch (on) {
case 'click':
portalProps.openOnTriggerClick = true
portalProps.closeOnTriggerClick = true
portalProps.closeOnDocumentClick = true
break

case 'focus':
portalProps.openOnTriggerFocus = true
portalProps.closeOnTriggerBlur = true
break

default: // default to hover
portalProps.openOnTriggerMouseOver = true
portalProps.closeOnTriggerMouseLeave = true
// Taken from SUI: https://git.io/vPmCm
portalProps.mouseLeaveDelay = 70
portalProps.mouseOverDelay = 50
break
}

if (hoverable) {
portalProps.closeOnPortalMouseLeave = true
portalProps.mouseLeaveDelay = 300
}

if (on === 'click') {
portalProps.openOnTriggerClick = true
portalProps.closeOnTriggerClick = true
portalProps.closeOnDocumentClick = true
} else if (on === 'focus') {
portalProps.openOnTriggerFocus = true
portalProps.closeOnTriggerBlur = true
} else if (on === 'hover') {
portalProps.openOnTriggerMouseOver = true
portalProps.closeOnTriggerMouseLeave = true
// Taken from SUI: https://git.io/vPmCm
portalProps.mouseLeaveDelay = 70
portalProps.mouseOverDelay = 50
}

return portalProps
}

Expand All @@ -258,18 +255,21 @@ export default class Popup extends Component {
}

handleClose = (e) => {
debug('handleClose()')
const { onClose } = this.props
if (onClose) onClose(e, this.props)
}

handleOpen = (e) => {
debug('handleOpen()')
this.coords = e.currentTarget.getBoundingClientRect()

const { onOpen } = this.props
if (onOpen) onOpen(e, this.props)
}

handlePortalMount = (e) => {
debug('handlePortalMount()')
if (this.props.hideOnScroll) {
window.addEventListener('scroll', this.hideOnScroll)
}
Expand All @@ -279,11 +279,13 @@ export default class Popup extends Component {
}

handlePortalUnmount = (e) => {
debug('handlePortalUnmount()')
const { onUnmount } = this.props
if (onUnmount) onUnmount(e, this.props)
}

popupMounted = (ref) => {
debug('popupMounted()')
this.popupCoords = ref ? ref.getBoundingClientRect() : null
this.setPopupStyle()
}
Expand Down Expand Up @@ -333,10 +335,12 @@ export default class Popup extends Component {
</ElementType>
)

const mergedPortalProps = { ...this.getPortalProps(), ...portalProps }
debug('portal props:', mergedPortalProps)

return (
<Portal
{...this.getPortalProps()}
{...portalProps}
{...mergedPortalProps}
trigger={trigger}
onClose={this.handleClose}
onMount={this.handlePortalMount}
Expand Down
89 changes: 89 additions & 0 deletions test/specs/lib/AutoControlledComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,55 @@ describe('extending AutoControlledComponent', () => {
wrapper
.should.have.state(randomProp, props[randomProp])
})

it('sets state for props passed as undefined by the parent', () => {
consoleUtil.disableOnce()

const props = makeProps()
const autoControlledProps = _.keys(props)

const randomProp = _.sample(autoControlledProps)
const randomValue = faker.hacker.phrase()

props[randomProp] = undefined

TestClass = createTestClass({ autoControlledProps, state: {} })
const wrapper = shallow(<TestClass {...props } />)

wrapper
.instance()
.trySetState({ [randomProp]: randomValue })

wrapper
.should.have.state(randomProp, randomValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment here says // not updated, but the assertion is that the randomProp was updated from undefined to the randomValue. Is this correct?

})

it('does not set state for props passed as null by the parent', () => {
consoleUtil.disableOnce()

const props = makeProps()
const autoControlledProps = _.keys(props)

const randomProp = _.sample(autoControlledProps)
const randomValue = faker.hacker.phrase()

props[randomProp] = null

TestClass = createTestClass({ autoControlledProps, state: {} })
const wrapper = shallow(<TestClass {...props } />)

wrapper
.instance()
.trySetState({ [randomProp]: randomValue })

// not updated
wrapper
.should.not.have.state(randomProp, randomValue)

// is original value
wrapper
.should.have.state(randomProp, props[randomProp])
})
})

describe('initial state', () => {
Expand Down Expand Up @@ -244,5 +293,45 @@ describe('extending AutoControlledComponent', () => {
wrapper
.should.not.have.state(randomDefaultProp, randomValue)
})

it('sets state to undefined for props passed as undefined by the parent', () => {
consoleUtil.disableOnce()
const props = makeProps()
const autoControlledProps = _.keys(props)

const randomProp = _.sample(autoControlledProps)

TestClass = createTestClass({ autoControlledProps, state: {} })
const wrapper = shallow(<TestClass {...props} />)

// state exists initially
wrapper
.should.have.state(randomProp)

wrapper
.setProps({ [randomProp]: undefined })

// use "should not have" to assert undefined state
wrapper
.should.not.have.state(randomProp)
})

it('does not set state for props passed as null by the parent', () => {
consoleUtil.disableOnce()

const props = makeProps()
const autoControlledProps = _.keys(props)

const randomProp = _.sample(autoControlledProps)

TestClass = createTestClass({ autoControlledProps, state: {} })
const wrapper = shallow(<TestClass {...props} />)

wrapper
.setProps({ [randomProp]: null })

wrapper
.should.have.state(randomProp, null)
})
})
})