setUpLocationSync causes an duplicate navigation events

[bourey]

Angular's router upgrade code provides a setUpLocationSync method which listens for AngularJS-initiated $locationChangeStart events, then calls Angular's router.navigateByUrl. The docs for this method state that "History.pushState does not fire onPopState, so the Angular location doesn't detect it. The workaround is to attach a location change listener"(

angular/packages/router/upgrade/src/upgrade.ts

Line 55 in e4c53f8

* History.pushState does not fire onPopState, so the Angular location

). Without this code, the Angular router does not respond to location changes initiated by AngularJS, regardless of whether the pages themselves are associated w/ AngularJS or Angular content. For example, AngularJS-based navigation links would fail to trigger a routing event between two Angular pages.

While this code works for AngularJS-initiated location changes, it currently produces duplicate NavigationStart, etc. events for Angular-initiated changes. The AngularJS router sees that the browser location has changed and triggers a location change event, which in turn triggers an additional NavigationStart event in Angular. It appears that we need some way to trigger Angular navigation only for location changes that originate in AngularJS.

This issue is currently causing duplicate router events for any Angular-driven page transition in dual-router apps, which is a performance issue.

[aciccarello]

I believe I am encountering a related issue where my call to router.navigate() with query params within the ngOnInit() method is being overriden because the $locationChangeStart event is triggering a route back to the same route with no queryParams.

My current workaround is to use setTimeout(fn, 0) to delay the router.navigate() call till after $locationChangeStart.

[gkalpak]

It is hard to investigate without a reproduction. @bourey, could you create a (preferably minimal 😁) reproduction; either as a repo or a live demo?
In case it helps, here are my stackblitz "playgrounds": ngUpgrade-static, ngUpgrade-lite

I wonder if this is related to #20244 at all.

@jasonaden, you marked it as a regression. Is there any previous version that is known to work?

[aciccarello]

@gkalpak It took me about 2 hours to get a concise reproduction but you can see the issue that I'm experiencing in this stackblitz. My example is complicated by some special bootstrapping but I believe the issue should be clearly demonstrated.

https://stackblitz.com/edit/ajc-angular-upgrade-locationsync-bug

I also tested my stackblitz with Angular versions 4.4.6 and 4.0.3 and the issue remained.

[jasonaden]

@gkalpak We thought it was a regression, but as it turns out it had previously been broken. Changing the label.

[jasonaden]

Also, this fix (#21728) went in so we can make a change such as this to upgrade.ts:

ngUpgrade.$injector.get('$rootScope')
  .$on('$locationChangeSuccess', (_: any, next: string, __: string, state: any) => {
    const url = document.createElement('a');
    url.href = next;

    if (!state || !state.navigationId) {
      router.navigateByUrl(url.pathname + url.search + url.hash, {skipLocationChange: true});
    }
});

We are currently testing this change internally to make sure it doesn't open up other issues.

[dgsmith2]

I tried the change mentioned by @jasonaden but it didn't work. My 'fix' was to wrap my call to setUpLocationSync in a setTimeout. I'm not sure how appropriate it is given...

The problem I faced was if a trailing slash was included in an Angular route and it is the initial route, the listener registered by the sync would lead AngularJS to inform Angular of the route for a second time. Angular would start navigation having stripped the slash, but when the 'duplicate' came in via AngularJS, with the trailing slash intact, the original (correct) Angular route would be cancelled, replaced with duplicate. This meant instead of my route for /foo being applied, my route for /foo/:id would run with an empty (0) value for :id

[dgsmith2]

In addtion to my previous work around, I am now ALSO using the skipLocationChange configuration @jasonaden mentioned. This became necessary as soon as we set onSameUrlNavigation: 'reload' on our router because we started encountering a "Throttling history state changes to prevent the browser from hanging" warning.

[jupapios]

Also, this fix (#21728) went in so we can make a change such as this to upgrade.ts:

ngUpgrade.$injector.get('$rootScope')
  .$on('$locationChangeSuccess', (_: any, next: string, __: string, state: any) => {
    const url = document.createElement('a');
    url.href = next;

    if (!state || !state.navigationId) {
      router.navigateByUrl(url.pathname + url.search + url.hash, {skipLocationChange: true});
    }
});

We are currently testing this change internally to make sure it doesn't open up other issues.

For us, in some cases, it was affecting the browser's back button getting stuck in a loop of two pages

The fix @jasonaden mentioned didn't work for me because state.navigationId was always defined, I ended up "fixing" it by validating that the current URL is different to the next one, which is no more than a hack, why is the $locationChangeStart being triggered with next and current being the same URL?.

Note: I'm using the last version of https://github.com/angular/angular/blob/7.0.x/packages/router/upgrade/src/upgrade.ts (that's where the resolveUrl comes from).

  ngUpgrade.$injector
    .get('$rootScope')
    .$on('$locationChangeStart', (_: string, next: string, current: string) => {
      const currentUrl = resolveUrl(current);
      const currentPath = location.normalize(currentUrl.pathname);
      const currentLocation = currentPath + currentUrl.search + currentUrl.hash;
      const nextUrl = resolveUrl(next);
      const nextPath = location.normalize(nextUrl.pathname);
      const nextLocation = nextPath + nextUrl.search + nextUrl.hash;

      if (currentLocation !== nextLocation) {
        router.navigateByUrl(nextLocation, {
          skipLocationChange: true,
        });
      }
    });

[aaronfrost]

All the comments on here have been extremely helpful. Thanks to everyone who has participated. I also needed to do the skipLocationChange: true hack, which means that I had to make my own copy of the setUpLocationSync function in order to do this. It sure would be nice of the API of the setUpLocationSync allowed me to pass this in so that I can stop having copy/pasted code in my code base.

It seems like @jasonaden said that he was going to get that into the codebase. But it's still not in. Any ideas on when this change might be made? I understand the push for Ivy. So I am simply looking for a timeline. Never is an acceptable answer, as well.

[gkalpak]

There are no immediate plans to work on this afaik, but if anyone wants to submit a PR, I would be happy to review 😉

[frederikprijck]

@aaronfrost @gkalpak Interesting, I have a scenario that's still not working when using skipLocationChange: true.

Here's a stackblitz for my scenario: https://angular-4hte4b.stackblitz.io/#/ng2
Here's the editable Stackblitz link: https://stackblitz.com/edit/angular-4hte4b (note: the bug only occurs when running the application outside of the stackblitz previewer, so using the first link)

If you open this link, click "AngularJS" and hit the browsers back button you will see it won't work until u click it twice.
If I "patch" the setUpLocationSync function the way you mention, the issue only occurs for the very first route (if it's an angular route). As soon as u start clicking around a bit (like: open the URL above, click AngularjS, click Angular, click AngularJS, hit back ), it works fine.

Did you manage to also resolve that specific scenario? If yes, any chance you could share how?

Thanks!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}