downshift icon indicating copy to clipboard operation
downshift copied to clipboard

"stateAndHelpers" param within "onInputValueChange" function does not align with desired state

Open aaronnuu opened this issue 6 years ago • 3 comments

  • downshift version: 3.4.1
  • node version: 10.9.0
  • yarn version: 1.17.3

Potentially related to #759

Problem description:

The stateAndHelpers param from the onInputValueChange function doesn't actually represent my desired state in certain circumstances

This was introduced in #217 to fix the cursor position issue however the state reducer was never called against the stateToSet

if (!isStateToSetFunction && stateToSet.hasOwnProperty('inputValue')) {
  this.props.onInputValueChange(stateToSet.inputValue, {
    ...this.getStateAndHelpers(),
    ...stateToSet,
  })
}

Further down, if the stateToSet is a function then the onInputValueChange function isn't called until later

this.props.onInputValueChange(newStateToSet.inputValue, {
  ...this.getStateAndHelpers(),
  ...newStateToSet,
})

Where newStateToSet has been passed through the stateReducer

newStateToSet = this.props.stateReducer(state, newStateToSet)

This means that depending on what is passed to the internalSetState function (a state object or state function) then the stateAndHelpers of the onInputValueChange function will have different values.

Suggested solution:

Call stateReducer against the stateToSet before passing it to the onInputValueChange function



if (!isStateToSetFunction) {
  const newStateToSet = this.stateReducer(this.state, stateToSet)
  if (newStateToSet.hasOwnProperty('inputValue')) {
    this.props.onInputValueChange(newStateToSet.inputValue, {
      ...this.getStateAndHelpers(),
      ...newStateToSet,
    })
  }
}

This will allow me to continue to rely on the downshift state as a source of truth (I am manipulating the selectedItem of a multi-select to always be an array)

If there is no other considerations that I may have missed with this fix then I can submit a PR.

aaronnuu avatar Nov 07 '19 06:11 aaronnuu

I am pretty sure you can handle your multi-select use case without any change in Downshift. Usually for a multi-select you set selectedItem as null and control the values yourself from your multiple component, while Downshift will believe it's combobox is always empty.

I think Kent's examples repo has a multiple select example that works great.

silviuaavram avatar Nov 13 '19 13:11 silviuaavram

Yeah I definitely can, if I do that I can't access the selectedItems value in the state reducer though which is handy in some situations.

I still think this is a bug, to me it doesn't make sense to have a stateAndHelpers param that doesn't actually reflect the state I want, especially when it's being done correctly ~20 lines below, so depending on how internalSetState is called you get different values in the onInputValueChange function.

It has a couple of other implications too like if you for any reason want to change the inputValue in your state reducer when it wouldn't normally, because of this line

if (!isStateToSetFunction && stateToSet.hasOwnProperty('inputValue')) {

the onInputValueChanged function won't be run when it really should be, and inversely if you remove the inputValue in the state reducer the function will be run when it shouldn't.

aaronnuu avatar Nov 14 '19 00:11 aaronnuu

I will come back to this once I go through with useCombobox and some other tasks, as this probably sounds logical. It will be a breaking behavior, that is why I want to make sure I understand the implications.

silviuaavram avatar Dec 12 '19 11:12 silviuaavram