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

4-10 seconds delay between navigated component's Constructor and ngOnInit() #18816

Closed
kkotak opened this issue Aug 21, 2017 · 40 comments
Closed

Comments

@kkotak
Copy link

kkotak commented Aug 21, 2017

I'm submitting a...


[ X] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Initial load of the module happens quickly but, when navigated to a route, the constructor of the destination component is also called in a timely manner, but the ngOnInit() method is not called for another 4-10 seconds. This happens every time and with not just one but all components that are called using navigation.

Expected behavior

This issue only surfaced when I upgraded Angular from 4.0.1 to 4.3.4. No code was changed in the app.

Minimal reproduction of the problem with instructions

I'm unable to create a plunker to reproduce this issue with shell code. If someone can shed some light on what happens between the component's constructor being called and ngOnInit() being called, I may be able to debug this further.

EDIT- Notably, the delay is observed right after processing Google Auth response that is processed by a Service, which then calls router.navigate() to the component in question.

What is the motivation / use case for changing the behavior?

The motivation is obvious. The delay is not expected behavior.

Environment

Production code is built using the following command -
node --max_old_space_size=8192 ./node_modules/.bin/ng build --aot --prod


Angular version: X.Y.Z
@angular/cli: 1.3.0
node: 6.9.5
os: darwin x64
@angular/animations: 4.3.4
@angular/cdk: 2.0.0-beta.8
@angular/common: 4.3.4
@angular/compiler: 4.3.4
@angular/compiler-cli: 4.3.4
@angular/core: 4.3.4
@angular/flex-layout: 2.0.0-rc.1
@angular/forms: 4.3.4
@angular/http: 4.3.4
@angular/material: 2.0.0-beta.8
@angular/platform-browser: 4.3.4
@angular/platform-browser-dynamic: 4.3.4
@angular/platform-server: 4.3.4
@angular/router: 4.3.4
@angular/cli: 1.3.0



Browser:
- [ X] Chrome (desktop) version XX
- [ X] Chrome (Android) version XX
- [ X] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ X] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX   v6.9.5
- Platform:   Mac OSX latest.

Others:

@rvalimaki
Copy link

rvalimaki commented Aug 22, 2017

[wrong information here!] I can confirm this issue, and also report that the problem is in Angular v. 4.2.6, with two router related fixes:

[updated information] Actually the 4.2.0 is the broken version, so there has to be something that's different between 4.1.3 (last working version) and 4.2.0 (first broken version).

All prior versions, including 4.1.3 works for us, but 4.2.0 is broken for some of the links. Constructor for the new component gets called when an ordinary [router-link] is clicked immediately, but ngOnInit() is called only after sometimes very long delay, or by clicking everywhere on the screen multiple times (so it's not an easy change detector view update problem, I'm afraid).

Most of our links are working just fine, but there are some links that almost 100% consistently fails on Angular 4.2.0 and newer (tested versions include pretty much all Angular2 alpha/beta/rc and release versions, and eg. 4.1.3 and broken versions 4.2.0, 4.2.5, 4.2.6, 4.3.1 & 4.3.5).

Our application is rather huge, and I can't think of a way to reproduce this on plunker. At least these broken links are addressing a sub route inside a lazy loaded module, and they happen to have query params with them.

EDIT: IMPORTANT: I really don't understand how I got it "working" with 4.2.5. As I tested today, all versions from 4.2.0 are affected, and last working version is then 4.1.3. Probably some of the caches were not really cleaned during the test with 4.2.5

@rvalimaki
Copy link

rvalimaki commented Aug 22, 2017

Btw. we are not using Google Auth or similar. And we do have async ngOnInit methods, though the problem persists also when all async ngOnInit calls where made non-async.

Also the behaviour for us was that the old routed component stayed on the screen, but just taking the right half of the <router-outlet> space. When the new routed component finally came (and ngOnInit() got called), the old component disappeared and the new component took its place (as expected).

We do have guards for our "application level" or "module level" routes, but not any additional route guards for these particular sub routes. Links in guestion were inside one single guarded route.

As an additional note: it doesn't matter whether our ES output version is ES5, ES2015, ES2016 or ES2017 (which is otherwise somewhat broken still with Zone.js). Currently working combination we have is ES2016 output with Angular 4.1.3.

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

@rvalimaki do you have OnPush on your routed component, or it's holder?

@rvalimaki
Copy link

rvalimaki commented Aug 22, 2017

@tytskyi the link is inside our internal "grid" component` (kind of datatable), that happens to be OnPush, but these routed components are not ```OnPush```, but on default change detection instead.

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

@rvalimaki OnPush works hierarchically (at least for the regular components, i am not sure about the routed components). Could you try to remove all OnPush above problematic component?

What i am thinking - is that when router navigates to component - change detection does not happen. But then, in 4-10 seconds you touch something on the screen that triggers change detection on the whole app.

But this is only my guess

@rvalimaki
Copy link

@tytskyi I started trying that immediately after your first comment, but our app takes some ages to build... But now when I have Angular 4.3.5, and that particular "Grid" component with ChangeDetectionStrategy.Default, those links are unfortunately still not working correctly.

@rvalimaki
Copy link

rvalimaki commented Aug 22, 2017

@tytskyi I changed now all the remaining OnPush modes to Default on that lazy loaded module AND all the modules it does relate anyhow, but unfortunately the problem still persists.

I agree that change detection is related to that, and the "4-10 seconds" as described by @kkotak pretty much seems to be determined from external change detection changes. Eg. we have a sockjs connection with a heartbeat, and every time the heartbeat comes (eg. every 10 seconds or so), the routed component fixes itself immediately.

Those being said, I could probably overcome this problem by not using [router-link] but a custom click handler + router->navigate by myself, but I really don't want to do that. I rather use 4.2.5 for now and wait for a real fix. I'm eager to help with that, though I can't think of how those rather innocent looking changes from 4.2.5 to 4.2.6 can make things break.

EDIT: I was wrong, 4.1.3 is the latest working version, so 4.2.0 and later are not working.

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

@rvalimaki could you try to use https://angular.io/api/core/ChangeDetectorRef#markForCheck right in the constructor of the problematic component?
Or if not worked then https://angular.io/api/core/ChangeDetectorRef#detectChanges?

Maybe in combination with setTimeout(fn, 0)

This does not help to solve the problem, but at least we proof this is problem of change detection

@rvalimaki
Copy link

@tytskyi thanks, I tried that already last week, but calling changeDetectorRef inside the constructor resulted in fatal errors on Angular, effectively breaking the application.

I'll try again, using setTimeout() this time.

@kkotak
Copy link
Author

kkotak commented Aug 22, 2017

I can confirm that we see the same behavior as @rvalimaki of the old routed-out component staying on the screen despite the fact that ngDestroy was called on it. Also we're not using onPush for the routed-in component.

@rvalimaki
Copy link

rvalimaki commented Aug 22, 2017

Thanks @tytskyi

Indeed, calling ChangeDetectorRef markForCheck() + detectChanges() "fixes" the situation:

  constructor(private cdr: ChangeDetectorRef) {
    setTimeout(() => this.setChanged(), 0);
  }

  setChanged() {
    this.cdr.markForCheck();
    this.cdr.detectChanges();
  }

Without setTimeout(), everything breaks.

One of the problems with this workarounds is that we would need to repeat this in pretty much all components that can be possibly navigated to, which is plenty of components.

@kkotak
Copy link
Author

kkotak commented Aug 22, 2017

Confirming that the workaround suggested by @rvalimaki also 'fixes' the issue on our end. We also have the same concern over propagating the workaround across the app to multiple components. In our case, the routed-in component also relies on the data retrieved in the ngOnInit() method, hence even though the component is rendered, the data it relies on still takes the same long time to render. We wouldn't want to move all that code from ngOnInit() to the constructor.

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

Yeah, it is not the fix. It was just a bit of investigation...

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

Could be related to this #16883 (somehow not listed in CHANGELOG.md)

@rvalimaki
Copy link

After adding this workaround to only 58 most likely navigated "main page components" (that fortunately shared a common ancestor class) things seems to work pretty smooth for now...

I'm eager to help for debugging the actual issue more when someone investigates this issue. Just drop a note.

@tytskyi: spot on! #16883 seems to be the culprit. @vicb probably knows how to mitigate this issue.

@kkotak
Copy link
Author

kkotak commented Aug 22, 2017

It turns out to be a lot of workaround for us. @tytskyi - do you recommend we revert back to 4.2.5?

@tytskyi
Copy link

tytskyi commented Aug 22, 2017

@kkotak i cannot recommend to downgrade or upgrade, since i am just the user of angular.
And i don't have similar problem (i don't even use router-outlets), i just tried to narrow down the problem, because i saw that some time ago that PR i linked and i saw another issue with lifecycle hooks related to this changes (i can't find link to the issue now).

What i would recommend is to try to reproduce the same problem on the very minimal code so that it is easier to debug. However this does not guarantee that problem will be fixed immediately...

@kkotak
Copy link
Author

kkotak commented Aug 22, 2017

Thanks @tytskyi. Not sure how to reproduce it in minimal code. I did try with a template on Plunker but there are factors at play that I'm not able to pin down. Hopefully your pointer to #16883 will help someone in the team to start looking at it.

@rvalimaki
Copy link

@kkotak do you happen to have lazy loaded modules on your code?

And are those non-working routes subroutes of said lazy loaded modules?

@kkotak
Copy link
Author

kkotak commented Aug 23, 2017

@rvalimaki No we don't have lazy loaded modules in the app.

@rvalimaki
Copy link

@kkotak I got it wrong, it's 4.1.3 that's the last working version. Versions 4.2.0 onwards are broken.

@rvalimaki
Copy link

Well, it seems that after navigating to a component using that change detection workaround, the sub routes of that component do have similar problem still, and it doesn't go away using the same workaround for those sub route components (there are plenty of them!).

This time, when our sub routes are in "tabs" that you can click multiple times, the problem seems to be actually worse, since by clicking those tabs several times, you get multiple empty components under router-outlet like that:

<router-outlet></router-outlet>
<sub-a></sub-a>
<sub-b></sub-b>
<sub-a></sub-a>
<sub-b></sub-b>
<sub-a></sub-a>
<sub-b></sub-b>
<sub-a></sub-a>
<sub-b><!-- actual OLD content here, but others are empty --></sub-b>

After some time, when some other change detection happens somewhere, these components are gone and replaced by the actual one. Needless to say, this situation kind of sucks, and we have to revert to Angular version 4.1.3 for now.

Ps. we don't use observables unless they are really needed. We are using Promises with async/await for our data retrievals & also for our guards, since these are much more easier to use for our C#.NET developers that are trying to grasp TypeScript & Angular. TBH the async/await syntax is more clear for everyone other too. Just thinking if this is one of the many possibilities that could make it so that our routed components are not initialized as they should be.

@kkotak
Copy link
Author

kkotak commented Aug 24, 2017

Thanks @rvalimaki for the updates. I also confirm that 4.1.3 is the last working version. I am a bit surprised that the issue has gone unnoticed for so long by the Angular community. Makes me want to think that there must be something uniquely wrong with our design.

I also confirm that the workaround just kicks the can down the road to other components and MdDialog components, which show up empty. Our first hunch was that the issue might be related to the subscribe handler from where the navigate() call is made, but even after trying various alternatives and unsubscribes, etc. problem still persists.

Interestingly, in our case, if the workaround is not used and if you accept the first delay in navigation, the rest of the components, dialogs, etc. work just fine. In other words, the problem seems to only occur once.

I'm really hoping someone with the knowledge of the inner working of this area, would take a look. Downgrading back to 4.1.3 is a really painful path due to other 3rd party dependencies.

@vicb
Copy link
Contributor

vicb commented Aug 24, 2017

Closing as no repro.
If you can reproduce the issue with a sample demo app, re-open an issue. Thanks.

@vicb vicb closed this as completed Aug 24, 2017
@larssn
Copy link

larssn commented Aug 31, 2017

We're also seeing all kinds of weird issues with change detection as of late. Even had to do ".detectChanges()" a few places that makes no sense - in places that used to work just fine.

Currently I'm experiencing the same issue as OP: A routed component doesn't initialize until I cause a few other events. I'll try the constructor workaround.

EDIT.
Didn't work.

EDIT2:
So this is my "fix" before every router.navigation(...)

setTimeout(() => { console.log(); }, 10); // Hack for broken change detection
this.router.navigate(['somewhere'], { replaceUrl: true });

@rvalimaki
Copy link

@larrssn I'm afraid, that this constructor workaround just leads into bigger problems.

From now, I suggest downgrading to 4.1.3, as it's the last working Angular version. We have had also other change detection related problems with versions > 4.1.3, so we are currently staying on 4.1.3.

Now we are way too busy to get our applications released, so we cannot investigate this problem for now. Hopefully someday we can have the time to construct a working Plunker demo of this problem to @vicb to investigate.

Anyway, it seems that there are at least 3 companies having this problem.

@larssn do you happen to use async/await or promises on your code, or do you use observables instead?

@larssn
Copy link

larssn commented Sep 1, 2017

@rvalimaki This particular project has no async/await in it (which I believe gets transpiled out anyway), and mostly Observables instead.

I tried downgrading to 4.1.3, but couldn't figure out the correct mix of dependencies to avoid weird compiler specific errors.

Our workaround is sufficient for now. 👍

@marekozw
Copy link

I have also faced this behavior. Following workaround works for me:

constructor (
  private zone: NgZone,
  private changeDetector: ChangeDetectorRef
) {
  setTimeout(() => {
    this.zone.run(() => {
      this.changeDetector.detectChanges();
    });
  });
}

Very similar to @tytskyi suggestion.

I know that putting zone inside timeout may look weird. This is my result of trial and error process. Looks like this task has to be put on queue first and then wrapped inside zone.

Caveats:

  • because we are manually rushing change detection, the component might have some problems with @Inputs. I had to assign default values to my inputs in order to make it work.

@Input() public site: Site;
changed to
@Input() public site: Site = {};

  • I also had to move all my observable definitions from ngOnInit to constructor (same reason as above).
constructor() {}

public onOnInit(): void {
  this.selectedInstance$ = this.selectedInstanceSubject$
    .distinctUntilChanged()
}

changed to

constructor() {
  this.selectedInstance$ = this.selectedInstanceSubject$
    .distinctUntilChanged()
}

public onOnInit(): void {}

Looking forward to get it sorted.

@rvalimaki
Copy link

rvalimaki commented Sep 29, 2017

"Looking forward to get it sorted."

Well, without working Plunker sample and this bug affecting only 3 companies so far, it's not going to get solved, and that's also the reason the issue closed for now. No assignees on this issue as well.

We are sticking to working version 4.1.3 until we have enough time to investigate more on this issue. In some distant future, maybe.

@sidleal
Copy link

sidleal commented Sep 29, 2017

Same problem here... in my case this behaviour just happens when I'm logged with google auth, when logged with firebase email/pwd I have no delay.
It's my first project with angular, so I don't know if I'm doing something stupid. But the cited workaround works fine for me too. Thanks.

  refresh() {
    setTimeout(() => {
      this.zone.run(() => {
        this.cdr.detectChanges();
      });
    });  
  }

@samuelmoser
Copy link

I have the exact same problem as @sidleal. I have a workaround in the component in which a router event is started
<li routerLink="/mock" routerLinkActive="active"><a>Mock</a></li>
the workaround with tick() is in the constructor:
constructor(private router: Router, ref: ApplicationRef) { router.events .filter(event => event instanceof NavigationEnd) .subscribe(x => { ref.tick(); });

@krahs1
Copy link

krahs1 commented Nov 14, 2017

It would be nice to have a real fix for this as we also have seen this. In our case we are using this.router.navigateByUrl(relativeUrl) but see the exact same behavior. Is the issue to get it fixed a working sample or the number of people seeing the issue?

The Solution marekozw posted here worked for us. I posted it below for reference. Other solutions posted here worked but had the issue where sub component loading was also delayed.

constructor (
private zone: NgZone,
private changeDetector: ChangeDetectorRef
) {
setTimeout(() => {
this.zone.run(() => {
this.changeDetector.detectChanges();
});
});
}

Thanks marekozw this was a big help!

@rvalimaki
Copy link

@krahs1

Which Angular version you are using? I just tested with Angular 5.0.0 and somehow links and router-outlets seems to be magically working, at least until we got the infamous "EXCEPTION: Expression has changed after it was checked" ((. After that, all router-outlets will duplicate components when navigated to.

@krahs1
Copy link

krahs1 commented Nov 16, 2017

We have recently moved to 5 to try to get past the issue. I am trying to construct a simple example trying to duplicate our situation. I will let you know if I am able to dup it.

@danail-vasilev
Copy link

danail-vasilev commented Feb 14, 2018

Hi Guys,
I am having the same issue:

  1. Using a grid component (ngx-datatable) - image.
  2. One of the grid columns has a template with a routerLink.
  3. When I click on the link a page with a component should be loaded but its ngOnInit event is not triggered at all.

The interesting thing is that if I click on the link that is on a visible grid row it opens properly but if I scroll down the grid and then click on the link it doesn't load the component properly.

The proposed workaround by marekozw works properly on my side.

I will try to reproduce the issue in a runnable sample maybe next week if I have some time.

Thanks !

@ghost
Copy link

ghost commented Mar 2, 2018

Hi, I am having exactly the same problem, the workaround worked for me also.

@danail-vasilev, I am in the same situation as you, using that grid component bugs the navigation. Did you find any solution yet?

Thanks!

@danail-vasilev
Copy link

Hi @cfacello,
The issue is reproducible when the grid has a scroller. Our designer requested to use an infinite scroll of the page, so currently I am not observing this issue. Hence I postponed the issue isolation.
Regards,

@ghost
Copy link

ghost commented Mar 2, 2018

@danail-vasilev thanks for the answer.

Anyway, I searched a bit forward and found something important:
The bad: Is the datatable itself having an issue. swimlane/ngx-datatable#1232
The good: The user who posted the issue, created a fork of the swimlane datatable, fixing the issue. I just tested it and it works. At least until the swimlane guys officially fix it.

@jyosh
Copy link

jyosh commented May 4, 2018

I submitted a new bug related to this with a demo app - #23697
Looking for more insights on that

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests