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

In some cases there are multiple identical requests #355

Closed
Haroenv opened this issue Sep 20, 2017 · 30 comments
Closed

In some cases there are multiple identical requests #355

Haroenv opened this issue Sep 20, 2017 · 30 comments

Comments

@Haroenv
Copy link
Contributor

Haroenv commented Sep 20, 2017

Do you want to request a feature or report a bug?

bug

Bug: What is the current behavior?

a single refinement (any of them) cases N requests with the exact same parameters

Bug: What is the expected behavior?

one request per exact same parameters

What is the version you are using? Always use the latest one before opening a bug issue.

4.0.10

This is visible in

As discussed with @vvo, we should deduplicate requests either at the React InstantSearch or Helper level, since this might be hard to solve otherwise.

@mthuret
Copy link
Contributor

mthuret commented Sep 20, 2017

I think we can go for the deduplicate requests solutions only after understanding what is causing the bug (even though we find out that we don't have better solutions than deduplicating requests for now).
cc @Haroenv @vvo

@vvo
Copy link
Contributor

vvo commented Sep 21, 2017

Finding the root cause is a priority, as for fixing like you said it can depend on our strategy (inside RIS, or inside the helper). Still the helper could be smarter on this part (pending request, same parameter, reuse request).

@callmephilip
Copy link

Seeing this issue as well and it's messing things up quite a bit for our analytics component. Do you guys have any plans to fix this in the near future?

@Haroenv
Copy link
Contributor Author

Haroenv commented Nov 27, 2017

We are working on this right now, in the meantime any situations in which this occurs will help us pinpoint the issue. Can you make a reproduction on for example CodeSandbox @callmephilip ?

@callmephilip
Copy link

@Haroenv i don't think i can provide an easily reproducible example - the app is closed source. those multiple requests seem to fire when refinements are adjusted - we are using standard RefinemnentList components from RIS, nothing funky

@Haroenv
Copy link
Contributor Author

Haroenv commented Nov 27, 2017

A refinement list will always send two requests, one with analytics: false in it because one is to get the possible values, and the other for the number of results for that. Which analytics are you rolling out @callmephilip, the built-in Algolia ones?

In any case, this issue is for cases when two duplicate requests are done on every change, not just on refinementList

@callmephilip
Copy link

I am seeing 2 requests with analytics: false when updating refinements
re:analytics - we rolled a simple widget and a connector responsible to reporting search results being fetched to Google Tag manager. The connector looks smth like this:

import { createConnector } from 'react-instantsearch';

export const connectAnalytics = createConnector({
  displayName: 'AlgoliaAnalytics',

  getProvidedProps(props, searchState, searchResults) {
    // inspect search results here and report to GTM
   return props;
  },
});

@callmephilip
Copy link

The requests sent seem identical:

{"requests":[{"indexName":"development_products","params":"query:
hitsPerPage:50
maxValuesPerFacet:10
page:0
highlightPreTag:<ais-highlight-0000000000>
highlightPostTag:</ais-highlight-0000000000>
facets:["make","model","type","year","price","color"]
tagFilters:
facetFilters:[["make:BMW"]]"},{"indexName":"development_products","params":"query=
hitsPerPage:1
maxValuesPerFacet:10
page:0
highlightPreTag:<ais-highlight-0000000000>
highlightPostTag:</ais-highlight-0000000000>
attributesToRetrieve:[]
attributesToHighlight:[]
attributesToSnippet:[]
tagFilters:
analytics:false
facets:make"}]}

And

{"requests":[{"indexName":"development_products","params":"query:
hitsPerPage:50
maxValuesPerFacet:10
page:0
highlightPreTag:<ais-highlight-0000000000>
highlightPostTag:</ais-highlight-0000000000>
facets:["make","model","type","year","price","color"]
tagFilters:
facetFilters:[["make:BMW"]]"},{"indexName":"development_products","params":"query=
hitsPerPage:1
maxValuesPerFacet:10
page:0
highlightPreTag:<ais-highlight-0000000000>
highlightPostTag:</ais-highlight-0000000000>
attributesToRetrieve:[]
attributesToHighlight:[]
attributesToSnippet:[]
tagFilters:
analytics:false
facets:make"}]}

@mthuret
Copy link
Contributor

mthuret commented Nov 27, 2017

Hi @callmephilip

I have some questions: are you using InstantSearch in controlled mode? If yes, are you directly modifying the searchState of InstantSearch (by changing refinements of the RL?). Or are you changing the RL props?

A temporary workaround that can be used when dealing with multiple request until we find the underlying issue is to wrap the IS component and use the shouldComponentUpdate hook to remove the unwanted rendering.

@callmephilip
Copy link

callmephilip commented Mar 14, 2018

@mthuret

I have finally got a chance to start looking into this in more depth and I am definitely seeing some strange behaviors. Here are a few more points that might provide additional context:


"react-instantsearch": "^4.5.1"
"react-instantsearch-theme-algolia": "^4.5.1"

@callmephilip
Copy link

callmephilip commented Mar 14, 2018

I've just upgraded to 5.0.1 and I am still seeing the issue. I am open to switching off the controlled mode and trying to figure out an alternative filtering modal approach (e.g. css based so that the refinements do not get unmounted) but we still will require to control the search state for routing and will be hitting the same issue

@samouss
Copy link
Collaborator

samouss commented Mar 15, 2018

Thanks for those information, I understand that your app is a closed source. But having a small reproduction could help us to investigate more closely the issue. You can find a template in CodeSandbox.

@callmephilip
Copy link

@samouss you can find a mini reproduction here: https://codesandbox.io/s/jj3990ry3

To my best understanding, whenever we were trying to use a wrapped component inside the InstantSearch component, this would trigger multiple searches. In the example above we have a simple search box extension to support random place holders. I am not 100% sure if it has something to do with the lib we are using (https://github.com/arunoda/react-komposer) - I have not tried this with any other compositions. Hopefully this is enough of a starting point to help us in the investigation

@samouss
Copy link
Collaborator

samouss commented Mar 19, 2018

Thanks, it will help us a lot! I will take a look in the coming days.

@samouss samouss removed the RIS only label Mar 23, 2018
@samouss
Copy link
Collaborator

samouss commented Mar 27, 2018

Hi @callmephilip, I looked at your example.

The issue come from the SearchBox component and the random placeholder generation. With the current implementation of React InstantSearch when a connector receive new props it will perform a shallow comparison between the previous & the next props. When the props are different the component schedule a new request. It's known issue on our side here is the issue.

Back to your example, the SearchBox will receive a new random translation on every render. It means that every time the translation will change a new request will be schedule. Here is an example that set always the same translation, you can notice that the request are not doubled. You can assign a placeholder at the creation of the SearchBox (it will change on every refresh) or you can use the connector to build your own widget. By creating your own widget you can encapsulate the generation of the placeholder at the component level.

@callmephilip
Copy link

Thanks for the clarifications, Samuel. Does this effectively prohibit the usage of any wrapped components inside IS?

@samouss
Copy link
Collaborator

samouss commented Mar 28, 2018

No the usage of wrapper is not prohibit. What should be avoided (wrapper or not) is to pass a new reference of a prop on each render even when it doesn't change. But I hope we will get rid of this limitation in the upcoming versions.

@bobylito
Copy link
Contributor

It seems that the latest version of react-instantsearch does not have the issue anymore. You can try this here: https://codesandbox.io/s/6opx1zk8n (based on a fork of @callmephilip example with the latest react-instantsearch version)

@samouss
Copy link
Collaborator

samouss commented Aug 8, 2018

Actually no React InstantSearch still have the issue. It's patched in our API Client but not in the library itself. For example users that implement a custom search client (like the one implemented by @iam4x) will still have the issue.

@CodeMohan
Copy link

CodeMohan commented Oct 15, 2018

We are experiencing the same issue. Is there any work around for this?
package: "react-instantsearch": "4.1.2",

@samouss
Copy link
Collaborator

samouss commented Oct 15, 2018

@CodeMohan for the moment the workaround is inside the Algolia client. You can opt-in for an option that deduplicate pending requests. This behaviour is enabled by default with the version 5.2.0 of React InstantSearch.

If for any reasons you can't upgrade to the v5 you can use the prop algoliaClient to provide a custom client to React InstantSearch. The provided client must be at least on the version 3.28.0. The client must be created with the option _useRequestCache: true. Here is an example that shows how to implement it.

@CodeMohan
Copy link

Thanks @samouss. We tried the second approach you suggested. It did remove the duplicate calls but experiencing other side effects. We are investigating those issues.

@cloakedninjas
Copy link

I've stumbled across this myself. Recently moved from purely client rendering to SSR and was seeing duplicate requests being sent out. With the above option going into algoliaClient this has removed the duplicate requests.

However - toggling refinements on/off/on/off results in new requests being made after each interaction, while our client-side only code was smart enough to not send identical requests out.

I've done text diffs on the requests with the same refinements and the payloads are identical.

Is there any update on this issue?

@samouss
Copy link
Collaborator

samouss commented Jul 3, 2019

Could you provide us a reproducible example? That would help to understand the issue. We provide a template to avoid you the boilerplate part. You can find it on CodeSandbox.

@Haroenv
Copy link
Contributor Author

Haroenv commented Jul 3, 2019

note the prop is now called searchClient, but still works the same

@cloakedninjas
Copy link

We provide a template to avoid you the boilerplate part. You can find it on CodeSandbox.

Do you have a createInstantSearch version of the boilerplate ?

@Haroenv
Copy link
Contributor Author

Haroenv commented Jul 3, 2019

@cloakedninjas
Copy link

I found the cause of my other issue, I was re-creating the searchClient each render, which explains the cache being emptied and not preventing duplicate requests.

However _useRequestCache: true is still required (even with 5.7.0)

@samouss
Copy link
Collaborator

samouss commented Jul 3, 2019

Glad to see that you solved your issue. Yes, _useRequestCache is required to effectively deduplicate the request. Note that in the next version of the client this option will be enabled by default. At some point, we'll fix those issues inside React InstantSearch, but it's a tedious task that requires a lot of internal changes.

@Haroenv
Copy link
Contributor Author

Haroenv commented Apr 24, 2020

this has been fixed by using algoliasearch@4 or _useRequestCache in v3

@Haroenv Haroenv closed this as completed Apr 24, 2020
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

8 participants