fix(router): ensure routerLinkActive updates when associated routerLi…

…nks change (#38511)

This commit introduces a new subscription in the `routerLinkActive` directive which triggers an update
when any of its associated routerLinks have changes. `RouterLinkActive` not only needs to know when
links are added or removed, but it also needs to know about if a link it already knows about
changes in some way.

Quick note that `from...mergeAll` is used instead of just a simple
`merge` (or `scheduled...mergeAll`) to avoid introducing new rxjs
operators in order to keep bundle size down.

Fixes #18469

PR Close #38511

goldens/public-api/router/router.d.ts

@@ -378,7 +378,7 @@ export declare class RouterEvent {
     url: string);
 }
 
-export declare class RouterLink {
+export declare class RouterLink implements OnChanges {
     fragment: string;
     preserveFragment: boolean;
     /** @deprecated */ set preserveQueryParams(value: boolean);
@@ -394,6 +394,7 @@ export declare class RouterLink {
     };
     get urlTree(): UrlTree;
     constructor(router: Router, route: ActivatedRoute, tabIndex: string, renderer: Renderer2, el: ElementRef);
+    ngOnChanges(changes: SimpleChanges): void;
     onClick(): boolean;
 }
 
@@ -429,7 +430,7 @@ export declare class RouterLinkWithHref implements OnChanges, OnDestroy {
     target: string;
     get urlTree(): UrlTree;
     constructor(router: Router, route: ActivatedRoute, locationStrategy: LocationStrategy);
-    ngOnChanges(changes: {}): any;
+    ngOnChanges(changes: SimpleChanges): any;
     ngOnDestroy(): any;
     onClick(button: number, ctrlKey: boolean, metaKey: boolean, shiftKey: boolean): boolean;
 }

packages/router/src/directives/router_link.ts

@@ -7,8 +7,8 @@
  */
 
 import {LocationStrategy} from '@angular/common';
-import {Attribute, Directive, ElementRef, HostBinding, HostListener, Input, isDevMode, OnChanges, OnDestroy, Renderer2} from '@angular/core';
-import {Subscription} from 'rxjs';
+import {Attribute, Directive, ElementRef, HostBinding, HostListener, Input, isDevMode, OnChanges, OnDestroy, Renderer2, SimpleChanges} from '@angular/core';
+import {Subject, Subscription} from 'rxjs';
 
 import {QueryParamsHandling} from '../config';
 import {Event, NavigationEnd} from '../events';
@@ -115,7 +115,7 @@ import {UrlTree} from '../url_tree';
  * @publicApi
  */
 @Directive({selector: ':not(a):not(area)[routerLink]'})
-export class RouterLink {
+export class RouterLink implements OnChanges {
   /**
    * Passed to {@link Router#createUrlTree Router#createUrlTree} as part of the `NavigationExtras`.
    * @see {@link NavigationExtras#queryParams NavigationExtras#queryParams}
@@ -167,6 +167,9 @@ export class RouterLink {
   private commands: any[] = [];
   private preserve!: boolean;
 
+  /** @internal */
+  onChanges = new Subject<RouterLink>();
+
   constructor(
       private router: Router, private route: ActivatedRoute,
       @Attribute('tabindex') tabIndex: string, renderer: Renderer2, el: ElementRef) {
@@ -175,6 +178,13 @@ export class RouterLink {
     }
   }
 
+  /** @nodoc */
+  ngOnChanges(changes: SimpleChanges) {
+    // This is subscribed to by `RouterLinkActive` so that it knows to update when there are changes
+    // to the RouterLinks it's tracking.
+    this.onChanges.next(this);
+  }
+
   /**
    * Commands to pass to {@link Router#createUrlTree Router#createUrlTree}.
    *   - **array**: commands to pass to {@link Router#createUrlTree Router#createUrlTree}.
@@ -298,6 +308,9 @@ export class RouterLinkWithHref implements OnChanges, OnDestroy {
   // TODO(issue/24571): remove '!'.
   @HostBinding() href!: string;
 
+  /** @internal */
+  onChanges = new Subject<RouterLinkWithHref>();
+
   constructor(
       private router: Router, private route: ActivatedRoute,
       private locationStrategy: LocationStrategy) {
@@ -336,8 +349,9 @@ export class RouterLinkWithHref implements OnChanges, OnDestroy {
   }
 
   /** @nodoc */
-  ngOnChanges(changes: {}): any {
+  ngOnChanges(changes: SimpleChanges): any {
     this.updateTargetUrlAndHref();
+    this.onChanges.next(this);
   }
   /** @nodoc */
   ngOnDestroy(): any {

packages/router/src/directives/router_link_active.ts

@@ -7,7 +7,8 @@
  */
 
 import {AfterContentInit, ChangeDetectorRef, ContentChildren, Directive, ElementRef, Input, OnChanges, OnDestroy, Optional, QueryList, Renderer2, SimpleChanges} from '@angular/core';
-import {Subscription} from 'rxjs';
+import {from, of, Subscription} from 'rxjs';
+import {mergeAll} from 'rxjs/operators';
 
 import {Event, NavigationEnd} from '../events';
 import {Router} from '../router';
@@ -79,14 +80,13 @@ import {RouterLink, RouterLinkWithHref} from './router_link';
   exportAs: 'routerLinkActive',
 })
 export class RouterLinkActive implements OnChanges, OnDestroy, AfterContentInit {
-  // TODO(issue/24571): remove '!'.
   @ContentChildren(RouterLink, {descendants: true}) links!: QueryList<RouterLink>;
-  // TODO(issue/24571): remove '!'.
   @ContentChildren(RouterLinkWithHref, {descendants: true})
   linksWithHrefs!: QueryList<RouterLinkWithHref>;
 
   private classes: string[] = [];
-  private subscription: Subscription;
+  private routerEventsSubscription: Subscription;
+  private linkInputChangesSubscription?: Subscription;
   public readonly isActive: boolean = false;
 
   @Input() routerLinkActiveOptions: {exact: boolean} = {exact: false};
@@ -95,7 +95,7 @@ export class RouterLinkActive implements OnChanges, OnDestroy, AfterContentInit
       private router: Router, private element: ElementRef, private renderer: Renderer2,
       private readonly cdr: ChangeDetectorRef, @Optional() private link?: RouterLink,
       @Optional() private linkWithHref?: RouterLinkWithHref) {
-    this.subscription = router.events.subscribe((s: Event) => {
+    this.routerEventsSubscription = router.events.subscribe((s: Event) => {
       if (s instanceof NavigationEnd) {
         this.update();
       }
@@ -104,9 +104,26 @@ export class RouterLinkActive implements OnChanges, OnDestroy, AfterContentInit
 
   /** @nodoc */
   ngAfterContentInit(): void {
-    this.links.changes.subscribe(_ => this.update());
-    this.linksWithHrefs.changes.subscribe(_ => this.update());
-    this.update();
+    // `of(null)` is used to force subscribe body to execute once immediately (like `startWith`).
+    from([this.links.changes, this.linksWithHrefs.changes, of(null)])
+        .pipe(mergeAll())
+        .subscribe(_ => {
+          this.update();
+          this.subscribeToEachLinkOnChanges();
+        });
+  }
+
+  private subscribeToEachLinkOnChanges() {
+    this.linkInputChangesSubscription?.unsubscribe();
+    const allLinkChanges =
+        [...this.links.toArray(), ...this.linksWithHrefs.toArray(), this.link, this.linkWithHref]
+            .filter((link): link is RouterLink|RouterLinkWithHref => !!link)
+            .map(link => link.onChanges);
+    this.linkInputChangesSubscription = from(allLinkChanges).pipe(mergeAll()).subscribe(link => {
+      if (this.isActive !== this.isLinkActive(this.router)(link)) {
+        this.update();
+      }
+    });
   }
 
   @Input()
@@ -121,7 +138,8 @@ export class RouterLinkActive implements OnChanges, OnDestroy, AfterContentInit
   }
   /** @nodoc */
   ngOnDestroy(): void {
-    this.subscription.unsubscribe();
+    this.routerEventsSubscription.unsubscribe();
+    this.linkInputChangesSubscription?.unsubscribe();
   }
 
   private update(): void {

packages/router/test/integration.spec.ts

@@ -4344,7 +4344,7 @@ describe('Integration', () => {
        })));
   });
 
-  describe('routerActiveLink', () => {
+  describe('routerLinkActive', () => {
     it('should set the class when the link is active (a tag)',
        fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
          const fixture = createRoot(router, RootCmp);
@@ -4494,6 +4494,29 @@ describe('Integration', () => {
          advance(fixture);
          expect(paragraph.textContent).toEqual('false');
        }));
+
+    it('should not trigger change detection when active state has not changed', fakeAsync(() => {
+         @Component({
+           template: `<div id="link" routerLinkActive="active" [routerLink]="link"></div>`,
+         })
+         class LinkComponent {
+           link = 'notactive';
+         }
+
+         @Component({template: ''})
+         class SimpleComponent {
+         }
+
+         TestBed.configureTestingModule({
+           imports: [RouterTestingModule.withRoutes([{path: '', component: SimpleComponent}])],
+           declarations: [LinkComponent, SimpleComponent]
+         });
+
+         const fixture = createRoot(TestBed.inject(Router), LinkComponent);
+         fixture.componentInstance.link = 'stillnotactive';
+         fixture.detectChanges(false /** checkNoChanges */);
+         expect(TestBed.inject(NgZone).hasPendingMicrotasks).toBe(false);
+       }));
   });
 
   describe('lazy loading', () => {

packages/router/test/regression_integration.spec.ts

@@ -14,6 +14,54 @@ import {RouterTestingModule} from '@angular/router/testing';
 
 describe('Integration', () => {
   describe('routerLinkActive', () => {
+    it('should update when the associated routerLinks change - #18469', fakeAsync(() => {
+         @Component({
+           template: `
+          <a id="first-link" [routerLink]="[firstLink]" routerLinkActive="active">{{firstLink}}</a>
+          <div id="second-link" routerLinkActive="active">
+            <a [routerLink]="[secondLink]">{{secondLink}}</a>
+          </div>
+           `,
+         })
+         class LinkComponent {
+           firstLink = 'link-a';
+           secondLink = 'link-b';
+
+           changeLinks(): void {
+             const temp = this.secondLink;
+             this.secondLink = this.firstLink;
+             this.firstLink = temp;
+           }
+         }
+
+         @Component({template: 'simple'})
+         class SimpleCmp {
+         }
+
+         TestBed.configureTestingModule({
+           imports: [RouterTestingModule.withRoutes(
+               [{path: 'link-a', component: SimpleCmp}, {path: 'link-b', component: SimpleCmp}])],
+           declarations: [LinkComponent, SimpleCmp]
+         });
+
+         const router: Router = TestBed.inject(Router);
+         const fixture = createRoot(router, LinkComponent);
+         const firstLink = fixture.debugElement.query(p => p.nativeElement.id === 'first-link');
+         const secondLink = fixture.debugElement.query(p => p.nativeElement.id === 'second-link');
+         router.navigateByUrl('/link-a');
+         advance(fixture);
+
+         expect(firstLink.nativeElement.classList).toContain('active');
+         expect(secondLink.nativeElement.classList).not.toContain('active');
+
+         fixture.componentInstance.changeLinks();
+         fixture.detectChanges();
+         advance(fixture);
+
+         expect(firstLink.nativeElement.classList).not.toContain('active');
+         expect(secondLink.nativeElement.classList).toContain('active');
+       }));
+
     it('should not cause infinite loops in the change detection - #15825', fakeAsync(() => {
          @Component({selector: 'simple', template: 'simple'})
          class SimpleCmp {