Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL Redirection Infinite Loop #113

Closed
AndrewGlazier opened this issue Oct 31, 2019 · 97 comments
Closed

URL Redirection Infinite Loop #113

AndrewGlazier opened this issue Oct 31, 2019 · 97 comments

Comments

@AndrewGlazier
Copy link

AndrewGlazier commented Oct 31, 2019

Issue seems to relate to this comment;
#94 (comment)
Creating a new ticket as I cannot reproduce using the browser back button.

This bug is reproducible in the vanilla example for coexisting angular micro-frontends found here:
https://github.com/joeldenning/coexisting-angular-microfrontends

Reproduction steps;
Using the navigation app, trigger a route change, and then quickly trigger another nav change.

Issue is reproducible on;
IE11 (Very common)
Firefox (Mediocre-Hard)
Chrome (Rare)

Notes;
In our application we have found that having more simultaneous angular apps running in parallel triggers the issue more commonly, however this bug is reproducible with only one app actively loaded by the single-spa (however with difficultly even in IE11). (Issue does not occur at all with the app running in angular standalone without the spa)

The issue appears to be also related to Router.forRoot in some way, upon removing this from an app, the issue becomes more difficult to reproduce even with IE11, a similar effect to whether the app wasn't running.

This seems to be related to the speed of the navigation. Navigating twice very quickly causes this effect.

If struggling to reproduce, we found that having a button which changes to swap between two pages and spam clicking it causes the bug to reproduce on IE11 every time.

The navigations do not need to be between separate apps, routing between two pages on the same app causes the issue.

@joeldenning
Copy link
Member

I do not have time to work on this issue - outside contributions would be greatly appreciated.

@rknash18
Copy link

rknash18 commented Nov 6, 2019

@AndrewGlazier I have fixed this issue by adding the onSameUrlNavigation: 'ignore' option to router module in angular.

@joeldenning
Copy link
Member

Oh wow thank you for sharing that @rknash18. It would be great if that workaround works for everyone else's issues they've been having, too. I've commented in #94 asking others to test if this works for them.

If it does, let's look into incorporating this into single-spa-angular so that all benefit from it.

@ashuaggarwal94
Copy link

Hi @joeldenning , @rknash18 ,

The solution provided in the comment does not work for me I m stilling using the changeDectection ref as an alternative.

@rknash18
Copy link

rknash18 commented Nov 7, 2019

@ashuaggarwal94 I am not sure about the issue you are experiencing , I have fixed issue with router trying go back on forth when I clicked on a menu option.

@gubertcalixto
Copy link

gubertcalixto commented Dec 5, 2019

I identified that this loop starts in reroute.js file
image
In my case performAppChanges reached 400 times until the page was available again

Edit 1: Also find out that it happens because of replaceState in navigation-event.js file, which calls reroute.js above methods.
image
As you can see, it happens in the following order:

  1. pushState (navigation-event.js)
  2. performAppChanges (reroute.js)
  3. replaceState (navigation-event.js)
  4. performAppChanges (reroute.js)
  5. pushState (navigation-event.js)
  6. LOOP BETWEEN performAppChanges and replaceState

Default behavior:

  1. pushState
  2. performAppChanges

The conflict occurs between following routes
https://my-url/route-before-problem/
https://my-url/route-trying-to-access
No other routes are involved in proccess (what I mean is that even though Angular makes redirects because of authorization it always happens between same two routes)

@gubertcalixto
Copy link

My current guess is this line. I think the second line
urlReroute(createPopStateEvent(state))
is runned after the second router change, causing unexpected behavior since browser is already changing again (to new url) and single-spa is processing the old url.
PS: JUST A GUESS, trying to debug it a little more.

@joeldenning
Copy link
Member

Thanks for helping to debug this - why would single-spa's code have the old url? Angular router should call replaceState with the new url, not the old one.

@gubertcalixto
Copy link

gubertcalixto commented Dec 8, 2019

What I meant was that single-spa reroute is happening during an router change, which causes the bug, but not sure about it at all. It feels like single spa is processing during another route change.

@gubertcalixto
Copy link

Test it out for about 2 hours. I'm creating an repo with the bug!
Notice that loop starts after "finishUpAndReturn" method is called twice, @joeldenning, any idea?

@gubertcalixto
Copy link

Created an repo that reproduces the bug @joeldenning

@joeldenning
Copy link
Member

joeldenning commented Dec 14, 2019

I am not going to look at this bug anytime soon. I do not use Angular for any projects ever and this bug is a hard one when I tried to fix it in the past. The only path forward for this to be fixed is community contribution.

@gubertcalixto
Copy link

gubertcalixto commented Dec 15, 2019

Notice something that might be the bug!!!

Base Code

this.router.navigateByUrl('threee').catch(res => {
    this.router.navigateByUrl('one');
});

The Result

image
This image shows the arguments send to replaceState and to pushState.

What I Found Out

  1. As you can see, urls has /test prefix
  • This is because I added provide: APP_BASE_HREF, useValue: '/test', making all urls for that app to have /test prefix (/test will be in activtyFunction of SingleSPA)
  1. What I'm interested in is that first url is /test/two, which means that Angular doesn't have changed route to threee.
  • Actually, /test/two is the current route in Angular App (before redirecting to any route at all)
  1. After that Angular itself keep activating replaceState and pushState, but only when singleSpa overrides this methods from window.history

PS: Even though I do not alter APP_BASE_HREF the bug keeps happening and Angular keeps sending "/two" other than "/threee"

PS 2: I'm thinking that I could fix the problem checking if url is the same as current url.
But I don't know if it can cause any other problems. Any other suggestions please 🙏?

@gubertcalixto
Copy link

gubertcalixto commented Dec 16, 2019

Notice that the problem is in the following line
If I delete the following line, bug is fixed.

@TheMcMurder (git blamed this code). I just need to know why is this code there. Because I not sure what might happen if I just delete it. And what you suggest, should I try to get why is pendingPromises making angular start a loop or just do something to fix this code (still don't know what)?

@TheMcMurder
Copy link
Contributor

Full disclosure: I don't use angular for any projects.

I don't recall specifically why that line of code exists, but this is a really complicated interaction between two different libraries: angular-routing, and single-spa. They both operate with different ideas about how routing is to be accomplished and who is in control of routing.

I'll try to take a look at the repo you created, but if I'm understanding correctly it's only consistently reproducible in IE which will make debugging extremely hard (I use linux as my daily driver). I wouldn't suggest deleting lines of code in single-spa as a solution. I know it's a slow response and I thank you for your continued patience.

@gubertcalixto
Copy link

gubertcalixto commented Jan 16, 2020

@TheMcMurder Actually is totally reproducible in Chrome / Firefox (using Windows at least). The bug happens 100% in my repo using the following settings:
Chrome v 79.0.3945.117 (64 bits)
Windows 10 (64 bits) v 18363.592
or in
Firefox v 72.0.1 (64-bits)
Microsoft Edge v 44.18362.449.0 (Microsoft EdgeHTML v 18.18363)
Blisk v 12.0.92.83 (Official Build) (32-bit)

IE: I didn't tried.

PS: I removed that line from single-spa and builded to test and works just fine

@OriolInvernonLlaneza
Copy link
Contributor

Hi, I can trigger the loop in my apps with Chrome and Ubuntu too.

@kylethebaker
Copy link

Also can confirm this happens in several of our angular micro-apps, running both the latest chrome and firefox nightly. It doesn't happen on every navigation event, but when it does happen it will happen consistently for a particular one.

This is something that I'm investigating as well, I'll post here if I figure anything out.

@kisslove
Copy link

the redirection infinite loop stills exist.any solutions?

@kisslove
Copy link

kisslove commented Jan 19, 2020

I try all solutions above,but failed. so i spend one day to solve the problem.luckily,it is fixed for me.
update file:src/navigation/navigation-events.js
details as my fork: https://github.com/kisslove/single-spa/blob/master/src/navigation/navigation-events.js

@gubertcalixto
Copy link

gubertcalixto commented Jan 19, 2020

@kisslove Could you explain your commit, why adding "isNavGoOrBack" made it work? I think your commit is about go back in browser bug, but actually the route loop can occur in others situations (in my repo I made an example only using 1 redirect after another)

@kisslove
Copy link

@gubertcalixto I add ‘isNavGoOrBack‘ ,that just make a distinction between click back-forward in browser and normal route. because the popstate event will be triggered by doing a browser action such as a click on the back or forward button (or calling history.back() or history.forward() in JavaScript).
Normal route will trigger history.pushState and history.replaceState,but go back in browser only trigger history.replaceState. the problem is history.pushState and history.replaceState both call urlReroute(createPopStateEvent(state)).obsolutely,it duplicates.

@kisslove
Copy link

kisslove commented Jan 20, 2020

@gubertcalixto I test your repo,it works after using my update.

@pankajparkar
Copy link

This issue only happens if user click on two different single spa link in a fraction of second. To fix this issue I disabled the links until any of the single page application is loading.

When any application loads via single-spa, it runs below events at start and end.
start - single-spa:before-routing-event
end - single-spa:routing-event
In between there are other events also gets called.

@HostListener('window:single-spa:before-routing-event')
beforeRouting() {
    const navbar: HTMLHtmlElement = document.querySelector('navbar');
    const activeLink: HTMLHtmlElement = document.querySelector('.nav-link.active')
    navbar.style.pointerEvents = 'none';
    activeLink.style.classList.add('loading');
}

@HostListener('window:single-spa:routing-event')
afterRoutingEvent() {
    setTimeout(_ => {
      const navbar: HTMLHtmlElement = document.querySelector('navbar');
      const activeLink: HTMLHtmlElement = document.querySelector('.nav-link.active')
      navbar.style.pointerEvents = 'none';
      activeLink.style.classList.remove('loading');
    }, 500);
}

@gubertcalixto
Copy link

@kisslove maybe you could create an PR with your correction for single spa. The bugs that I know/heard about was:

  1. Reproduced in my repo (fast change of route or using route promises)
  2. The bug you tested (navigate back/forward using browser actions)
  3. Another one that happened because of multiple angular redirects in production because of an router guard (but not sure if this one is the same of the tested in my repo).

PS: If you don't do it I probably will do in the next month (on vacation right now 😎)

@kylethebaker
Copy link

@pankajparkar The cases where I see it is when the back button is pressed or when doing a single direct navigation from using the router (not triggered by user interaction). It's possible though that either of those are causing cascading navigation events (guards, redirects, etc), so the theory of "too many navigation's too quickly" is valid.

I'm using the fork by @kisslove and it appears to be working, I did notice one time where the redirects started for a couple of bounces and then resolved themselves quickly, but that was single time out of several. So far I haven't seen any other negative side-effects or regressions within my apps from it.

@vaibhavarora14
Copy link
Contributor

@DmitryBrus you can try it at https://github.com/varora1406/coexisting-angular-microfrontends/tree/issue-inifnite-url-loop
I am also going to start work from today after office hours on this to fix 👩‍🚀

@gauravpandvia
Copy link

@joeldenning @arturovt After the version 3.4.1 fix of infinite redirection, it came back haunting again in our app. But this time it was weird, as it was not happening consistently but intermittently. Seeing the above comments as @DmitryBrus said it is happening when there are two close redirection routing in the angular app. Since in a more complex big app, it is quite visible and pertinent but I tried creating a small reproducible version of the problem here in this cloned template of coexisting-angular-frontend repo.

In /app1 I have used Angular v8 and single-spa-angular v3.6.0 and added a route named /test1 & /test2 in it. I have provided the link to Test1 Component in app.component.html and in Test1 component I have added a router navigation to Test2 component under ngOnInit. Observe that it happens only in the first time of app start, that when you will click on Test 1 it will go to test2 then back to test 1 then back to test2. Also, if you put a debug point on ngOnInit router navigate command in Test1 Component, you will notice that it is being paused infinite times. Hope this helps and we can get a solution of this.

@joeldenning
Copy link
Member

Thanks for the repo demonstrating the issue. I do not plan on working on this issue, but welcome anybody who does to discuss it here and/or submit a pull request.

jones-starz added a commit to jones-starz/single-spa-angular that referenced this issue Aug 3, 2020
@plrthink
Copy link

Any updates on this? I'm still facing this issue which causes infinite loop between two routes.

@ashuaggarwal94
Copy link

@plrthink You can try imports: [RouterModule.forRoot(routes, { onSameUrlNavigation: "ignore" })], in angular routing module it worked for me!! maybe you can give it a shot!!!

@plrthink
Copy link

Well, I don't think it would work since 'ignore' is the default value of the option onSameUrlNavigation.

@plrthink
Copy link

And it's indeed not working after testing.

@arturovt
Copy link
Collaborator

arturovt commented Nov 7, 2020

Hey guys, we have released the fix in 4.6.0 version. Could anyone check if it resolves your issue? @varora1406 could you also check in your example?

@joeldenning
Copy link
Member

Closing since the fix is published in 4.6.0 - we can reopen if that fix doesn't solve all cases.

@JudahMorrison
Copy link

Hello, single spa crew.
I have been dealing with this issue for a while and upgrading single-spa-angular to 4.7 and single-spa to 5.8.1 didn't seem to fix it for me.

@arturovt
Copy link
Collaborator

Hello, single spa crew.
I have been dealing with this issue for a while and upgrading single-spa-angular to 4.7 and single-spa to 5.8.1 didn't seem to fix it for me.

Could you paste your main.single-spa.ts?

@JudahMorrison
Copy link

import { enableProdMode, NgZone } from '@angular/core';

import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
import { Router } from '@angular/router';
import { ɵAnimationEngine as AnimationEngine } from '@angular/animations/browser';

import { singleSpaAngular, getSingleSpaExtraProviders } from 'single-spa-angular';

import { AppModule } from './app/app.module';
import { environment } from './environments/environment';
import { singleSpaPropsSubject } from './single-spa/single-spa-props';

if (environment.production) {
enableProdMode();
}

const lifecycles = singleSpaAngular({
bootstrapFunction: singleSpaProps => {
singleSpaPropsSubject.next(singleSpaProps);
console.log('BOOTSTRAP FUNCTION LANDING');
return platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule(AppModule);
},
template: '',
Router,
NgZone,
AnimationEngine,
});

export const bootstrap = lifecycles.bootstrap;
export const mount = lifecycles.mount;
export const unmount = lifecycles.unmount;

@arturovt
Copy link
Collaborator

I see, could you add NavigationStart to options in the same way you pass NgZone and Router?

@JudahMorrison
Copy link

@arturovt You are truly an everyday superhero.

@kmiasko
Copy link

kmiasko commented Nov 20, 2020

@arturovt Doesn't resolve this issue for our team too. For easier reproduction try setting CPU Throttling in Performance tab in developer settings.

I've also cloned @varora1406 repo, updated packages, added NavigationStart to start method and i can still reproduce this.

@arturovt
Copy link
Collaborator

arturovt commented Nov 20, 2020

@kmiasko

I've just cloned @varora1406 repo, updated packages, added NavigationStart to options and I can't reproduce it anymore (tho I could easily reproduce it earlier):

image

Could you provide a minimal reproducible example?

I also tried this script which clicks links 200 times automatically:

function sleep() {
  return new Promise(resolve => setTimeout(resolve));
}

window.tryToReproduce = async () => {
  // Only app1 and app2
  const links = [...document.querySelectorAll('navbar-primary-nav li a')].slice(1);

  for (let i = 0; i < 200; i++) {
    console.log('Navigating...');
    links[0].click();
    await sleep();
    links[1].click();
    await sleep();
  }
};

And ran it from my browser via tryToReproduce(), the issue is not reproducible for me.

@kmiasko
Copy link

kmiasko commented Nov 20, 2020

@arturovt Are you sure you're using issue-inifnite-url-loop branch?

@vaibhavarora14
Copy link
Contributor

vaibhavarora14 commented Nov 21, 2020

Hi guys, sorry I have been away for some time. I have actually switched from Angular to React from quite some time 🙂

@arturovt I can't see button added by me in your screenshot. Please confirm if you are checking at issue-infinite-url-loop branch.
image

@kmiasko
Copy link

kmiasko commented Nov 25, 2020

@arturovt My bad. I did everything again from the beginning, by the book on issue-inifnite-url-loop branch on @varora1406 example and it's working, there is no loop.

I've also solved our team issue with the loop. As we used angular sub app inside angular main app i had to also patch router in the main app using getSingleSpaExtraProviders() in platformBrowserDynamic.

Thank you for your time, and again sorry for the fuzz.

@arturovt
Copy link
Collaborator

@kmiasko I'm glad that you've solved it!

@chenji336
Copy link

single-spa-anggular 4.x this question.how to fix infinite loop by single-spa-angular 3.x ?

@joeldenning
Copy link
Member

single-spa-angular@3 doesn't have it fully fixed - see https://github.com/single-spa/single-spa-angular/pull/235/files#diff-d41e9b77a99ac86d9908fc97b9df8578479c310a0fe85f632f375b2d9e95f39cR202. We need to make a corresponding change in the 3.x branch to fix it. Would you be interested in submitting a pull request with it?

@nicknasirpour
Copy link

nicknasirpour commented Mar 22, 2021

Hi @chenji336 @joeldenning I will submit a pull request sometime this week implementing the fix for the 3.x branch.

Something of note is that this solution in single-spa-angular alone won't solve 100% of issues.

I have a base project, which is an angular module, where singleSpa.start() is called. My parcels are subsequently mounted onto a component of this project. In the base project, during a navigation from my login component to another component, I was getting an infinite loop related to this issue. The infinite loop behaviour stopped when I removed the call to singleSpa.start(). I had to implement the non-imperative skipping logic in my base application as well.

@binhtran04
Copy link
Contributor

Adding Router, NavigationStart and getSingleSpaExtraProviders like this would help for my case when using redirectTo in RouterModule:

const lifecycles = singleSpaAngular({
  bootstrapFunction: (singleSpaProps) => {
    singleSpaProps$.next(singleSpaProps);
    return platformBrowserDynamic().bootstrapModule(AppModule);
    return platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule(AppModule);
  },
  template: '<app-root />',
  Router,
  NavigationStart,
  NgZone,
});

It is also mentioned in docs https://single-spa.js.org/docs/ecosystem-angular/#api-updates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests