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

v2 routing issue with going back #589

Closed
alexbudure opened this issue Nov 28, 2018 · 28 comments
Closed

v2 routing issue with going back #589

alexbudure opened this issue Nov 28, 2018 · 28 comments

Comments

@alexbudure
Copy link

alexbudure commented Nov 28, 2018

Feedback on the v2 prerelease version 🆕

When synchronizing with Vue Router, there's a bug when navigating between the pages. For example, if I'm on the search page and I go to another page (ie /about-us) and then try to go back, it takes two back button clicks to get back. Sometimes on the first back button click, the query from the search gets added to the current page (ie /about-us?{query}) and then finally on the second back button click it takes you back to the search page.

Another thing I've noticed, which might be related to the problem above, is that if you're on the search page and you click a link with a query from the nav bar (ie vehicles over 9000) the search results do not respond to changes in the URL.

What is the version you are using?

v2.0.0-beta.1

@Haroenv
Copy link
Contributor

Haroenv commented Nov 28, 2018

I have not tried it out yet, but have you tried making the whole routing object in data, instead of creating it in the template? I think you might have an infinite loop situation like described in earlier issues here 🤔

@alexbudure
Copy link
Author

I just tried your suggestion and no luck

@Haroenv
Copy link
Contributor

Haroenv commented Nov 29, 2018

When I try your repository out, I have a hard time trying to replicate what behaviour you mean. Can you maybe make a video or something of the misbehaving behaviour? Thanks!

EDIT: I didn't realise you were talking about the browser back button

@Haroenv
Copy link
Contributor

Haroenv commented Nov 29, 2018

We currently have an issue with the historyRouter of InstantSearch when all the widgets tree are mounted / unmounted. By default the router write to the history when the full tree is unmounted. It’s perfectly fine when the root tree is removed programmatically (e.g. a toggle button). The URL goes from:

"/search?brand=Apple" → "/search"

Once the “Back” button is pressed the URL will come back to the previous state and if we mount the widget again the state is the correct one.

One other use case is to use the lib with a built-in router (e.g. Vue Router). With this case the fact that the router write to the URL is an issue because it push one “useless” entry inside the history. The URL goes from:

"/search?brand=Apple" → "/about" (Vue Router) → "/about" (IS Router)

Vue Router detect a page change with the link component and push a new entry inside the history. But it’s only at that time that the Vue app will start unmount the the tree. It means that the IS Router push a new entry that will prevent the user to go back on the previous page on the first click.

We don’t have a solution yet. A workaround is to disable the write inside the IS router when it’s used with Vue Router but at the same time it disable the URLSync on unmount. It’s a tradeoff that can be implemented in user-land at the moment. I don’t think we want to support this inside InstantSearch.

What you can do in your case is the following:

  1. copy the source of historyRouter
  2. remove the write call inside dispose
  3. use that new router

However, this workaround will only work once we do a small change in ais-instant-search, which is adding destroyed() { this.instantSearchInstance.dispose() } (feel free to do a PR for this, but we will do it as well)

I hope that this is clear, me and @samouss have been trying to find a solution for both use-cases for the whole day now and this was the best we could find.

@alexbudure
Copy link
Author

Just to let you know, I really appreciate the effort you guys put in for every request/issue that gets posted.

After your last comment, I'm starting to understand the issue.

In the image below, I print the pathname at the top of the write call. When I filter a refinement, it runs and shows /inventory, which is good. But after when I switch pages to /about-us, this function also runs. Not only that, it runs like 15 times which I don't know why.

screen shot 2018-11-29 at 1 49 05 pm

The solution that required the least amount of work for me was to wrap the router.push method in a conditional that only runs if current page is also the search page. In my example that's /inventory. I haven't checked every use case, but it seems to be working fine for me.

write: routeState => {
  if (window.location.pathname === '/inventory') {
    this.$router.push({
      query: routeState,
    })
  }
}

A more universal solution would be to add a Boolean argument to the write call that would be true if the current page is the search page. ie (routeState, isSearchPage) =>

@Haroenv
Copy link
Contributor

Haroenv commented Nov 29, 2018

Right, that makes sense. I’m not exactly sure how you got in a situation where there would be 15 calls, but we always recommend to debounce the URL synchronisation, since it’s notoriously slower than DOM updates; that could be a cause of your over push.

@alexbudure
Copy link
Author

I've ran into more routing troubles. I have a panel with more filters that gets shown and hidden by a button using v-show. When I get to the search page with a filter already in the query, I can only add more filters to it and every time I remove that filter and close the panel, my filter gets reapplied. This is using default router + state mapping.

https://res.cloudinary.com/driverseat/video/upload/v1543645338/router_s4qbjo.mov

@gpassarelli
Copy link

I'm having the same issue as @alexbudure described later.

If the URL is loaded with some filters query params, even if I remove all filters, and afterwards apply new filters the ones that were loaded with the URL, will keep and only add the new ones.

In other words, it's only possible to add new filters and not change them.

It was working with v2 before, it stopped working after I've updated for the last version.

@Haroenv
Copy link
Contributor

Haroenv commented Dec 7, 2018

Can you make a sandbox with reproduction that it worked before? I don’t see any code that could’ve influenced that behaviour

@gpassarelli
Copy link

@Haroenv I'll try to create a sandbox to reproduce, for now I made 2 videos, one that is in production with the previous version of the code, and one that is in local env with the new version of the code.

Live version with the old version of the code

Local version with the new version of the code

@gpassarelli
Copy link

@Haroenv , I believe this might be something related to the use of vue-router and the routing method mentioned in the docs.

I looked at @alexbudure code, and he uses the vue-router as well. I'm trying to create a codesandbox but it´s not loading correctly the beta version of vue-instantsearch.

So i'm not sure if we missed something on the docs. Because it was not clear the proper way to do it when we have both routers.

@Haroenv
Copy link
Contributor

Haroenv commented Dec 10, 2018

I have not been able to dive more deeply into it, but there's definitely something going wrong, although I'm not sure what yet. I wonder if you could avoid the issues by using the historyRouter from InstantSearch without trying to sync it to Vue Router. Technically it also works, although it means you won't have the access to the InstantSearch created URL from Vue Router itself. 🤔

@BenjaminWalsh
Copy link

I've spent some time today looking at the same issue. I think the problem is in instantsearch.js
import { history as historyRouter } from 'instantsearch.js/es/lib/routers'

I tried swapping out the history router for a custom router as detailed on
https://www.algolia.com/doc/guides/building-search-ui/going-further/routing-urls/vue/

and by copying the demo linked at the bottom here (which is broken as I write this):
https://codesandbox.io/s/github/algolia/doc-code-samples/tree/master/Vue+InstantSearch/routing-vue-router

When logging out the functions as they happen the write (and other functions) happen multiple times.

@mategvo
Copy link

mategvo commented Jul 25, 2019

I also have the issue, go back button needs to be clicked twice or more times. Sometimes it just changes the window scroll position. Sometimes it doesn't do anything.

I think I know what the problem is.

I am manipulating facetFilters, query, and filters in searchParameters to modify feed contents to fit current page. For exmaple, a product with categories "accessories, toothbrushes" is shown to a user, I change the facetFilters to match these categories. While user can still pick their own categories, within the range of automatically selected facetFilters. And to give you some background, the ui utilises infinite scroll and masonry as well.

what happens is the route changes to show a new product, when it's shown, the filters are applied which in turn push another history state, which makes two states for one click

This is the general component structure:

App
^< Product page
^< Feed 

As you can see, product page and feed are parallel. Which means the product page can change to a different product, but the feed doesn't reload. Instead, I am requesting results from Algolia with a watcher in feed component:

    watch: {
      $route() {
        this.filterByRoute()
      }
    },

while "filterByRoute" does something like this:

      filterByRoute() {
        if (this.$route.name === 'category') {
          var facetFilters = `categories.name:${this.$route.params.slug}`
        }
...
        // forces algolia to refresh 
       //  but probably also causes the history state to be pushed
       // which means that the go back button won't work as expected
        this.searchParameters = {
          facetFilters: facetFilters
        }

I am not sure why facetFilters changes are reflected in $router state (if they are), the URL of the page does not change. Also, I don't have an idea at the moment how this could be done differently.

I would like Algolia not to push $router or history states, unless the URL actually changes.

@mategvo
Copy link

mategvo commented Jul 25, 2019

Update. I have a partial solution. I am changing facet filters in <router-link> to include the query parameters

              <router-link
                 :to="{name:'product',
                 params: { slug: item.slug, product: item},
                 query: buildQuery(item)
              }">

This solved the problem, but it's also a limitation to how we can use Algolia...

@mategvo
Copy link

mategvo commented Jul 25, 2019

Update II

Again things are not working as expected:

  1. The url search parameters disappear shortly after a page is loaded
  2. Search parameters entered via algolia components/fitlers stay even if user goes to another page, f.e. <router-link :to="{name: 'home'}">home</router-link>.

@Haroenv
Copy link
Contributor

Haroenv commented Jul 26, 2019

@mateuszgwozdz, could you make a sandbox which replicates the issue? Having a duplicate entry in the history when removing the component is confusing for sure, but we are forced to do it as far as I can see. If we don't add an entry when someone unmounts the component without changing the route, there'll be a stale URL, if they update the url manually, it'll have a duplicate URL. I'm not sure if router-link has this option, but could you see if there's a way it can replace the last path instead of pushing a new entry in the history?

@mategvo
Copy link

mategvo commented Jul 26, 2019

@Haroenv thanks a lot for getting back to me quickly!

could you make a sandbox which replicates the issue

Is there any simpler way we can solve this? This would be quite tedious... What should be included in the sandbox? How can I share it with you confidentially

but we are forced to do it as far as I can see

The back button is very imporant element of any user interface. How else can we make the go back button work as expected? f.e. Shift multiple states back at once?

someone unmounts the component without changing the route

the component is not unmounted

replace the last path instead of pushing new entry

I did that. Here's some documentation https://router.vuejs.org/api/#replace

              <router-link
                :to="{name:'product',
                  query: buildQuery(item),
                  params: { slug: item.slug, product: item}}"
                class="product-image-link"
                replace>

The problem persists.

I don't understand why the state has to be pushed, is there a way to disable this behaviour?

@Haroenv
Copy link
Contributor

Haroenv commented Jul 26, 2019

You can disable the pushing of the URL, where there would be only one search URL saved to the page, which then would however replace the last history element on the site (that's how replace works), by in the page w

       write(routeState) {
-            vueRouter.push({
+            vueRouter.replace({
              query: routeState,
            });
          },

https://codesandbox.io/s/vue-instantsearch-v2-starter-irpgc

@mategvo
Copy link

mategvo commented Jul 31, 2019

It's working now. Sorry. I must have made a mistake somewhere.

@Haroenv thank you for helping out. There's one more issue with your last propsed solution https://codesandbox.io/s/vue-instantsearch-v2-starter-irpgc

When user clicks on a result, router-link is passing properties to the view. One of them is "slug", but there's also product infromation, that is used to display partial results while application awaits response with full product information from the API.

Problem with this solution is that these parameters disappear.

<router-link :to="{ name:'product', params: { slug: item.slug, product: item}}">
    Awesome Product
</router-link>

From the example above "slug" is passed to the url currectly, while "product" is missing. Do you want me to set up a sandbox for this?

Otherwise this solution solves the go back issue!

@MaxHalford
Copy link

What's the current status on this? I'm using InstantSearch and I have to click twice to go back to the previous page.

@Haroenv
Copy link
Contributor

Haroenv commented Aug 11, 2020

@MaxHalford, the current solution is to use a custom router (which can be the same as the real historyRouter), but without calling this.write in the dispose hook

@joe-berg-colossus
Copy link

@MaxHalford, the current solution is to use a custom router (which can be the same as the real historyRouter), but without calling this.write in the dispose hook

do you have an example of this you could share? also dealing with a similar issue

@nadge
Copy link

nadge commented Apr 19, 2021

Also experiencing this issue in v3.6

Have to press back twice to go back to the previous state. This seems to be because on page load it adds the current url to the history so then when pressing back fir the first time then it just goes back to itself.

I copied the historyRouter and removed this.write call from the dispose method but still experienced the same issue

I did manage to workaround this issue by wrapping the pushState call with an if statement to check if it's the current url and things seem to be working as expected.

I copied this file node_modules/instantsearch.js/es/lib/routers/history.js and replaced the following code on line 119

this.writeTimer = window.setTimeout(function () {
  setWindowTitle(title);
  window.history.pushState(routeState, title || '', url);
  _this.writeTimer = undefined;
}, this.writeDelay);

with this:

this.writeTimer = window.setTimeout(function () {
    setWindowTitle(title);

    if(url !== window.location.href){
        window.history.pushState(routeState, title || '', url);
    }

    _this.writeTimer = undefined;
}, this.writeDelay);

Not quite sure on the implications of this and if it may affect anything else but from the testing I have done this seems to have solved the issue for me.

@Haroenv
Copy link
Contributor

Haroenv commented Apr 19, 2021

yes, checking whether the url has already been changed is a good call, thanks @nadge

@pjatx
Copy link

pjatx commented Jun 4, 2021

thanks @nadge, was trying to chase this issue down and this was an easy fix.

pjatx added a commit to pjatx/instantsearch.js that referenced this issue Jun 4, 2021
@kode8
Copy link

kode8 commented Feb 18, 2022

You can also try window.history.replaceState, replace not push.

if (_this2.shouldPushState && lastPushWasByISAfterDispose) {
window.history.replaceState(routeState, title || '', url);
_this2.latestAcknowledgedHistory = window.history.length;
}

or

if(url !== window.location.href){
window.history.replaceState(routeState, title || '', url);
}

@Haroenv
Copy link
Contributor

Haroenv commented Feb 20, 2022

In the latest version of InstantSearch / Vue InstantSearch this issue is fixed actually: algolia/instantsearch#5004 (4.38.1 of InstantSearch.js)

If you have a new issue, please open it up again :)

@Haroenv Haroenv closed this as completed Feb 20, 2022
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

10 participants