algolia / angular-instantsearch

⚡️Lightning-fast search for Angular apps, by Algolia
https://algolia.com/doc/deprecated/instantsearch/angular/v4/api-reference/instantsearch/
MIT License
261 stars 73 forks source link

Using routing: true adds a duplicate history state on destroy #396

Open corentin-gautier opened 5 years ago

corentin-gautier commented 5 years ago

Hi!

So this is a weird bug:

I'm displaying a list of results which link to the details of the results: when routing is set to true I have to press back twice to go back to the list

Haroenv commented 5 years ago

This is expected unfortunately. The problem is that if we didn't add the extra history entry, the next route would still have the instantsearch query parameters. Instead what you will need to do is history.replaceState (instead of .pushState) on the route change which happens after InstantSearch was destroyed.

I'm not fully sure what the best solution is here yet, I've struggled to find a good way to:

  1. remove the URL parameters when there's no route change, but when InstantSearch unmounts
  2. remove the URL parameters without history addition when the route changes too.

You can't rely on the path or something like that, since InstantSearch can write those too.

Do you have any input?

corentin-gautier commented 5 years ago

@Haroenv Oh ok that makes sense ! Can't you use router events for this ? (if there's been a route change event you know you should replaceState otherwise pushState)

corentin-gautier commented 5 years ago

@Haroenv just found out that in iOS i need 3 history.back() to actually go back, so my workaround of doing location.back() twice doesn't work ... any news on this ?

Haroenv commented 5 years ago

No news on this, it's a very complicated topic for which we haven't yet found a solution. Can you give a minimal example of an app which needs to go back three times only on iOS?

corentin-gautier commented 5 years ago

@Haroenv unfortunately I don't have enough time available to spent on this project to be able to provide an exemple of this :/

ValentinFunk commented 5 years ago

@Haroenv we're experiencing a couple of bugs with the history/url handling as well, unfortunately very tricky to reproduce. Would it be possible to supply a custom implementation of the RoutingManager to instantsearch.js?

Haroenv commented 5 years ago

Yes, you can pass an object to the routing prop, there’s documentation for that on the main documentation website under “custom routing”

tkrugg commented 5 years ago

https://www.algolia.com/doc/guides/building-search-ui/going-further/routing-urls/angular/ And there's more about the that routing object on InstantSearch.JS documentation, which Angular InstantSearch is based upon. https://www.algolia.com/doc/guides/building-search-ui/going-further/routing-urls/js/?language=angular#references

ValentinFunk commented 5 years ago

Thank you @tkrugg @Haroenv :) :)

ValentinFunk commented 5 years ago

For anyone ending up here, here's a very simple implementation for a router that does not add to history for angular and uses query params:

import { Injectable } from '@angular/core';
import { Router, ActivatedRoute } from '@angular/router';
import { Subject } from 'rxjs';
import { takeUntil } from 'rxjs/operators';

@Injectable()
export class AngularInstantsearchRouter {
  paramsSnapshot: any;

  onDispose = new Subject();
  disposed = false;

  constructor(private router: Router, private activatedRoute: ActivatedRoute) {
    this.activatedRoute.queryParams
      .pipe(takeUntil(this.onDispose))
      .subscribe(
        params => (this.paramsSnapshot = this._deSerialize(params.p || '{}'))
      );
    this.paramsSnapshot = this._deSerialize(
      this.activatedRoute.snapshot.queryParams.p || '{}'
    );
  }

  onUpdate(callback: (route) => void) {
    this.activatedRoute.queryParams
      .pipe(takeUntil(this.onDispose))
      .subscribe(params => {
        const deserialized = this._deSerialize(params.p || '{}');
        console.log('not passing', deserialized);
      });
  }

  read() {
    return this.paramsSnapshot;
  }

  write(routeState) {
    if (this.disposed) {
      return;
    }

    this.router.navigate([], {
      relativeTo: this.activatedRoute,
      queryParams: { p: this._serialize(routeState) },
      replaceUrl: true
    });
  }

  createURL(state: any) {
    const tree = this.router.createUrlTree([], {
      relativeTo: this.activatedRoute,
      queryParams: { p: this._serialize(state) }
    });
    return this.router.serializeUrl(tree);
  }

  dispose() {
    this.onDispose.next();
    this.disposed = true;
  }

  private _serialize(state) {
    return JSON.stringify(state);
  }

  private _deSerialize(state) {
    try {
      return JSON.parse(state);
    } catch (e) {
      console.warn(e);
      return {};
    }
  }
}

to use provide it in your component that uses the instantsearch in it's template

@Component({
  selector: 'app-search-results',
  templateUrl: './search-results.component.html',
  styleUrls: ['./search-results.component.scss'],
  changeDetection: ChangeDetectionStrategy.OnPush,
  providers: [
    AngularInstantsearchRouter
  ]
})
export class SearchResultsComponent {
  config = {
    searchClient: algoliasearch(...),
    indexName: ALGOLIA_TRIPS_INDEX,
    routing: {
      routing: this.angularSearchRouter
    }
  };

  constructor(private angularSearchRouter: AngularInstantsearchRouter) {
  }
}

It works fine for our usecase without using onUpdate since we trigger the search manually in the component based on normal route params

Haroenv commented 5 years ago

Thanks @Kamshak, that's a really interesting implementation, if there's a good amount of feedback on this, we could maybe implement this into the main library. Now it looks somewhat opinionated, but that's likely solvable

HcroakerDev commented 4 years ago

@Kamshak I'm having trouble triggering a UI state change through calling the write() function with your above method. Is there a possibility to do this? When I reload the page, the UI State is updated according to the route.

EMenardc commented 4 years ago

@HcroakerDev I have the same issue, have your found any solution? Thanks.

ValentinFunk commented 4 years ago

Could you explain a bit more? Do you mean by navigating via angular router or by calling the write directly?

EMenardc commented 4 years ago

@Kamshak Whenever I initially load my component where the base Algolia instant search component is located. It makes a call to the algolia endpoint but the values does not show until I touch a filter or I make a change in the component. I tried to remove the ChangeDetectionStrategy to normal but it now calls the routing around 2000 times on the component initialization and on filter changes. This is pretty much my issue. Thanks

Yllescas-cr commented 4 years ago

hi there is currently another solution for event history.back ()? because my url is duplicated and stays in the same place. Thanks

Haroenv commented 4 years ago

https://github.com/algolia/angular-instantsearch/issues/396#issuecomment-496132812 is the canonical solution until we add an option to disable the final write

chhaymenghong commented 4 years ago

I used this to get around this issue. ngOnDestroy(): void { super.ngOnDestroy(); setTimeout(() => history.back(), 500); }

chhaymenghong commented 4 years ago

@Kamshak Whenever I initially load my component where the base Algolia instant search component is located. It makes a call to the algolia endpoint but the values does not show until I touch a filter or I make a change in the component. I tried to remove the ChangeDetectionStrategy to normal but it now calls the routing around 2000 times on the component initialization and on filter changes. This is pretty much my issue. Thanks

it is probably because you use ais-pagination component and you have so many pagination pages for your search result. From my debugging, it seems like it runs stateToRoute function as many times as the number of pagination pages.

ValentinFunk commented 4 years ago

@Kamshak Whenever I initially load my component where the base Algolia instant search component is located. It makes a call to the algolia endpoint but the values does not show until I touch a filter or I make a change in the component. I tried to remove the ChangeDetectionStrategy to normal but it now calls the routing around 2000 times on the component initialization and on filter changes. This is pretty much my issue. Thanks

If you use OnPush try using the async pipe for hits

// component
@ViewChild() aisInstantsearch: NgAisInstantSearch; 

      <ng-container *ngIf="(aisInstantsearch.change | async)?.results?.hits; let hits">
          <hit *ngFor="let hit of hits" [hit]="hit"></hit> 
      </ng-container>
Celtian commented 3 years ago

@Haroenv Can you provide valid woraround for latest angular and latest angular-instantsearch. This bug causing that algolia and routing is completely broken.

tkrugg commented 3 years ago

Hi @Celtian sorry for the late reply. For now the easiest workaround is not to use routing: true but a custom router.

...
import { history as browserHistoryRouter } from "instantsearch.js/es/lib/routers";
import { simple } from "instantsearch.js/es/lib/stateMappings";

// creating a custom router
const router = browserHistoryRouter({ /* ... */ });

// modify behaviour on dispose
// https://github.com/algolia/instantsearch.js/blob/master/src/lib/routers/history.ts#L181-L192

router.dispose = (function() {
   if (this._onPopState) {
    window.removeEventListener('popstate', this._onPopState);
  }

  if (this.writeTimer) {
    window.clearTimeout(this.writeTimer);
  }

  //  this.write({});    <--- THIS IS THE LINE WE'RE REMOVING FROM THE ORIGINAL IMPLEM 
}).bind(router)

// Now use this router like the following
{
  // ... other InstantSearch configuration options ...
  // routing: true

  routing: {
      router: router,
      stateMappings: simple()
  }
}

Here is a full example showing this: https://codesandbox.io/s/angular11-algolia-routing-forked-jgbv4?file=/src/app/algolia.service.ts I hope this helps.

⚠️ if you happen to use Angular InstantSearch with peer dependency "InstantSearch v4", this may not work for you but it's being fixed here https://github.com/algolia/instantsearch.js/pull/4808