refactor(router): remove extra CD run on activation #16510

vicb · 2017-05-03T15:27:20Z

Problem

Before this PR, the information about the child outlet were stored in a RouterOutletMap on the parent RouterOutlet directive.
This means that all the RouterOutlets need to be created when a route is activated.
In order to instantiate the nested RouterOutlet, the CD need to run each time a segment is activated in order to create the nested RouterOutlet.

Running the CD multiple times causes trouble with the animation.

Solution

As we do not want to run the CD multiple times, we can not rely any more on the outlets being present when a route is activated. Then we can not store the RouterOutletMap on the RouterOutlet.

RouterOutletMap now forms a tree that contain all the information about the currently active route.

When a route is activated, if the RouterOutlet is already instantiated it is immediately activated.
However when the RouterOutlet is instantiated yet, it will be activated in its ngOnInit lifecycle hook.

Other notes

In the former implementation it was possible to detect when a template misses a RouterOutlet because they must be present while the route is activated.

With this PR it is not possible to detect that any more because RouterOutlet do not have to be instantiated while a Route is activated.

I think that the behavior after the PR is more reasonable:

<ng-container *ngIf="cond">
  <router-outlet></router-outlet>
</ng-container>

Before:

if cond was false while the route is activated this would be an error as the outlet would not exist when the route is activated,
if cond was true then route activation would work fine. Later if cond becomes false, the outlet would be destroyed and children information stored on the directive (in the RouterOutletMap) would be destroyed. If cond become true again the outlet would be empty because that info has been lost.

closes #16423

TODO

add more tests for the attach/detach scenario
validate with animation
validate within g3
discuss about breaking changes

vsavkin

Left some questions.

My main question is the following:

Should not we create a tree of OutletContext altogether ahead of time, basically by writing this function:RouterStateSnapshot => OutletContext.

Every node will have some nulls which will be filled in during activation, but the structure of the tree should be known ahead of time.

Right now you assemble the tree as you go, and I think it makes the code harder to follow.

What do you think?

vsavkin · 2017-05-06T18:19:48Z

packages/router/src/router_outlet_map.ts

+  }
+
+  getContext(childName: string): OutletContext|null {
+    const info = this.contexts.get(childName);


maybe rename info into context for consistency

right, I missed this renaming.

vsavkin · 2017-05-06T18:20:36Z

packages/router/src/router_outlet_map.ts

-  registerOutlet(name: string, outlet: RouterOutlet): void { this._outlets[name] = outlet; }
+  onChildOutletDestroyed(childName: string): void {
+    const context = this.getContext(childName);
+    if (context) {


when would it be null?

Agreed. This is to please TS as getContext could return null. We should really have an assert

vsavkin · 2017-05-06T18:24:33Z

packages/router/src/router_outlet_map.ts

   */
-  removeOutlet(name: string): void { this._outlets[name] = undefined !; }


RouterOutletMap is a public API, so removing this is a breaking change.

vsavkin · 2017-05-06T18:25:35Z

packages/router/src/router_outlet_map.ts

+  resolver: ComponentFactoryResolver|null = null;
+  outletMap = new RouterOutletMap();
+  attachRef: ComponentRef<any>|null = null;
+}

 /**
 * @whatItDoes Contains all the router outlets created in a component.
 *
 * @stable
 */
 export class RouterOutletMap {


Since you already making it a breaking change, do you think it makes sense to rename this class into something like OutletContextChildren. I think this name would work better.

Agreed.

I still need to discuss how to handle breaking changes for this PRs. I have refrained from doing more breaking changes in this PR before discussing with the team.

vsavkin · 2017-05-06T18:29:30Z

packages/router/src/directives/router_outlet.ts


  @Output('activate') activateEvents = new EventEmitter<any>();
  @Output('deactivate') deactivateEvents = new EventEmitter<any>();

  constructor(
      private parentOutletMap: RouterOutletMap, private location: ViewContainerRef,
-      private resolver: ComponentFactoryResolver, @Attribute('name') private name: string) {
-    parentOutletMap.registerOutlet(name ? name : PRIMARY_OUTLET, this);
+      private resolver: ComponentFactoryResolver, @Attribute('name') name: string) {


maybe you could do @Attribute('name') private name: string = PRIMARY_OUTLET

vsavkin · 2017-05-06T18:34:48Z

packages/router/src/directives/router_outlet.ts

-  ngOnDestroy(): void { this.parentOutletMap.removeOutlet(this.name ? this.name : PRIMARY_OUTLET); }
+  ngOnDestroy(): void { this.parentOutletMap.onChildOutletDestroyed(this.name); }
+
+  ngOnInit(): void {


Why is it done in this ngOnInit and not in the constructor? Cause the parent's change detection will run before either constructor or ngOnInit.

And in the constructor this.activated will always return false.

vsavkin · 2017-05-06T18:37:41Z

packages/router/src/directives/router_outlet.ts

+  ngOnDestroy(): void { this.parentOutletMap.onChildOutletDestroyed(this.name); }
+
+  ngOnInit(): void {
+    if (!this.activated) {


Since there are three levels of nested if, maybe add a comment to each branch, so it is clear what triggers each condition.

vsavkin · 2017-05-06T18:43:19Z

packages/router/src/router.ts

-
-        // if we have a componentless route, we recurse but keep the same outlet map.
+        // If we have a normal route, we need to go through an outlet.
+        const context = parentOutletMap.getOrCreateContext(future.outlet);


Seems strange that we can create a new context during deactivation. Should not it only happen during activation?

mary-poppins · 2017-05-15T22:56:09Z

The angular.io preview for 27f1628 is available here.

mary-poppins · 2017-05-18T00:00:51Z

The angular.io preview for 62b0014 is available here.

mary-poppins · 2017-05-18T01:06:40Z

The angular.io preview for 2135bcb is available here.

… is activated

mary-poppins · 2017-05-18T01:34:36Z

The angular.io preview for 8d3e6f4 is available here.

- don't run change detection every time an outlet is activated - drop `routerOutletMap`s caused by angular/angular#16510

angular-automatic-lock-bot · 2019-09-11T15:20:47Z

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

vicb added area: router action: review The PR is still awaiting reviews from at least one requested reviewer labels May 3, 2017

vicb requested a review from mhevery May 3, 2017 15:27

googlebot added the cla: yes label May 3, 2017

vicb force-pushed the 0503-router branch 2 times, most recently from 2cc5f42 to 6a57a24 Compare May 4, 2017 15:14

vicb changed the title ~~refactor(router): misc~~ [WIP] refactor(router): remove extra CD run on activation May 4, 2017

vicb added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 4, 2017

vicb force-pushed the 0503-router branch 3 times, most recently from 5cd66ea to cf6d4a1 Compare May 5, 2017 13:02

mhevery approved these changes May 5, 2017

View reviewed changes

vsavkin reviewed May 6, 2017

View reviewed changes

vicb force-pushed the 0503-router branch 2 times, most recently from 1f831ff to e660488 Compare May 10, 2017 14:06

vicb changed the title ~~[WIP] refactor(router): remove extra CD run on activation~~ refactor(router): remove extra CD run on activation May 15, 2017

vicb force-pushed the 0503-router branch from e660488 to 27f1628 Compare May 15, 2017 22:47

refactor(router): cleanup, simplification

e69bce9

vicb force-pushed the 0503-router branch from 27f1628 to 62b0014 Compare May 17, 2017 23:53

vicb added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels May 17, 2017

vicb force-pushed the 0503-router branch from 62b0014 to 2135bcb Compare May 18, 2017 00:59

refactor(router): don't run the change detection every time an outlet…

8d3e6f4

… is activated

vicb force-pushed the 0503-router branch from 2135bcb to 8d3e6f4 Compare May 18, 2017 01:27

matsko merged commit 198edb3 into angular:master May 18, 2017

vicb mentioned this pull request May 19, 2017

refactor(router): don't run the change detection every time an outlet is activated #16883

Merged

vicb deleted the 0503-router branch May 19, 2017 17:26

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(router): remove extra CD run on activation #16510

refactor(router): remove extra CD run on activation #16510

vicb commented May 3, 2017 •

edited

Loading

vsavkin left a comment

vsavkin May 6, 2017

vicb May 6, 2017

vsavkin May 6, 2017

vicb May 6, 2017

vsavkin May 6, 2017

vsavkin May 6, 2017

vicb May 6, 2017

vsavkin May 6, 2017

vsavkin May 6, 2017

vsavkin May 6, 2017

vsavkin May 6, 2017

mary-poppins commented May 15, 2017

mary-poppins commented May 18, 2017

mary-poppins commented May 18, 2017

mary-poppins commented May 18, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

		*/
		removeOutlet(name: string): void { this._outlets[name] = undefined !; }

refactor(router): remove extra CD run on activation #16510

refactor(router): remove extra CD run on activation #16510

Conversation

vicb commented May 3, 2017 • edited Loading

vsavkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 15, 2017

mary-poppins commented May 18, 2017

mary-poppins commented May 18, 2017

mary-poppins commented May 18, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

vicb commented May 3, 2017 •

edited

Loading