single-spa / single-spa-angular

Helpers for building single-spa applications which use Angular
Apache License 2.0
203 stars 78 forks source link

URL Redirection Infinite Loop #113

Closed AndrewGlazier closed 4 years ago

AndrewGlazier commented 5 years ago

Issue seems to relate to this comment; https://github.com/CanopyTax/single-spa-angular/issues/94#issuecomment-520550455 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 commented 4 years ago

The fix for this is released for Angular 9 projects under the alpha dist tag. See https://github.com/single-spa/single-spa-angular/releases/tag/v4.1.0-alpha.0.

We plan on backporting the fix to single-spa-angular@3, as shown in #192. I'll post here once a new 3.x is released.

Could people please try out 4.1.0-alpha.0 and let us know if it fixes the issue for them? We'd like to get as many people verifying that it's fixed as possible.

joeldenning commented 4 years ago

One note about the fix:

You must upgrade to single-spa@>=5.4.0 inside of your root config for the fix to work!

joeldenning commented 4 years ago

This fix has been backported for Angular 2-8 via https://github.com/single-spa/single-spa-angular/releases/tag/v3.4.0. Note that you must add the extra single-spa providers to your code in order for it to work.

Please comment here after having tried it out to let us know if it's working for everyone. We are keeping this issue open until we have a few people confirm that it's fixed

gubertcalixto commented 4 years ago

Tried to use the v3.4.1 in my repo and unfortunately had the following error image Thing I did before test it:

  1. Update single-spa.min.js import from System.import('https://cdnjs.cloudflare.com/ajax/libs/single-spa/4.3.5/system/single-spa.min.js') to System.import('https://cdnjs.cloudflare.com/ajax/libs/single-spa/5.5.0/system/single-spa.min.js')

  2. Updated single-spa-angular package to 3.4.1

  3. Updated main.single-spa.ts file as indicated in release notes

joeldenning commented 4 years ago

This looks like an error with differential loading (es5 vs es2015). single-spa-angular@3 doesn't use ng-packagr right now - @arturovt can you comment on what our options are to support differential loading for single-spa-angular@3?

@gubertcalixto what you can try is to use the es2015 version of your bundled output while testing the bug - that one will call classes with new correctly

gubertcalixto commented 4 years ago

Test it over again, this time building the app and accessing main-es5.js and it worked just fine (main-es2015.js did not work, with same error as above). Anyway, this button below finally does not make infinite loop anymore.
image

THANK YOU GUYS 👍

arturovt commented 4 years ago

I was testing so fast that I forgot about differential loading.... :grimacing:

I will think about what we can do with that.

vaibhavarora14 commented 4 years ago

For me as well, this fix of alpha release is good till now.

joeldenning commented 4 years ago

For those using single-spa-angular@3, we released https://github.com/single-spa/single-spa-angular/releases/tag/v3.5.0 which has support for differential loading. With all the feedback positive so far on the 4.x alpha release, I think we should just release this as latest. @arturovt do you agree?

arturovt commented 4 years ago

I would want to hear feedback from @matt-gold since he was very active in helping to test out our code changes.

joeldenning commented 4 years ago

This fix is published under the latest dist-tag. Release notes at https://github.com/single-spa/single-spa-angular/releases/tag/v4.2.0.

matt-gold commented 4 years ago

@arturovt The bug is fixed for me on single-spa-angular 4.2.0 + single-spa 5.4.2 👍

vaibhavarora14 commented 4 years ago

Was working on this issue, it's still reproducible. You can test below code and find it on Mozilla firefox https://github.com/varora1406/coexisting-angular-microfrontends/tree/issue-inifnite-url-loop

joeldenning commented 4 years ago

Reopening this while we investigate the repo provided by @varora1406.

arturovt commented 4 years ago

@varora1406 you didn't provide any steps to reproduce it. What do I have to do?

vaibhavarora14 commented 4 years ago

@arturovt sorry for that. just try to switch randomly with routes fast. You will get the results in a minute or two most probably. Remember to use Mozilla Firefox, to get it reproducible easily

vaibhavarora14 commented 4 years ago

I have also added a button in latest commit, which can help you recreate bug easily

joeldenning commented 4 years ago

Someone contacted me in the single-spa slack workspace this morning saying that they had found a change that might work as a solution. The change is to modify skipNextPopState to be a number that acts as a counter - every time pushState/replaceState is called, it is incremented; and every time that popstate is fired it is decremented. He said that it's possible for multiple pushStates and replaceStates to pile up while waiting for the popstate event.

The implementation approach seems good to me - I encouraged him to open up a pull request with the change, where we can discuss it.

vaibhavarora14 commented 4 years ago

@joeldenning no pull request is opened yet for this issue. can you share implementation approach details?

joeldenning commented 4 years ago

Hi @varora1406, my previous comment details the implementation. I'll add a few more links for anyone who's interested in submitting a pull request with the change:

https://github.com/single-spa/single-spa-angular/blob/a4cc1633bf0b8f39c57484a51e7895561818c37b/src/src/extra-providers.ts#L15

https://github.com/single-spa/single-spa-angular/blob/a4cc1633bf0b8f39c57484a51e7895561818c37b/src/src/extra-providers.ts#L54-L55

https://github.com/single-spa/single-spa-angular/blob/a4cc1633bf0b8f39c57484a51e7895561818c37b/src/src/extra-providers.ts#L36-L37

https://github.com/single-spa/single-spa-angular/blob/a4cc1633bf0b8f39c57484a51e7895561818c37b/src/src/extra-providers.ts#L63

The skipNextPopState variable should be changed to an integer instead of a boolean. It should be incremented when pushState/replaceState are called, and decremented whenever onPopState is called. We should only call ngZone.run() when it is 0

I'd gladly review a PR that implements the change.

arturovt commented 4 years ago

It'd also be great if we reproduce that behavior in our tests before making that change. And after the change is done we'll be able to see if that code change really fixes that issue.

joeldenning commented 4 years ago

Here's the conversation from Slack about this.

image

I don't know Dmitry's Github handle, but if you're watching this thread Dmitry could you please comment on whether your solution has worked?

DmitryBrus commented 4 years ago

@joeldenning I didn't observe infinite loop, only one redundant redirection. Before opening PR I'd like to be sure that I can clearly reproduce the issue, unfortunately I do not see it happen on new repo, I tried and played around, set two redirection close one another but still no luck. The only place it happen for me in our pretty big app, I tried to remove one by one all irrelevant parts and see if I can get clear reproduction... still in progress.

vaibhavarora14 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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.

plrthink commented 4 years ago

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

ashuaggarwal94 commented 4 years ago

@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 commented 4 years ago

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

plrthink commented 4 years ago

And it's indeed not working after testing.

arturovt commented 4 years ago

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 commented 4 years ago

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

JudahMorrison commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

JudahMorrison commented 4 years ago

@arturovt You are truly an everyday superhero.

kmiasko commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

vaibhavarora14 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

chenji336 commented 3 years ago

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

joeldenning commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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