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

Component perform same request on creation #365

Closed
samouss opened this issue Nov 20, 2017 · 20 comments
Closed

Component perform same request on creation #365

samouss opened this issue Nov 20, 2017 · 20 comments
Assignees

Comments

@samouss
Copy link
Contributor

samouss commented Nov 20, 2017

In Storybook all components perform at least two requests when they are created. When a component involved some facets (ex: RefinementList, RangeInput) the component will trigger four requests on creation.

You can reproduce easily in Storybook, open the developer panel & watch the network panel. Maybe it's related to how Storybook instantiate the page, but I also checked on Spencer & Williams and some of the requests are also executed multiple times. On the homepage when you trigger a search two requests are trigger and they are both replay once (four request in total).

request

@Haroenv
Copy link
Contributor

Haroenv commented Nov 20, 2017

I really think issues like those should be solved upstream. I don't understand why the cache doesn't kick in for these requests, and neither in RIS 😢

@samouss
Copy link
Contributor Author

samouss commented Nov 20, 2017

I agree on the fact that maybe the cache could handle this. But we still have a problem we shouldn't perform four request on component creation. If it's a design problem we should probably think about it at some point. Same for RIS.

@rayrutjes
Copy link
Member

I can take a look at this.
I expect double the amount of requests given the wrapper around the story is entirely decoupled from the actual story.

However it seems there are 4 times the amount of queries.

In Vue InstantSearch, the solution for this kind of problems is to "stop" the store and "start" it again.

In stories I voluntarily didn't use that to not introduce complexity.

@rayrutjes rayrutjes self-assigned this Nov 20, 2017
@rayrutjes
Copy link
Member

I just checked and I think the case where there are 4 requests only happens if you are not using the last version.
We fixed an issue in #359

Can you check with the latest version @samouss ?

@samouss samouss closed this as completed Nov 21, 2017
@samouss samouss reopened this Nov 21, 2017
@samouss
Copy link
Contributor Author

samouss commented Nov 21, 2017

In stories I voluntarily didn't use that to not introduce complexity.

Yes, but inside the component we already do something with "start" & "stop". We also need to do it at the application level?

Can you check with the latest version @samouss ?

On Storybook it should be the latest version right?

@rayrutjes
Copy link
Member

rayrutjes commented Nov 21, 2017

Yes, but inside the component we already do something with "start" & "stop". We also need to do it at the application level?

This is done by default in the Index component.

In story book however it is a bit hacky and the logic doesn't kick in.

On Storybook it should be the latest version right?

On the live version on don't see the 4 requests issue, do you?

Edit: I see it on first load indeed. Probably related to Storybook and the way it loads / mounts stuff.

@samouss
Copy link
Contributor Author

samouss commented Nov 21, 2017

Thanks for the precisions @rayrutjes! But on Spencer & Williams if you trigger a search on the Homepage, it will trigger two requests with two duplicates (four in total).

@rayrutjes
Copy link
Member

I'm not sure this library is used everywhere on that project.
@ehayman could tell us more about it.

@ehayman
Copy link
Contributor

ehayman commented Nov 21, 2017

We should upgrade to the latest version before saying anything definitive - @utay do you mind doing this and testing it out again?

@utay
Copy link

utay commented Nov 21, 2017

Sure. I'll upgrade, test it and let you know!

@utay
Copy link

utay commented Nov 21, 2017

So upgrading the library (to 1.3.2) didn't change anything. However, I don't know why, but when I remove the query-parameters prop of the ais-index component, it doesn't do the duplicate requests.
@rayrutjes @Haroenv do you have an idea of what is going on?

@rayrutjes
Copy link
Member

The query-parameters property is a bit "magic", and tries to do a bit too much I fear.
It is supposed to only merged into the helper the parameters that are given.

However, the helper is not always able to detect if something has really changed, so it will emit a change event and trigger an Algolia call.

I would need to investigate the implementation in more detail to come up with a better idea on how to address / improve this.

@rayrutjes
Copy link
Member

@utay could you point me toward the files you are referring to?

@utay
Copy link

utay commented Nov 21, 2017

Got you!

The logic of the homepage (link) is a bit complex, here is the /contact page code which also has a search, is much simpler and has the exact same behaviour while removing query-parameters.

@rayrutjes
Copy link
Member

I spent a fair amount of time on this and narrowed down the issue to a few things.

  • upgrade algoliasearch-helper to 2.23.0 to benefit from the "smarter" change detection
  • make sure this is actually subject to passe this

The current issue is that even though the query parameters are the same, they are not detected as such by the helper.

@rayrutjes
Copy link
Member

I think I will need to summon @bobylito here.
I'm not sure I get the full logic of how we detect if the state has changed.
Even when I take the existing state and set it again it triggers change events.

@bobylito
Copy link
Contributor

Hello everybody 👋

Happy to provide some information about the "change detection".

The helper is composed of two layers:

  • one is the "public" interface (the helper that we manipulate) which is an event emitter.
  • the other is the inner data structure that contains the search parameters. This object is built like an immutable data structure, meaning that it will return a new instance when there is a modification. The contract is that if the change of parameter is a noop (meaning that it doesn't really apply an actual modification) then it returns the same instance.

This mechanism is the core to the emission of "change" events. When the reference to the current search parameters is the same as the new one, the helper doesn't trigger a change event.

A "false positive" change event might happen if the SearchParameters returns a new instance even though it's not supposed to.

@rayrutjes
Copy link
Member

Thanks for that explanation @bobylito , I feel like this helps already narrowing down the issue.

A "false positive" change event might happen if the SearchParameters returns a new instance even though it's not supposed to.

I think this is what is happening, but I will need to spend a bit more time on it to figure out why the helper chooses not to return the previous state instance.

I've already found part of the issue which is that the parameters are sometimes observed by Vue, which means some additional keys are part of the value meaning that the helper will consider it as a new value.

I will keep you posted.

@julon
Copy link

julon commented Mar 5, 2018

Hi @rayrutjes,

not sure if it can be of any help but I met some similar behavior of storybook on my side,
which is related to the addon system.

Adding a withDecorator, calls the story.add callback twice. And adding custom editable values to withKnobs duplicates the mounting process thus duplicates the http calls. Maybe the withPreview you are using doubles it.

For the quadriple http calls issue, it may be related to the 'computed get/set' behavior in the input component which can call 'set' multiple times even if its the same value(on init, on new default value). Moving the set(value) searchStore calls to a query watcher can solve the problem.

Good luck! ✌️

@Haroenv
Copy link
Contributor

Haroenv commented Nov 6, 2018

please ope n a new issue with reproductible example if this problem is happening for anyone else 👍 thanks everyone for the information given here 🙂

@Haroenv Haroenv closed this as completed Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants