fix(ComboBox): moving filtered options out of state

[jpuri]

Fix for #162

[bvleur]

👍 This fix seems to work for me. Thank you.

[jpuri]

😄 great

[nikgraf]

Looks good to me.

@bvleur & @jpuri The purpose of having such properties in state is that every time it changes the render function would be called. Would it be possible that we miss a render (which should happen) due this change?

[jpuri]

Hey @nikgraf ,

That should not happen, coz filteredOptions are derived from other parameters in state and props. Check my comments here #162 .

@bvleur was facing this issue: "If you arrive at a input value matching an option by pressing backspace, the isMatchingOption won't be set to true."

The reason for the issue was that I was updating filteredOptions in state using setState() method and immediately after that I was using this.state.filteredOptions. setState is asynchronous and I was getting old value. I could have solved that in other ways. But I was thinking that filteredOptions are really not needed in state, I was always confused about this - so I moved them out and now its working fine.

[bvleur]

Would it be possible that we miss a render (which should happen) due this change?

filteredOptions is a derivative of some props and state. It's value should be a pure function of value, children, filterFunc and maxOptions.

We could even make that explicit in the code by moving from a filteredOptions variable to a getFilteredOptions()-function. For performance reasons you should probably memoize that function.

If the statement above is correct then any change to the input of that function might cause filteredOptions to change and should cause a re-render. But because it's input is from state and props changes already cause the component to re-render, so we won't miss a render.

[jpuri]

We already know specific places where filteredOptions can change, so I think calling function at just those places might be more performant than memoize and calling at each render.

Mostly its called only when inputValue changes and in that case re-filtering is actually needed.

[bvleur]

The reason I mentio the getFilteredOptions and memoizing alternative is mostly to explain how you could see it as derived state and why I think it is correct to not have it trigger a rerender by itself.

The alternative will probably only be slower if the "is it cached"-check is particulary heavy (which I don't think it is) but there is not real need to actually go that route, given that the current implementation (with this fix) works fine.

[nikgraf]

Sorry for the late reply! I see what you mean. Good work!