fix(router): routerLinkActive should not throw when not initialized #13273

DzmitryShylovich · 2016-12-06T14:20:34Z

Also quickly describe what this PR fixes

if we have a template with only interpolation it works fine:

<div routerLinkActive="active" #rlaDashboard="routerLinkActive"> 
    <a routerLink="/dashboard">
      Dashboard isActive: {{rlaDashboard.isActive}}
    </a>
  </div>

but if we have bindings it fails:

<div routerLinkActive="active" #rlaTest="routerLinkActive"> 
    <a routerLink="/test">
    </a>
    <span *ngIf="rlaTest.isActive">DOES NOT WORK </span>
     or
   <span [ngClass]="{'highlight':rlaTest.isActive}">DOES NOT WORK </span>
  </div>

Getter is called before ContentChildren is initialized thus it failed here with the error Cannot read property some of undefined

vicb · 2016-12-06T18:54:04Z

modules/@angular/router/test/integration.spec.ts

@@ -1959,6 +1959,7 @@ describe('Integration', () => {
         @Component({
           template: `<a routerLink="/team" routerLinkActive #rla="routerLinkActive"></a>
              <p>{{rla.isActive}}</p>
+              <span *ngIf="rla.isActive"></span>


missing a .not.toThrow() somewhere ?

do u think it makes sense to assert that component creation doesn't throw an error?

expect(() => f = TestBed.createComponent(ComponentWithRouterLink)).not.toThrow();

do u think it makes sense to assert that component creation doesn't throw an error?

If it used to throw yes, otherwise no

vicb · 2016-12-06T18:58:56Z

modules/@angular/router/src/directives/router_link_active.ts

@@ -89,46 +89,50 @@ export class RouterLinkActive implements OnChanges,

  private classes: string[] = [];
  private subscription: Subscription;
+  private _isActive: boolean = false;


isActive for consistency

isActive without underscore is a public prop https://github.com/angular/angular/blob/master/modules/%40angular/router/src/directives/router_link_active.ts#L103

make all other private prop start with _ then please

I can rename it to something without underscore.
state, active?

whatever sounds best for you and you feel describes it best

vicb · 2016-12-06T19:07:14Z

modules/@angular/router/src/directives/router_link_active.ts


  ngAfterContentInit(): void {
-    this.links.changes.subscribe(s => this.update());
-    this.linksWithHrefs.changes.subscribe(s => this.update());
+    this.links.changes.subscribe(() => this.update());


vicb · 2016-12-06T19:08:34Z

Please split the changes into 2 commits:

a refactor,
a fix

Also quickly describe what this PR fixes, add a unit test.

Thanks

vicb · 2016-12-07T00:05:49Z

Please re-arrange in 1 refactor + 1 fix before it can be reviewed & merged

vicb · 2016-12-07T23:28:41Z

modules/@angular/router/src/directives/router_link_active.ts

-    } else {
-      this.classes = data.split(' ');
-    }
+    this.classes = (Array.isArray(data) ? data : data.split(' ')).filter(c => !!c);


const classes = Array.isArray(data) ? data : data.split(' '); this classes = classes.filter(c=> c);

vicb · 2016-12-07T23:29:13Z

modules/@angular/router/src/directives/router_link_active.ts

@@ -6,7 +6,7 @@
 * found in the LICENSE file at https://angular.io/license
 */

-import {AfterContentInit, ContentChildren, Directive, ElementRef, Input, OnChanges, OnDestroy, QueryList, Renderer} from '@angular/core';
+import {AfterContentInit, ChangeDetectorRef, ContentChildren, Directive, ElementRef, Input, OnChanges, OnDestroy, QueryList, Renderer} from '@angular/core';


move to 2nd commit (.d.ts as well)

vicb · 2016-12-07T23:33:41Z

modules/@angular/router/src/directives/router_link_active.ts

@@ -118,13 +121,15 @@ export class RouterLinkActive implements OnChanges,

  private update(): void {
    if (!this.links || !this.linksWithHrefs || !this.router.navigated) return;
-
    const isActive = this.hasActiveLink();


const hasActiveLinks() ? (also add "s" on the method name)

vicb · 2016-12-07T23:41:15Z

modules/@angular/router/src/directives/router_link_active.ts

+      this.active = isActive;
+      this.classes.forEach(
+        c => this.renderer.setElementClass(this.element.nativeElement, c, isActive));
+      this.cdr.detectChanges();


why do you actually detectChanges here ?

Otherwise it throws Expression ___ has changed after it was checked. We change get isActive() inside ngOnChanges which is forbidden without the second cycle of change detection. And I tried almost everything to avoid it but without any success.

One of the reasons why a separate directive was a bad idea.

Fixes angular#13270

DzmitryShylovich · 2017-01-06T07:47:48Z

@vicb ready for review

jelbourn · 2017-01-12T23:01:36Z

@vicb can you re-review? This fix is important for material.

mhevery · 2017-01-18T00:22:16Z

@vicb:

Fix: This fails http://test/ui#id=OCL:144769341:BASE:144769352:1484698287624:11baf20
Rebase

…13273) Fixes #13270 PR Close #13273

PR Close #13273

…13273) Fixes #13270 PR Close #13273

PR Close angular#13273

…ngular#13273) Fixes angular#13270 PR Close angular#13273

PR Close angular#13273

…ngular#13273) Fixes angular#13270 PR Close angular#13273

angular-automatic-lock-bot · 2019-09-10T15:21:38Z

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

PR Close angular#13273

…ngular#13273) Fixes angular#13270 PR Close angular#13273

googlebot added the cla: yes label Dec 6, 2016

DzmitryShylovich force-pushed the gh/13270 branch from 2c171ff to a925091 Compare December 6, 2016 14:47

DzmitryShylovich changed the title ~~fix(router): routerLinkActive active should not throw when not initia…~~ fix(router): routerLinkActiveshould not throw when not initialized Dec 6, 2016

DzmitryShylovich force-pushed the gh/13270 branch 3 times, most recently from 25715c6 to 6966594 Compare December 6, 2016 16:08

DzmitryShylovich changed the title ~~fix(router): routerLinkActiveshould not throw when not initialized~~ fix(router): routerLinkActive should not throw when not initialized Dec 6, 2016

DzmitryShylovich force-pushed the gh/13270 branch from 948d87d to 8a4561b Compare December 6, 2016 17:28

vicb added area: router action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 6, 2016

vicb reviewed Dec 6, 2016

View reviewed changes

vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 6, 2016

DzmitryShylovich force-pushed the gh/13270 branch from 9942261 to 1ef95ce Compare December 7, 2016 00:22

vicb reviewed Dec 7, 2016

View reviewed changes

DzmitryShylovich force-pushed the gh/13270 branch 2 times, most recently from cbb8969 to 0fb7db8 Compare December 8, 2016 12:06

DzmitryShylovich closed this Dec 10, 2016

DzmitryShylovich reopened this Jan 5, 2017

refactor(router): clean up RouterLinkActive

9751548

DzmitryShylovich force-pushed the gh/13270 branch from 8c6a616 to 519eea3 Compare January 6, 2017 07:35

fix(router): routerLinkActive should not throw when not initialized

fb5bd34

Fixes angular#13270

DzmitryShylovich force-pushed the gh/13270 branch from 519eea3 to fb5bd34 Compare January 6, 2017 07:40

vicb self-assigned this Jan 6, 2017

This was referenced Jan 7, 2017

[Tabs] md-tab-link: routerLinkActive.isActive error angular/components#1967

Closed

docs(tabs): add note for core bug that breaks md-tab-nav-bar angular/components#2563

Closed

jelbourn added the hotlist: components team Related to Angular CDK or Angular Material label Jan 12, 2017

vicb mentioned this pull request Jan 18, 2017

rebases #13273 #13972

Closed

mhevery closed this in 1a92e3d Jan 18, 2017

mhevery pushed a commit that referenced this pull request Jan 18, 2017

fix(router): routerLinkActive should not throw when not initialized (#…

e8ea741

…13273) Fixes #13270 PR Close #13273

DzmitryShylovich deleted the gh/13270 branch January 18, 2017 07:10

mhevery pushed a commit that referenced this pull request Jan 19, 2017

refactor(router): clean up RouterLinkActive (#13273)

b8b6b1d

PR Close #13273

mhevery pushed a commit that referenced this pull request Jan 19, 2017

fix(router): routerLinkActive should not throw when not initialized (#…

49c4b0f

…13273) Fixes #13270 PR Close #13273

wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017

refactor(router): clean up RouterLinkActive (angular#13273)

333da93

PR Close angular#13273

wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017

fix(router): routerLinkActive should not throw when not initialized (a…

4247ed7

…ngular#13273) Fixes angular#13270 PR Close angular#13273

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

refactor(router): clean up RouterLinkActive (angular#13273)

a09cbbf

PR Close angular#13273

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

fix(router): routerLinkActive should not throw when not initialized (a…

75b1c2d

…ngular#13273) Fixes angular#13270 PR Close angular#13273

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

idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021

refactor(router): clean up RouterLinkActive (angular#13273)

986da30

PR Close angular#13273

idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021

fix(router): routerLinkActive should not throw when not initialized (a…

4149b84

…ngular#13273) Fixes angular#13270 PR Close angular#13273

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): routerLinkActive should not throw when not initialized #13273

fix(router): routerLinkActive should not throw when not initialized #13273

DzmitryShylovich commented Dec 6, 2016 •

edited

Loading

vicb Dec 6, 2016

DzmitryShylovich Dec 6, 2016

vicb Dec 6, 2016

vicb Dec 6, 2016

DzmitryShylovich Dec 6, 2016

vicb Dec 6, 2016

DzmitryShylovich Dec 6, 2016

vicb Dec 7, 2016

vicb Dec 6, 2016

vicb commented Dec 6, 2016

vicb commented Dec 7, 2016

vicb Dec 7, 2016

vicb Dec 7, 2016

vicb Dec 7, 2016

vicb Dec 7, 2016

DzmitryShylovich Dec 7, 2016 •

edited

Loading

DzmitryShylovich commented Jan 6, 2017 •

edited

Loading

jelbourn commented Jan 12, 2017

mhevery commented Jan 18, 2017

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

fix(router): routerLinkActive should not throw when not initialized #13273

fix(router): routerLinkActive should not throw when not initialized #13273

Conversation

DzmitryShylovich commented Dec 6, 2016 • edited Loading

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

vicb commented Dec 6, 2016

vicb commented Dec 7, 2016

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

DzmitryShylovich Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

DzmitryShylovich commented Jan 6, 2017 • edited Loading

jelbourn commented Jan 12, 2017

mhevery commented Jan 18, 2017

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

DzmitryShylovich commented Dec 6, 2016 •

edited

Loading

DzmitryShylovich Dec 7, 2016 •

edited

Loading

DzmitryShylovich commented Jan 6, 2017 •

edited

Loading