search

algolia / instantsearch

⚡️ Libraries for building performant and instant search and recommend experiences with Algolia. Compatible with JavaScript, TypeScript, React and Vue.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/
MIT License
3.72k stars 523 forks source link

useInfiniteHits does not reset its current page when filters change. Also has out of sync data when useInstantSearch().refresh() is called #5410

Open jpepper-wistia opened 1 year ago

jpepper-wistia commented 1 year ago

🐛 Current behavior

Intended use cases

I want to use useInfiniteHits() and have it correctly reset its data when:

Problems

Refreshing Data with useInfiniteHits()

When calling refresh from useInstantSearch to hard refresh data from server after user update action:

Expected Behavior:

RESOLVED: Changing Filters does not reset useInfiniteHits() current page number

When updating the selected filters object for the InstantSearch / Configure element's filters prop, useInfiniteHits does not reset its state correctly:

NOTE: This was observed when providing an "empty" search string (in order to only leverage filters without text based search)

Expected Behavior:

🔍 Steps to reproduce

Use a custom search client

E.g.

import { InstantSearch, Configure } from 'react-instantsearch-hooks';

const customSearchClient = (props) => {
  return {
    search: async (requests, analytics) => foo({context, requests, analytics})
  }
}

const AlgoliaWrapper = (props) => {
   return (
      <InstantSearch
          indexName=""
          searchClient={searchClient}
          suppressExperimentalWarning={true}
    >
      <Configure
        filters={filters}
        facets={['*']}
      />
      {children}
    </InstantSearch>
    )
}

Use useInfiniteHits hook


import { useInfiniteHits, useInstantSearch } from 'react-instantsearch-hooks';

const Parent = () => {
    return (
    <AlgoliaWrapper>
         <ExampleComponent/>
    </AlgoliaWrapper>
    )
}

const ExampleComponent = () => {
   const { hits, results } = useInfiniteHits();

   const { refresh } = useInstantSearch();

   return (
      <Foo
          data={hits}
          // e.g. trigger after some manual user action (e.g. deleting or updating data)
          onDataUpdate={()=> refresh()}
       />
   )
}

Live reproduction

none (see code snippet above)

💭 Expected behavior

Manually "refreshing" data

Fetching updates results with changed filters and an empty search string

Package version

react-instantsearch-hooks 6.32.1

Operating system

macOs

Browser

Chrome

Code of Conduct

jpepper-wistia commented 1 year ago

Update:

I was able to resolve the 2nd bug where page number would not reset when filters change by manually specifying page={0} as part of the Configure parent element.

const Foo = () => {
return (
  <InstantSearch {...props...}>
     <Configure
          filters={someFilters}
          page={0} // this fixed `useInfiniteHits()` for me resetting the page number correctly when filters change
     />
     {children}
  </InstantSearch>
 ) 
 }

I am still running into a similar issue as https://github.com/algolia/instantsearch/issues/5263 when trying to refresh the data in useInfiniteHits

Haroenv commented 1 year ago

Regarding the issue you have posted an update about, Configure doesn't automatically reset the page, however the refine functions from useMenu/useRefinementList do reset the page. This is because Configure is aimed at parameters that aren't visible to the user, and thus shouldn't reset any other parameters.

Your other issue is already tracked in #5263, we indeed don't have a solution for this today, as the behaviour isn't completely clear. InfiniteHits keeps track internally of all the "seen" pages. If you refresh, should the page become 0 as well, or should you stay at page N and clear the previous pages? Today only the "last seen page" gets an update, as refresh will only update that one.

What's the exact behaviour you'd expect for InfiniteHits?

jpepper-wistia commented 1 year ago

Thanks for the response @Haroenv !

Regarding expected behavior for InfiniteHits, we are using it with an "infinite scroll" UX pattern (not pagination) which should help avoid some of that complexity you listed for our purposes.

With infinite scroll in mind, we just need a way to "reset" the full state useInfiniteHits() hook is using so that it can:

We believe this should allow us to correctly show filtered results depending on what data update actions a user performed.

Example Scenario A:

Example Scenario B:

Example Scenario C:

Our intended approach

What we've tried

/// consuming code const Foo = () => { const cache = useMemoryCache(); const data = useInfiniteHits( { cache }); const { refresh } = useInstantSearch();

return ( <Bar onUserUpdate={() => { // NOTE: Also tried reversing these cache.clear(); refresh(); }) /> ) }

* A combination of a custom `cache` with `useInfiniteHits` as well as `setUiState()` from `useInstantSearch`
   * As suggested as part of this comment https://github.com/algolia/instantsearch/issues/5263#issuecomment-1363062010
   * This seems to run into race conditions as the behavior is unpredictable
E.g.
```tsx
function useMemoryCache() {
  return useMemo(() => {
    let cachedHits;
    let cachedState;
    return {
      read({ state }) {
        return isEqual(cachedState, getStateWithoutPage(state)) ? cachedHits : null;
      },
      write({ state, hits }) {
        cachedState = getStateWithoutPage(state);
        cachedHits = hits;
      },
      clear() {
        cachedHits = undefined;
        cachedState = undefined;
      },
    };
  }, []);
}

const INDEX_NAME = 'SomeIndex';
/// consuming code
const Foo = () => {
  const cache = useMemoryCache();
  const data = useInfiniteHits( { cache });
  const { setUiState } = useInstantSearch();

  return (
    <Bar
        onUserUpdate={() => {
           // NOTE: Also tried reversing these
           setUiState((state) => {
              state[INDEX_NAME].page = 0;
              state[INDEX_NAME].configure.page = 0;
              return state;
            });
           cache.clear();
        })
    />
  )
}

Do you have any suggestions on how to manually force "reset" useInfiniteHits to a default state (page 0 and fresh results for that page)?

This is a major blocker for us as it prevents supporting user update/delete actions from views using Algolia results that require an infinite scroll.

Haroenv commented 1 year ago

I believe the behaviour you're described may be fixed by combining setUiState (or setIndexUiState) combined with refresh as well as cache.clear, for good measure

I'd recommend trying this:

function createMemoryCache() {
    let cachedHits;
    let cachedState;
    return {
      read({ state }) {
        return isEqual(cachedState, getStateWithoutPage(state)) ? cachedHits : null;
      },
      write({ state, hits }) {
        cachedState = getStateWithoutPage(state);
        cachedHits = hits;
      },
      clear() {
        cachedHits = undefined;
        cachedState = undefined;
      },
    };
  }
}

const INDEX_NAME = 'SomeIndex';
/// consuming code

const cache = createMemoryCache();
const Foo = () => {
  const data = useInfiniteHits( { cache });
  const { setUiState, refresh } = useInstantSearch();

  return (
    <Bar
        onUserUpdate={() => {
           setUiState((state) => {
              state[INDEX_NAME].page = 0;
              state[INDEX_NAME].configure.page = 0;
              return state;
            });
           cache.clear();
           refresh();
        })
    />
  )
}

Note that you'll need to be careful to pass the memory cache to _every instance of useInfiniteHits/InfiniteHits__ you have on the page.

jpepper-wistia commented 1 year ago

@Haroenv Thanks for the suggestion.

I attempted that approach and I'm seeing some unpredictable behavior with that solution.

Observed behavior:

I added some console logs to see whats going on:

function createMemoryCache() {
  console.log('\t\t\t\t-CREATE MEMORY CACHE---');
  let cachedHits;
  let cachedState;
  return {
    read({ state }) {
      console.log('\t\t\t-MEMORY CACHE: READ-state', cachedState);
      console.log('\t\t\t-MEMORY CACHE: READ-cachedHits', cachedHits);
      return isEqual(cachedState, getStateWithoutPage(state)) ? cachedHits : null;
    },
    write({ state, hits }) {
      console.log('\t\t\t-MEMORY CACHE: WRITE-state', state);
      console.log('\t\t\t-MEMORY CACHE: WRITE-cachedHits', hits);
      cachedState = getStateWithoutPage(state);
      cachedHits = hits;
    },
    clear() {
      console.log('\t\t\t-MEMORY CACHE: CLEAR');
      cachedHits = undefined;
      cachedState = undefined;
    },
  };
}

Observed Scenario Example

Here's what its showing in the console:

image

From debugging the createMemoryCache here's what I've observed

Any thoughts on how to resolve the unpredictable behavior with the state param thats being sent to the cache.write function?

jpepper-wistia commented 1 year ago

Follow up: It looks like that unpredictable stale state is coming from connectInfiniteHits -> getRenderedState -> renderOptions param has the state referencing an older page when then gets rewritten to the cache.write after its cleared.

Maybe it's some kind of race condition / sequencing of events?

Haroenv commented 1 year ago

Do you have this exact scenario in a sandbox (or even better a test) so we can look into what the solution would be?

As a workaround before this is fixed you could take isCleared in account in the cache:

function createMemoryCache() {
  console.log('\t\t\t\t-CREATE MEMORY CACHE---');
  let cachedHits;
  let cachedState;
  let isCleared;

  return {
    read({ state }) {
      if (isCleared) return null;
      console.log('\t\t\t-MEMORY CACHE: READ-state', cachedState);
      console.log('\t\t\t-MEMORY CACHE: READ-cachedHits', cachedHits);
      return isEqual(cachedState, getStateWithoutPage(state)) ? cachedHits : null;
    },
    write({ state, hits }) {
      console.log('\t\t\t-MEMORY CACHE: WRITE-state', state);
      console.log('\t\t\t-MEMORY CACHE: WRITE-cachedHits', hits);
      cachedState = getStateWithoutPage(state);
      cachedHits = hits;
      isCleared = false;
    },
    clear() {
      console.log('\t\t\t-MEMORY CACHE: CLEAR');
      cachedHits = undefined;
      cachedState = undefined;
      isCleared = true;
    },
  };
}
jpepper-wistia commented 1 year ago

Hello @Haroenv, thanks for following up.

I tried the solution above, but we did not have success with adding the isCleared logic to the cache either.

We do not have this scenario setup in a sandbox currently to share; however, I can add this as a note on our team to follow up on if its required to investigate a solution.

sbkl commented 1 year ago

the useInfiniteHits hooks from "react-instantsearch-hooks-web" seems to have the right behaviour while "react-instantsearch-hooks" used in react native doesnt. Any solution on this?

Haroenv commented 1 year ago

@sbkl, there's no difference in behaviour between React InstantSearch Hooks Web and React InstantSearch Hooks with relation to this. Do you have a sandbox or other code sample to clarify what doesn't work for you? Maybe in a new discussion