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 522 forks source link

`connectStateResults` does not provide up-to-date values in server side rendering #5255

Open ksntcrq opened 3 years ago

ksntcrq commented 3 years ago

Describe the bug 🐛

In some cases, connectStateResults does not provide the correct up-to-date values.

To Reproduce 🔍

Unfortunately, it's not possible for me to give a working example here. But the flow is as follow:

Expected behavior 💭

connectStateResults should always provide the latest values. There should not be a difference between the provided searchResults.hits and the hits provided by connectHits, for instance. Nor there should be a difference between the provided searchState.query and searchResults.query.

Environment:

Additional context

We're using the latest 6.10.0 version of both react-instantsearch-core and react-instantsearch-dom.

We're using the following connectors:

We're also using server-side rendering, click analytics, and conversion analytics. Here is how the configure component looks like:

        <Configure
            optionalFilters={optionalFilters}
            filters={filters}
            queryLanguages={queryLanguages}
            hitsPerPage={hitsPerPage}
            removeWordsIfNoResults="lastWords"
            clickAnalytics
            analytics
        />

The search is performed on a single index, correctly passed to InstantSearch.

ksntcrq commented 3 years ago

Unsure if this will help, but it feels like these two issues are kind of related?

Haroenv commented 3 years ago

Hey @ksntcrq, do you have a sandbox with this behaviour? Is it not happening when you use 6.9.0? Thanks!

Haroenv commented 3 years ago

looks like it's a duplicate of algolia/react-instantsearch#3018 indeed. Feel free to give more information on your problem there, and use 6.9.0 in the mean time

ksntcrq commented 3 years ago

@Haroenv It does happen on 6.9.0, as well as 6.8.2. Could you please reopen?

Haroenv commented 3 years ago

I misunderstood the issue then, sorry!

https://github.com/algolia/react-instantsearch/pull/2953 sounds similar to the issue you're having

Haroenv commented 3 years ago

I've been trying to reproduce this the last couple of days, but always come short of touching the actual problem. Eg. I made https://codesandbox.io/s/charming-nash-cq896?file=/src/App.js, but there I always see searchResults being updated and a render using searching: false.

To debug this properly, I'd love to see a sandbox that shows this behaviour

I'm not sure what causes the bug yet, but in the mean time, you could use connectHits and connectSearchBox instead of connectStateResults in your use case if you see the data not being correct.

ksntcrq commented 3 years ago

I sent a working version in private to @mathougui, let me know if you need more details.

The component using searching also uses query and nbHits from searchResults, so I'm afraid I can't only use connectHits and connectSearchBox, but I'll try to find a workaround with this idea in mind. Thanks!

ksntcrq commented 3 years ago

The issue seems to be introduced as soon as we're using connectHitInsights. Because we have server-side rendering, we do it this way:

export default connectHitInsights(getAlgoliaAnalytics())(CustomHit);
export function getAlgoliaAnalytics() {
    if (isServerSide()) {
        return null;
    }

    if (isEmpty(window.aa)) {
        return null;
    }

    return window.aa;
}
ksntcrq commented 3 years ago

I managed to reproduce here. Just type two letters in the input, and see the outdated log in the console.

Haroenv commented 3 years ago

I simplified the example and found the following things that matter: https://codesandbox.io/s/stoic-hermann-98bx9?file=/pages/index.js

  1. stateResults behind hits has the issue, before it doesn't
  2. a hitComponent must be given in default Hits component

For your specific use case, can you try putting StateResults in front of Hits?

ksntcrq commented 3 years ago

Changing the order seems to be working. Thanks!

Haroenv commented 3 years ago

Found some more things: https://codesandbox.io/s/billowing-browser-gd3kn?file=/src/App.js