Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

Autocomplete, empty query and slow network. #465

Closed
mthuret opened this issue Oct 11, 2017 · 13 comments
Closed

Autocomplete, empty query and slow network. #465

mthuret opened this issue Oct 11, 2017 · 13 comments

Comments

@mthuret
Copy link
Contributor

mthuret commented Oct 11, 2017

Context:

You have an autocomplete using the connectAutoComplete connector.

  1. You type one query "a"
  2. You removed that query
  3. You type another query "b"

On 3, the result of the query "a" or the result of empty query depending of the configuration of your autocomplete will appear first, and then you'll have b. That's producing a flash, and more importantly can be disturbing with slow network.

autocomplete-empty-query

The reason behind this is that we do not reset hits to [] each time we perform a query, therefore when doing a new one, you first have the results of the previous query inside the props. This become a problem in case you hide some of the results like here if there's no query entered by the user.

See:
https://discourse.algolia.com/t/react-instantsearch-w-autosuggest-unwanted-initial-hit-results-displaying/2495
https://discourse.algolia.com/t/react-instantsearch-v4-1-3-is-out-tada/3167/4

Goal:

You can make some condition yourself to avoid this behaviour like I did here https://codesandbox.io/s/k9p6y7zllr taking advantage of the componentWillReceiveProps lifecycle method. At the bare minimum we should add this in the autocomplete guide.

I'm also wondering if we could do something inside the library that can prevent this.

What do you think?

@oyeanuj
Copy link

oyeanuj commented Dec 8, 2017

@mthuret Also facing this. It seems that the workaround in the codesandbox would work only for initial results but not on change of query.

Is it possible to either empty the hits (say, if optedIn) or have a status like isRefined which would be false when the hits aren't for the currentRefinement? This way, users could tailor their behavior - choose to show results regardless and suffer the flicker or show only accurate results and have a second of no results?

@samouss
Copy link
Collaborator

samouss commented Dec 10, 2017

The proposed workaround doesn't work depending on how you trigger the refine & setState calls. I have done a jsFiddle example showing the different downsides of the proposed approach.

Context:

  1. You type one query "p"
  2. You removed that query
  3. You type another query "m"

On the first example, everything works. On the second & third example you can observe a blink on the list when the input value pass from empty to something. If you have trouble to see the blink you can emulate a slower connection.

Explaination:

  • With plain state: this is the workaround used in the CodeSandbox. But it only works thanks to a race condition. When the lifecycle componentWillReceiveProps is called the state is not correctly set. Since setState has been called on the input change event the state should be "p" instead of "". But it's async by nature, so in that case the workaround works because when the hook is called the state is still an empty string.

  • With state updater: in this situation we will trigger the setState call with a state updater in componentWillReceiveProps. As we seen in the previous example setState is called on the input change event. So the previous state inside the updater is now correct instead of having "" we will have "p" which is the current state. Then condition is executed and the component will be render the hits of the previous search.

  • With callback state: in this situation, we call refine from the setState callback after that we are sure that the state has been correctly update. So even if we don't use the state updater in componentWillReceiveProps the state is still correct. As in the previous example the hits passed to the components are the one of the previous search.

@oyeanuj
Copy link

oyeanuj commented Dec 15, 2017

@samouss @mthuret Running into a similar problem with connectInfiniteHits. I'd rather show a skeleton loading state while results are being fetched than flicker and back but the connector doesn't pass any loading state.

In both these cases, thoughts on adding an isFetching state?

@Haroenv
Copy link
Contributor

Haroenv commented Dec 15, 2017

@oyeanuj, that should be covered by #544 I think

@oyeanuj
Copy link

oyeanuj commented Dec 15, 2017

@Haroenv So, I think #544 only covers providing a loading component and isSearchStalled (which I am not sure is the same as loading) to SearchBox and connectSearchBox.

@bobylito can confirm?

I think ideally the loading status should be provided to connectHits, connectInfiniteHits, and connectAutoComplete. With that field, I feel like this ticket would have a decent workaround as well.

@oyeanuj
Copy link

oyeanuj commented Jan 8, 2018

Hi folks - just following up to see if there is any update here or thoughts on what a workaround or solution might be? At the moment, it is the biggest missing element for a smooth experience on our app.

Thanks for all of your work on the library!

@samouss
Copy link
Collaborator

samouss commented Jan 8, 2018

Hi @oyeanuj, sorry for the late answer.

You are right the isSearchStalled is not the same as loading, this indicator is pass to true when search as the name suggest is "stalled". It happens after a given delay that can be configured on the InstantSearch component. You can find more informations about that in our documentation.

IMO we don't need to pass additional props to connectHits, connectInfiniteHits, and connectAutoComplete since we can compose them with connectStateResults. The latter give you access to the current state of the search, you can find a boolean indicator searching on it. So you can use this information for display your skeleton.

Here is a quick example that hide the suggestions when a search is in progress:

const AutoComplete = ({ currentRefinement, hits, searching, refine }) => (
  <div>
    <input
      value={currentRefinement}
      onChange={event => refine(event.currentTargert.value)}
      placeholder="Type a product"
    />
    <div hidden={searching}>
      {currentRefinement && (
        <ul>{hits.map(hit => <li key={hit.objectID}>{hit.name}</li>)}</ul>
      )}
    </div>
  </div>
);

const AutoCompleteConnected = connectAutoComplete(connectStateResults(AutoComplete));

@rtman
Copy link

rtman commented Jan 21, 2018

@samouss I'm finding the above method works, but causes additional flickering due to searching seeming to turn on and off a few times during typing in a search. Is there anyway to smooth this out?

@oyeanuj
Copy link

oyeanuj commented Jan 26, 2018

@samouss thank you for the workaround! and i had the same experience & thought as @rtman's comment above.

@samouss
Copy link
Collaborator

samouss commented Feb 6, 2018

Hi @oyeanuj @rtman

We found another solution without the flickering effect. At the end what we want to display is only the results that match the currentRefinement. Inside the searchResults we have access to the state of the query. So we can check if the currentRefinement match the query of the current searchResults.

Here is an example on CodeSandbox with & without the condition, it's easier to see the difference with a network throttling. On the first example the previous results is immediately displayed when we start typing. But on the second one the results appear only when the currentRefinement match the query of the current searchResults.

Let me know if it works for your use case.

class AutoComplete extends Component {
  state = {
    hits: this.props.hits
  };

  componentWillReceiveProps(nextProps) {
    const { currentRefinement: nextRefinement, hits, searchResults } = nextProps;
    const { currentRefinement } = this.props;

    const hasMatchingResults =
      !!searchResults &&
      !!searchResults.query &&
      currentRefinement.startsWith(searchResults.query);

    this.setState({
      hits: hasMatchingResults && !!nextRefinement ? hits : []
    });
  }

  render() {
    const { currentRefinement, refine } = this.props;
    const { hits } = this.state;

    return (
      <div>
        <input
          value={currentRefinement}
          onChange={event => refine(event.currentTarget.value)}
          placeholder="Type a product"
        />

        <ul>
          {hits.map(hit => (
            <li key={hit.objectID}>
              <Highlight attributeName="name" hit={hit} />
            </li>
          ))}
        </ul>
      </div>
    );
  }
}

const AutoCompleteConnected = connectAutoComplete(connectStateResults(AutoComplete));

@oyeanuj
Copy link

oyeanuj commented Aug 21, 2018

@samouss Two question here: Do you think the currentRefinement could ever be made available within connectStateResults? Currently, I have my search box in a different component connected with connectSearchBox, and results in a different sibling component, and at every place there is little logistical tediousness connecting the two components.

Also, any other thoughts on a solution for this problem?

@samouss
Copy link
Collaborator

samouss commented Aug 21, 2018

@oyeanuj I'm not sure to fully understand the issue. Could you provide more context or even an example? It will helps a lot! Here is the link for the React InstantSearch template.

@sarahdayan
Copy link
Member

This should be fixed now.

Also today, we recommend using the Autocomplete library instead of connectAutocomplete.

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

No branches or pull requests

6 participants