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.

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

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

Closed
aaronnuu opened this issue Nov 7, 2019 · 3 comments

Comments

@aaronnuu
Copy link

aaronnuu commented Nov 7, 2019

  • 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.

@silviuaavram
Copy link
Collaborator

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.

@aaronnuu
Copy link
Author

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.

@silviuaavram
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants