setIndex should not set page to 0 (and any other method)

[maxiloc]

I understand why it was there in the first place (the setPage(0)) but it the snippet bellow will not load page 2. And I think it is not what we want.

helper.setCurrentPage(2);
helper.setIndex(indexName);
helper.search()

[vvo]

PR or it did not happen :dancer:

[bobylito]

I don't agree, while changing the index you can't know in advance how many pages there will be in the next search therefore resetting the page to 0 is a safe bet. If you are sure that there will be a page, let's say because you are using a slave that only changes the ordering of the results, then you are in a specific case. And then you can use :

var page = helper.getCurrentPage();
helper.setIndex( indexName )
      .setCurrentPage( page )
      .search();

[vvo]

I agree with both arguments given their context but still @maxiloc was trapped by the behavior of setIndex when loading a state from a url, the order you do .setCurrentPage and .setIndex should not matter on the result.

If @maxiloc was trapped then I guess other people will be trapped too?

So:

• when changing the index using a UX element the expected behavior is always to be at page 0. Even if you were at page 5 and you are sure there will be a page 5 in the new index, it doesn't make sense to keep the page between the sort changes, no one would expect this.
• when there's a pageload, the .setCurrentPage and .setIndex order should not matter because it's disturbing

There's no API to handle both cases easily so I would just not set the page in setIndex because setIndex on a API level says: "set the index". not "set the index and the page".

Switching to page 0 when changing the index is a sweet shortcut but it can be tricky in some situations (page load).

We rather let the API consumer figure out that yes if he's implementing a sorting dropdown, he should reset to page 0 before calling search.

thoughts?

[bobylito]

Ok I understand better now. Makes sense when using this API. But, there is a cleaner way to do that when loading from a URL. Using setState, first retrieve the partial from the URL into an object, and then use setState to fix the state as defined in the URL. On Jun 26, 2015 8:12 PM, "Vincent Voyer" [email protected] wrote:

I agree with both arguments given their context but still @maxiloc https://github.com/maxiloc was trapped by the behavior of setIndex when loading a state from a url, the order you do .setCurrentPage and .setIndex should not matter on the result.

If @maxiloc https://github.com/maxiloc was trapped then I guess other people will be trapped too?

So:

• when changing the index using a UX element the expected behavior is always to be at page 0. Even if you were at page 5 and you are sure there will be a page 5 in the new index, it doesn't make sense to keep the page between the sort changes, no one would expect this.
• when there's a pageload, the .setCurrentPage and .setIndex order should not matter because it's disturbing

There's no API to handle both cases easily so I would just not set the page in setIndex because setIndex on a API level says: "set the index". not "set the index and the page".

Switching to page 0 when changing the index is a sweet shortcut but it can be tricky in some situations (page load).

We rather let the API consumer figure out that yes if he's implementing a sorting dropdown, he should reset to page 0 before calling search.

thoughts?

— Reply to this email directly or view it on GitHub https://github.com/algolia/algoliasearch-helper-js/issues/121#issuecomment-115821697 .

[redox]

I understand your point @bobylito but I think there are a lot of other things you can do that will generate a 0 results answer and that are not checked -> like if I add a wrong numeric filter and so on.

I would remove that setCurrentPage from the setIndex method to make sure we don't do anything magic.

[bobylito]

In the case of the URL I would still suggest using the setState, because it's less mutations and less functions calls.

But if you think that there is magic when we set the current page to 0 when we change the index, we have to have the same behavior in every methods that has the same behavior. Will fix that in the next version, but this will break the code of the users of the current version.

[bobylito]

Why don't we discuss about this issue around a cup of coffee Monday morning? On Jun 26, 2015 9:08 PM, "Sylvain Utard" [email protected] wrote:

I understand your point @bobylito https://github.com/bobylito but I think there are a lot of other things you can do that will generate a 0 results answer and that are not checked -> like if I add a wrong numeric filter and so on.

I would remove that setCurrentPage from the setIndex method to make sure we don't do anything magic.

— Reply to this email directly or view it on GitHub https://github.com/algolia/algoliasearch-helper-js/issues/121#issuecomment-115841331 .

[vvo]

Of course we will :)

[Nainterceptor]

Hello, I have an issue, probably linked to this one.

This workflow call right my page :

• helper.clearRefinements
• helper.setQueryParameter
• helper.addFacetRefinement
• helper.addDisjunctiveFacetRefinement
• helper.on('result', function(){})
• helper.setCurrentPage
• helper.search

But in this order, it's not working :

• helper.setCurrentPage
• helper.clearRefinements
• helper.setQueryParameter
• helper.addFacetRefinement
• helper.addDisjunctiveFacetRefinement
• helper.on('result', function(){})
• helper.search

Maybe it's important to document somewhere when we should call setCurrentPage ?

Have a nice day, Gaël

[bobylito]

@Nainterceptor Any method that will change the number of pages will reset the current page to avoid inconsistencies. For example, if it was not the case we could stay on page 3 even though there were only 2 pages returned. So in fact, in your example clearRefinements setQueryParameter addFacetRefinement and addDisjunctiveFacetRefinement will set the current page to 0. You should call setCurrentPage when you want to change the current page.

[Nainterceptor]

@bobylito It make sense, I agree, but it's maybe important to write somewhere in the doc, to prevent bad usages like mine ?

[bobylito]

You are right! :) #274

[vvo]

Maybe we could be even more magical and solve this issue by not resetting to page 0 if the page was manually set somewhere before a call to .search.

So if at any moment the user specifically says he wants page 1 and calls for .search() then he should be one page 1, no matter the calling order.

Documenting it will maybe help people struggling with this issue get an answer at some point. But only after having struggled with it for a good amount of time.

If we can fix the issue for good we should do it?

[rayrutjes]

+1 for me on this one, this currently breaks pagination on all WordPress instant-search instances ;)

[vvo]

Some context given @rayrutjes comment: https://github.com/algolia/algoliasearch-wordpress/pull/305

I know this is not constructive to say "We need to fix this". Still, we just experienced a painful debugging session because of the setPage(0) being done, behind the scenes while when using the helper, one could always think "This is a somehow low level API (no UI), I am in control when I call setQueryParameter()".

It's not the case if you forget/do not know what is happening.

[olance]

+1, just spent ~1h debugging instantsearch pagination wondering why an out-of-the-box widget wouldn't change pages... only to realize I have some code that changes the helper state for some advanced use case and was resetting the page before search() was called.

Although I understand where it comes from, it's an unexpected side-effect and breaks the principle of least astonishment. (yeah, big words! :))

[Jerska]

I also got bitten by setQueryParameter's setPage(0). I used an init widget only to change the highlight{Pre,Post}Tag s and couldn't understand why it was failing.

In my case, I guess I could just fix it by using instantsearch.js's searchParams instead, but it definitely is surprising.

=> https://github.com/algolia/algoliasearch-shopify/commit/e995a94523d8bbeb337671f3834bb7ee055ce6e1

[vvo]

Since we are gonna work on a revamped helper version those feedbacks are important and shows that everytime we do a little bit of magic in low level libraries it's worth considering the impacts it has more precisely.

Thanks!

[rayrutjes]

We need to methods for this indeed ;)

[Jerska]

Spent half an hour again today on this one in Zendesk. /o\ I was used to using custom widgets to set my refinements and did a hack to set a query parameter in the searchFunction, both of which I had to surround with cont page = getPage(); ...; setPage(page).

[Jerska]

Basically, I think the idea here is that it doesn't make sense to reset the page when done before the search.start() of instantsearch.js, because there, we're initializing with the same thing as always, so we know there'll be results, but it does when done afterwards. Maybe worth having a before start / after start mode ?

[vvo]

I would say for such a low level library, when modifying a state object, we should never try to be smart on the page being reset to 0. We have now seen that many people were tricked by it.

We are gonna work on instantsearch-core in January and will tackle this issue inside it I think.

[bobylito]

Alright this is indeed very not cool what you have to implement @Jerskouille :/ Let's move forward.

[Jerska]

Getting bit in my hack inside the searchFunction is definitely OK, the surpising part is really inside an init function of a custom widget.

[tekstrand]

Wanted to check in to see where this is at and if anyone has a recommended workaround. I'm running into this while try to create an initial query object based off of URL parameters in my app.

[Haroenv]

That’s still the case. You need to getPage first and do setPage with that page after doing that mutation.

[drzraf]

+1. I spent hours figuring out what was happening. I based my searchFunction on what is provided by https://www.algolia.com/doc/api-reference/widgets/instantsearch/js/#widget-param-searchfunction But even though the comments state // we re-apply the previous page, it's so counter-intuitive and hard to understand!

1. It appears like the button (view more, page 2) does not work.
2. You need to disable caching to see actual (redundants) requests issued and see page=0
3. You need to understand why page isn't incremented (and find this ticket or read X times the above link)

Some clearer warning must be set somewhere about this setPage() usage within searchFunction.