algolia / react-instantsearch

⚡️ Lightning-fast search for React and React Native applications, by Algolia.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/react/
MIT License
1.97k stars 386 forks source link

[react-instantsearch-hooks] Re-rendering components breaks initialUiState when using RefinementList with transformItems #3541

Closed wintersieck closed 1 year ago

wintersieck commented 2 years ago

🐛 Bug description

If you set both initialUiState on InstantSearch and transformItems on RefinementList, triggering a re-render to the component will cause it to ignore what you set for initialUiState.

🔍 Bug reproduction

Minimal reproduction in remix: https://github.com/wintersieck/algolia-instantsearch-hooks-issue

This is a basic remix app from npx create-remix, with Algolia packages installed and a basic setup in app/routes/index.tsx. We force a re-render 2 seconds after the page loads to demonstrate the issue.

💭 Expected behavior

initialUiState to set the initial state of the UI even through a re-render, since any number of things can trigger an initial re-render (in our case, it's useTranslation from react-i18next).

🖥 Screenshots

Initial state: initial state

After the component re-renders: after rerender

Environment

Thank you!

Haroenv commented 2 years ago

This is caused by the transformItems function not being stable, if you make it a reference which is stable by for example creating it outside of the component will not cause the component to reinitialise.

https://github.com/wintersieck/algolia-instantsearch-hooks-issue/blob/64400c83446c0398f77c8e9128453af5d451467d/app/routes/index.tsx#L42-L47

The underlying reason this happens is because most parameter changes cause a different request to be needed, and that's implemented by removing and starting the widget again, and this loses the refinement

wintersieck commented 2 years ago

Thanks for the response! It makes sense what causes it, but this seems like a difficult limitation because we then can't update the transformItems callback (for example, if the users changes the language they're looking at and we need to translate each item differently).

Removing and starting the widget again seems workable as long as it continues to pay attention to initialUiState when it restarts (which it doesn't seem to currently). That way we can keep initialUiState updated on our side so that it re-renders with the correct state when transformItems changes. Unless there's another way to set the UI state consistently from our side?

Thanks for your help!

Haroenv commented 2 years ago

The problem is that the widget clears up its state when it is unmounted (as you wouldn't want a refinement list that's no longer mounted to have an effect on the parameters, or manually do a cleanup).

I wonder if you could setUiState at the same time as updating your function to set the original state before the widget toggled again?

francoischalifour commented 2 years ago

In that case I would wrap transformItems in a useCallback() with the language as dependency. It would keep the initialUiState value across renders, and rerender when the language changes.

I created a sandbox to show you how it'd work.

Notice that there is a network request sent when changing the language for the first time, because the initial network request was sent from the server, and not cached on the browser. This is an improvement that we want to work on in our SSR APIs. We currently skip the initial network request on the browser, but we don't add it to the search client cache.

Edit: It appears that the refinement is still lost after the language change. We should probably apply a similar strategy to #3514 for <InstantSearch> so that it doesn't lose refinements on remounts.

francoischalifour commented 2 years ago

As @Haroenv mentioned, the reason why it can't work OOTB right now is because a prop change removes the widget, and then adds it back. The underlying InstantSearch.js library doesn't yet provide an Update API.

So that you don't lose the initial refinement, you can create a selector component that's in charge of re-applying the initial UI state on change.

function Selector({ value, onChange, children }) {
  const { uiState, setUiState } = useInstantSearch();

  return (
    <select
      value={value}
      onChange={(event) => {
        onChange(event.currentTarget.value);

        setTimeout(() => {
          setUiState(uiState);
        }, 0);
      }}
    >
      {children}
    </select>
  );
}

Check the full example in this sandbox. Notice that the refinement is kept when you change language. As you can see though, that's not optimal because it triggers an additional network request: the temporary one without the initialUiState applied.

FabienMotte commented 1 year ago

@wintersieck Did @francoischalifour's replies help you out?

We're doing a round of cleanup before migrating this repository to the new InstantSearch monorepo. This issue seems not to have generated much activity lately, so we're going to close it, feel free to reopen it if needed.