[EuiSearchBar] use EuiFieldSearch as controlled input

[nreese]

There is a race condition in EuiSearchBar where sometimes user input is entered before the last onSearch method call can update queryText. This results in a confusing user experience because the input looses characters. For example, the user may type redis but the input will show reds. I am consistently seeing this while running functional tests in Kibana that type long strings in the search bar.

This PR removes a couple of performance problems like using bind for property values. The PR removes the race condition by updating queryText before calling parse, ensuring that user input is always captured.

In search_box, the query was getting passed to EuiFieldSearch twice, once via defaultValue and then again in componentWillUpdate. The PR only passes the initial query value as defaultValue and all other updates are done via componentWillUpdate

[nreese]

It looks like the problem has more to do with the fact that EuiFieldSearch is uncontrolled and that is allowing the input to get out of sync.

The latest commit updates EuiFieldSearch so it can be used as a controlled input and updates EuiSearchBox to use EuiFieldSearch as a controlled input.

[stacey-gammon]

I'm going to play around with this some more to make sure my feedback is accurate, but am going to send this out now in case you can correct me.

Also I'm not sure if this is the bug you are running into, but search results can come back in a different order. I ran into this in the visualize listing table (https://github.com/elastic/kibana/blob/master/src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js#L60).

[stacey-gammon]

If we don't want to add a debounce in this component, or a way to pass one in, should still use one in dashboard listing table (haven't gotten to that PR so maybe you have already!). Just so we don't send a request for every key press.

[nreese]

debounce is handled by the consumer (like dashboard listing table). The neat thing is that search only hits the server when the number of dashboards exceeds the listingLimit. If that limit is not breached then all search stays local on the client.

[stacey-gammon]

Is the order of onKeyUp and onChange guaranteed? I think onKeyUp occurs first and onChange occurs second, when the input box blurs.

If so, I don't think this is going to work right (though haven't tested it so could be wrong). If the search box is initialized with value a, then the user presses b, won't onSearch be called with a, not ab, but once onChange is fired, the search box will show ab so the results won't match what's in the input box?

Also, do we really need a new onChange prop? Just seems to complicate this component. Which one should I use, onKeyUp, onSearch, or onChange? Some propTypes with descriptions might help.

One more thing to note is that user can paste text into the search box by right clicking which means onKeyUp won't get fired.

[nreese]

When the input is controlled, onKeyUp event.target.value is undefined since react is now tracking the value. This puts us in the weird situation where when the component is controlled, onChange has to be used to track the input value and onKeyUp has to used to submit onSearch. I do not think we can get around this and the input must be controlled.

In Chrome, onChange is getting called before onKeyUp.

[stacey-gammon]

What's the reason to skip this if value is null or undefined?

[nreese]

It causes react to complain when defaultValue is passed because then the input is uncontrolled and controlled.

[cjcenizal]

@nreese It might be a good idea to leave your explanation as a comment here for the next person who asks the same question.

[cjcenizal]

LGTM! Just had some suggestions about comments and tests.

[cjcenizal]

Can we add a verbose comment here to explain the intention behind organizing the logic like this? Here's my suggestion based on your PR description:

/**
 * We set `queryText` in one phase in this callback and then set subsequent derived state in a
 * second phase in `handleQueryTextChange`. This ensures the UI updates with all of the user's
 * input. Otherwise we could get into a race condition in which the parsing logic is still executing
 * while the user types rapidly, resulting in a UI which doesn't contain characters that the user
 * has typed.
 */

this.setState({ queryText }, this.handleQueryTextChange);

[cjcenizal]

Can we add tests for onKeyUp, onChange, and onSearch? Because we're intercepting the first two callbacks it will be good to protect against a breaking change to their behavior, and the last one is a new callback which needs documentation.