fix(router): deactivate outlet if already activated when calling acti…

[gigadie]

fix(router): deactivate outlet if already activated when calling activateWith

Instead of throwing an error, we are now able to deactivate the outlet before activating it again with a new route. This makes routing configuration more flexible when we have named outlets we want to re-use on different routes with nested child routes

Closes #20694

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

I described the current behavior in the issue #20694, the router throws an error when the user tries to navigate from one route /home to another /about and the routing config is defined as follows:

export const routes = [
{
    path: '',
    component: IndexComponent, 
    children: [
        {
            path: 'home',
            children: [
                { path: '', component: FirstComponent, outlet: 'first', data: { state: 'a' } },
                { path: '', component: SecondComponent, outlet: 'second', data: { state: 'b' } }
            ]
        },
        {
            path: 'about', 
            children: [
                { path: '', component: FirstComponent, outlet: 'first', data: { state: 'b' } },
                { path: '', component: SecondComponent, outlet: 'second', data: { state: 'a' } }
            ]
        }
    ]
}

Issue Number: #20694

What is the new behavior?

With the new behavior, instead of throwing an error, it simply deactivates the current outlet route, before activating the new one.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

[gigadie]

I signed it!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

[googlebot]

CLAs look good, thanks!

[gigadie]

Hi there, it seems Travis failed because of the issues they're having with Chrome at the moment. If there is a chance to restart the build it might pass (I can't).

Thank you

[Riobe]

Why did you change the engine tag in the package.json. I don't really see how that's related to a PR about routing?

[gigadie]

@Riobe that was my mistake. Reverted the changes.

[iammikecohen]

Will this get merged in?

[chaseWillden]

@gigadie When is this going to get merged in? We are facing the same issues.

[nihique]

We have also same problem and fix above helps, what are the plans for merging it in?

[alexma01]

when will the fix be done?

[danigt91]

When will this be merged guys?

[Ni55aN]

I found solution (not the best but completely working)

@ViewChild(RouterOutlet) outlet: RouterOutlet;

constructor(
	private router: Router
) { }

ngOnInit(): void {
	this.router.events.subscribe(e => {
		if (e instanceof ActivationStart && e.snapshot.outlet === "admin-panel")
			this.outlet.deactivate();
	});
}

[sliekens]

This change doesn't fix the underlying problem: router outlets sometimes remain "activated" after navigating away from them.

Throwing an error is still the correct behavior when an outlet is indeed active and something tries to activate it again.

There has to be another way to fix this problem.

[StefanRein]

My effect of problem

In my case we dynamically create modal dialogs with a named router outlet in there.
After the modal dialog and his components are destroyed, the named router outlet should be, too.
Since I reuse the name if I create the dialog again, he uses the outlet name, get the outlet reference from the map and it throws this activated error.

angular/packages/router/src/router_outlet_context.ts

Line 52 in ab870e0

context.outlet = null;

If I just delete my outlet from the map it works for my case:

this.contexts.delete(childName);

I understand the OutletContext needs to be still referenced for the use case of the NgIf for restoring.
But in the case it is "valid" destroyed, shouldn't it be removed from the map?
If it should not be removed from the map, how to get rid of the reference?

For me as user this is first irritating:
Error: Cannot activate an already activated outlet

Especially because I explicitly call deactivate on my referenced RouterOutlet and it then says it is still activated?

export class FooComponent implements OnInit, AfterViewInit, OnDestroy {
    @ViewChild(RouterOutlet)
    public routerOutlet: RouterOutlet;

    constructor(private router: Router) {
    }

    public ngOnInit() {
        // This one works actually:
        // this.router.events.subscribe(e => {
        //     if (e instanceof ActivationStart && e.snapshot.outlet === FinancesRoutingRouteNames.PaymentOutlet) {
        //         this.routerOutlet.deactivate();
        //     }
        // });
    }

    public ngAfterViewInit() {
        console.log(this.routerOutlet);
    }

    public ngOnDestroy() {
        this.routerOutlet.deactivate();
    }
}

Then if you look, you see that the messages means the OutletContext and not the RouterOutlet.

angular/packages/router/src/directives/router_outlet.ts

Line 53 in ab870e0

parentContexts.onChildOutletCreated(this.name, this);

Here it sets the RouterOutlet on the parentContexts (private parentContexts: ChildrenOutletContexts) which calls:

angular/packages/router/src/router_outlet_context.ts

Line 38 in ab870e0

onChildOutletCreated(childName: string, outlet: RouterOutlet): void {

Which then creates or gets a already existing OutletContext:

angular/packages/router/src/router_outlet_context.ts

Line 72 in ab870e0

context = new OutletContext();

---

More investigation

Maybe this is the wrong direction but I see the activateWith method of the RouterOutlet called in exactly two places.

1# place:
https://github.com/angular/angular/blob/master/packages/router/src/directives/router_outlet.ts#L69
2# place:

angular/packages/router/src/operators/activate_routes.ts

Line 187 in 9523991

context.outlet.activateWith(future, cmpFactoryResolver);

First time my outlet is created it is not firing the activateWith in the ngOnInit method.
Second time the outlet is created, it comes in the ngOnInit method and activates.

Then this is called in the ActivateRoutes class (2# place:):

// outlet: RouterOutlet | null
if (context.outlet) {
    // Activate the outlet when it has already been instantiated
    // Otherwise it will get activated from its `ngOnInit` when instantiated 
    context.outlet.activateWith(future, cmpFactoryResolver);
}

Then I remembered this method: onChildOutletDestroyed
Where following is called:

angular/packages/router/src/router_outlet_context.ts

Line 52 in ab870e0

context.outlet = null;

Questions:

Is this related:
if (context.outlet) from the activateRoutes method and context.outlet = null; in the onChildOutletDestroyed method?
Can someone into it provide information how the flow should be?
Why do we need the activateWith call in the else block in the ngOnInit method?

Possible solution

If I just remove the activateWith call in the else block in the ngOnInit method of the RouterOutlet everything seems to work as expected in my application.

https://github.com/angular/angular/blob/master/packages/router/src/directives/router_outlet.ts#L69

@jasonaden @vicb

Another side effect

My Router Outlets from the URL aren't removed by refreshing in the browser. So I got problems also with this and wrote the following:

    // export const PRIMARY_OUTLET = 'primary';
    private clearRouterOutlets() {
        const urlTree = this.router.createUrlTree([]);
        const clearingOutlets: {[outlet: string]: null} = {};
        const primaryOutletName = PRIMARY_OUTLET;

        // Get all router outlet names except the primary router outlet and remove them.
        Object.keys(urlTree.root.children)
              .filter(outletName => outletName !== primaryOutletName)
              .forEach(remainingOutletName => clearingOutlets[remainingOutletName] = null);

        this.router.navigate([
            {
                outlets: clearingOutlets
            }
        ]);
    }

This is then also working and the (outlet) urls queries are removed on refresh of the page. But before the Error was thrown again. But with the above suggestion it is also working.

[georgique]

I experience the same issue as @StefanRein described.

[maxime1992]

Running into the exact same case as @StefanRein

The description there is 👌

[David-van-der-Sluijs]

What happened, guys?

[David-van-der-Sluijs]

Any news on this matter?

[stephanmullerNL]

Anything we can do to help this move forward?

[atscott]

Closing, for two main reasons:

• This has no tests
• It doesn't address the root cause and is in an incomplete solution/band-aid.

The issue happens before we get to the activateWith. The question to ask in order to find the root cause is "Why wasn't this route already deactivated"? In fact, if you were to navigate to a route that doesn't activate these outlets again, you would still encounter a bug: those routes still never get deactivated. Please refer to #40196 for the full fix.

[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.}