breaking(AutoControlledProps): Ignore undefined

[jeffcarbs]

Breaking Change

This PR changes the way components auto control their state and how they release control. Previously, when a prop was removed or set to undefined, the component would not resume control. Now, when an auto controlled prop is removed or set to undefined, it will resume auto control.

This applies to all auto controlled prop for all components. The Dropdown open prop and state is used for illustration.

Before

Previously, passing undefined to a Component's auto controlled open prop would control the open state and prevent the Dropdown from auto controlling its open state:

<Dropdown open={undefined} />

After

Now, undefined props are treated as non-existent so you must explicitly pass a value if you wish you prevent the Dropdown from auto controlling the open prop.

<Dropdown open={false} />
<Dropdown open={null} />

---

Fixes an issue raised in #769

@levithomason - this is somewhat of an RFC as I'm not sure if this is the way we want this to work.

AutoControlledComponent (ACC) will control props that weren't passed by the parent, and defer to the parent-defined props if passed. However, in the case of that issue with the Modal there was no way to pass the open prop through to the Portal without forcing "controlled" usage. This PR updates the ACC to control props that are either missing entirely (existing behavior) or that are passed as undefined from the parent.

This seems sane to me because it follows the convention of:

• undefined - same as if I didn't pass it
• null - I explicitly passed an empty/null value
• anyValue - I explicitly passed a non-nil value

One weird thing is that if you initially set a prop to any value, and then later on set it to undefined, it won't unset the value. IMHO this could seem unexpected because doing something like { ...props, x: undefined } would yield an object with x === undefined.

Again, I opened this more just to start a discussion so if you don't think this makes sense I'm fine closing this. I kinda think we should support the usage suggested in #769, which this does, so if not this we'll need to figure out a different way.

[levithomason]

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

[levithomason]

This seems sane to me because it follows the convention of:

Agreed, this is something we should solve.

One weird thing is that if you initially set a prop to any value, and then later on set it to undefined, it won't unset the value. IMHO this could seem unexpected because doing something like { ...props, x: undefined } would yield an object with x === undefined.

I actually went down this same road when first creating the ACC. I recall specifically dealing with undefined. Initially, I had gone with the approach in this PR, then for reasons I can't quite recall, settled on not overriding undefined props even though React considers them non-present.

I believe it had to do with something similar to what you've noted. The difference / hang up is in trying to "remove" a prop after it has been set once. This works for props, but suddenly doesn't for the state of an ACC since it will not pull the undefined prop down over the state.

Possible Solution

I think we need to retain the user's ability to pass undefined as a prop, and have the ACC state reflect the same. The fact that props are copied to state should be transparent to the user IMO. So, the ACC state should work/feel like props.

We might be able to retain both features if we shallow diff props in componentWillReceiveProps and setState for any props were updated to undefined, in which case we'd remove them from state. Then, we stil set state values for undefined props in trySetState. If I am thinking about this correctly, that would ensure a user can "remove" auto controlled state by setting the prop to undefined and that prop would resume auto controlled behavior since trySetState would be allowed to update the local state value for that prop.

We could try this, feedback?

[jeffcarbs]

@levithomason - The issue I see there is that there's no way for the user to "give up control" without clobbering/resetting the value on the way out. Imagine a user controlling the value and then trying to give up control:

// Controlled
<Modal open /> // state.open === true

<Modal open={false} /> // state.open === false

<Modal open /> // state.open === true

// Giving up control by not passing the prop (assuming state currently === true)
<Modal /> // state.open === true

// Giving up control by passing undefined (assuming state currently === true)
<Modal open={undefined} /> // state.open === ?

We can...

1. set it to undefined, which would close the modal
2. ignore it, which would leave the modal open and let ACC take over controlling state

Option 2, which is the behavior within this PR, would be the same result as if the user did not pass the prop at all.

[saitonakamura]

IMO, we should seperate props that can accept undefined and the ones that shoudn't. For example, setting indefined over <Dropdown value={undefined} /> is perfectly sane for me, but you use <Modal>, you should set a bool, nor string, nor undefined, it makes no sense.

[levithomason]

@jcarbo I think we're saying the same thing, but I want to confirm.

Assuming / Releasing Control

User's should be able to assume and release control of any prop at any time. I think you should have two paths to giving up control, either remove the prop or pass an undefined value.

Assume this represents a sequential progression of prop values

// Controlled
<Modal open />
<Modal open={false} />

// Releasing control
<Modal />
// or
<Modal open={undefined} />

Persisted State

The second question is, when control is released, what should the persisted state be? There is some code that determines the initial value of an auto controlled prop. I think we need to run the same logic when a user releases control, except ignore defaultProps of course.

This way, if you have open={true} or value='my text', when you remove or set those to undefined you would get open={false} and value=''. I think this is the expected result. When a user removes a prop or passes undefined, they expect it to "go away":

// Controlled

<Modal open />              // this.state.open === true

// Releasing control
// (set initial values again)

<Modal />                   // this.state.open === false
// or
<Modal open={undefined} />  // this.state.open === false

That hunk of code in ACC does exactly that. For each auto controlled prop, it sets the initial value. If there is no actual prop value, then it defaults to the correct "not on / empty" value.

Final Solution

In order to do implement this, I think we pull the the logic for initial auto controlled prop values out from where it is into a function and also use it in componentWillReceiveProps. Then, when a user removes or sets a prop undefined, it will "go away" on state as well.

P.S.

I'm not sure if we'll need to add this to trySetState, but we can investigate that afterward.

[levithomason]

I believe we're in agreement on this. The current PR covers most of the discussed features. The remaining feature is to compare props on componentWillReceiveProps and reset state to the "initial value" if the prop has gone from having a value to "undefined".

I'll add this update now.

[codecov-io]

Current coverage is 100% (diff: 100%)

No coverage report found for master at 7d5f93f.

Powered by Codecov. Last update 7d5f93f...d9a9e66

[levithomason]

OK, I've updated this PR and added some tests. You can see the relevant branch logic here.

In short, if a user removes or sets a prop undefined, then we reinitialize its auto controlled state value as well. The prop will be auto controlled again from that point forward. If a user then adds a value to that prop, it will defer to the prop value again.

I'll merge once tests pass and @jcarbo gives this a sanity check scan. Also, I don't think this is a breaking change any longer, if anything, I'd call it a bug fix.

[levithomason]

@jcarbo would love a set of eyes on this guy. Will probably ship today either way if you're stacked.

[jeffcarbs]

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.

[levithomason]

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

[jeffcarbs]

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

[levithomason]

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.

[jeffcarbs]

@levithomason - left some comments that I think are worth addressing.

[levithomason]

I've updated both suggestions. I also added more tests for these. Finally, in light of #864, I removed use of _.omit and some other lodash methods here in favor of vanilla JS.

[levithomason]

Released in semantic-ui-react@0.61.0