fix(router): ensure routerLinkActive updates when associated routerLinks change #38349

atscott · 2020-08-05T19:08:50Z

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

atscott · 2020-08-05T19:10:50Z

presubmit

AndrewKushnir

LGTM (just a couple nits) 👍

Reviewed-for: public-api

packages/router/src/directives/router_link.ts

packages/router/src/directives/router_link_active.ts

packages/router/src/directives/router_link.ts

mary-poppins · 2020-08-05T22:48:15Z

You can preview 3abec29 at https://pr38349-3abec29.ngbuilds.io/.
You can preview 7479599 at https://pr38349-7479599.ngbuilds.io/.
You can preview 817591f at https://pr38349-817591f.ngbuilds.io/.

mary-poppins · 2020-08-05T22:59:46Z

You can preview de6aaec at https://pr38349-de6aaec.ngbuilds.io/.

mary-poppins · 2020-08-05T23:13:25Z

You can preview 87ea2f1 at https://pr38349-87ea2f1.ngbuilds.io/.

mary-poppins · 2020-08-06T00:29:15Z

You can preview d0f41ba at https://pr38349-d0f41ba.ngbuilds.io/.

mmalerba

LGTM

mary-poppins · 2020-08-06T23:10:34Z

You can preview ffcefd8 at https://pr38349-ffcefd8.ngbuilds.io/.

jelbourn

LGTM

Reviewed-for: public-api

…nks change 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 angular#18469

mary-poppins · 2020-08-17T15:20:14Z

You can preview 95af0b8 at https://pr38349-95af0b8.ngbuilds.io/.

alxhub

Reviewed-For: public-api

petebacondarwin

Reviewed-for: public-api

petebacondarwin · 2020-08-17T16:25:35Z

packages/router/src/directives/router_link_active.ts

  @ContentChildren(RouterLink, {descendants: true}) links!: QueryList<RouterLink>;
-  // TODO(issue/24571): remove '!'.


Generally I am against removing these comments unless the ! is removed (somehow) or replacing the comment with a reason why the ! can be retained.

But I guess Angular developers are aware that queries are only available after the appropriate hook has been called.

My thought process was that they could "technically" be undefined until afterContentInit but are reliably initialized after that. And that we could really never make this ? instead so the comment isn't really actionable in that way. I did some more reading in the issue report and found these guidelines that support keeping ! as well:

the field is reliably initialized, but TS cannot infer that (initialization can occur outside the ctor, for example), so ! should be kept.

#24571 (comment)
I think ContentChildren and ViewChildren could be considered "reliably initialized" after the correct lifecycle hooks.

If the field is annotated with @ViewChildren or @ContentChildren - Add back '!' - c).

#24571 (comment)

packages/router/src/directives/router_link.ts

…outerLinks change

mary-poppins · 2020-08-17T17:36:50Z

You can preview e21e492 at https://pr38349-e21e492.ngbuilds.io/.

…nks change (#38349) 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 #38349

…routerLinks change (angular#38349)" This reverts commit e0e5c9f. Failures in Google tests were detected.

…routerLinks change (#38349)" (#38511) This reverts commit e0e5c9f. Failures in Google tests were detected. PR Close #38511

…nks change (angular#38349) 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 angular#18469 PR Close angular#38349

…routerLinks change (angular#38349)" (angular#38511) This reverts commit e0e5c9f. Failures in Google tests were detected. PR Close angular#38511

…ociated routerLinks change (angular#38349)" (angular#38511)" This reverts commit 25ba8f4.

…routerLinks change (angular#38349)" This reverts commit e7f5e84.

…nks change (angular#38349) 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 angular#18469 PR Close angular#38349

…routerLinks change (angular#38349)" (angular#38511) This reverts commit e0e5c9f. Failures in Google tests were detected. PR Close angular#38511

angular-automatic-lock-bot · 2020-09-17T16:20:05Z

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

atscott added type: bug/fix area: router target: patch This PR is targeted for the next patch release router: directives RouterLink, RouterLinkActive, RouterOutlet etc. labels Aug 5, 2020

ngbot bot added this to the needsTriage milestone Aug 5, 2020

googlebot added the cla: yes label Aug 5, 2020

mmalerba self-requested a review August 5, 2020 19:12

atscott marked this pull request as ready for review August 5, 2020 19:14

atscott force-pushed the rla branch 3 times, most recently from 7479599 to 817591f Compare August 5, 2020 21:13

pullapprove bot requested review from alxhub, AndrewKushnir, jelbourn and pkozlowski-opensource August 5, 2020 21:14

AndrewKushnir approved these changes Aug 5, 2020

View reviewed changes

packages/router/src/directives/router_link.ts Show resolved Hide resolved

packages/router/src/directives/router_link_active.ts Show resolved Hide resolved

packages/router/src/directives/router_link.ts Show resolved Hide resolved

atscott added the aio: preview label Aug 5, 2020

atscott force-pushed the rla branch from de6aaec to 87ea2f1 Compare August 5, 2020 23:03

atscott force-pushed the rla branch from 87ea2f1 to d0f41ba Compare August 6, 2020 00:20

mmalerba approved these changes Aug 6, 2020

View reviewed changes

atscott force-pushed the rla branch 2 times, most recently from 0ae1d28 to ffcefd8 Compare August 6, 2020 23:01

atscott added the risk: low label Aug 9, 2020

jelbourn approved these changes Aug 10, 2020

View reviewed changes

atscott requested a review from petebacondarwin August 17, 2020 15:07

atscott force-pushed the rla branch from ffcefd8 to 95af0b8 Compare August 17, 2020 15:09

alxhub approved these changes Aug 17, 2020

View reviewed changes

petebacondarwin approved these changes Aug 17, 2020

View reviewed changes

fixup! fix(router): ensure routerLinkActive updates when associated r…

e21e492

…outerLinks change

atscott force-pushed the rla branch from 95af0b8 to e21e492 Compare August 17, 2020 17:26

atscott removed the request for review from pkozlowski-opensource August 17, 2020 19:32

atscott added the action: merge The PR is ready for merge by the caretaker label Aug 17, 2020

atscott closed this in e0e5c9f Aug 17, 2020

atscott added a commit to atscott/angular that referenced this pull request Aug 18, 2020

Revert "fix(router): ensure routerLinkActive updates when associated …

470e70f

…routerLinks change (angular#38349)" This reverts commit e0e5c9f. Failures in Google tests were detected.

atscott added a commit that referenced this pull request Aug 18, 2020

Revert "fix(router): ensure routerLinkActive updates when associated …

bee44b3

…routerLinks change (#38349)" (#38511) This reverts commit e0e5c9f. Failures in Google tests were detected. PR Close #38511

atscott added a commit that referenced this pull request Aug 18, 2020

Revert "fix(router): ensure routerLinkActive updates when associated …

f9a76a7

…routerLinks change (#38349)" (#38511) This reverts commit e0e5c9f. Failures in Google tests were detected. PR Close #38511

subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this pull request Aug 31, 2020

Revert "Revert "fix(router): ensure routerLinkActive updates when ass…

a3c907f

…ociated routerLinks change (angular#38349)" (angular#38511)" This reverts commit 25ba8f4.

subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this pull request Aug 31, 2020

Revert "fix(router): ensure routerLinkActive updates when associated …

acf692c

…routerLinks change (angular#38349)" This reverts commit e7f5e84.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 17, 2020

		@ContentChildren(RouterLink, {descendants: true}) links!: QueryList<RouterLink>;
		// TODO(issue/24571): remove '!'.

fix(router): ensure routerLinkActive updates when associated routerLinks change #38349

fix(router): ensure routerLinkActive updates when associated routerLinks change #38349

Uh oh!

Conversation

atscott commented Aug 5, 2020

Uh oh!

atscott commented Aug 5, 2020

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mary-poppins commented Aug 5, 2020

Uh oh!

mary-poppins commented Aug 5, 2020

Uh oh!

mary-poppins commented Aug 5, 2020

Uh oh!

mary-poppins commented Aug 6, 2020

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 6, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 17, 2020

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

atscott Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mary-poppins commented Aug 17, 2020

Uh oh!

angular-automatic-lock-bot bot commented Sep 17, 2020

Uh oh!

Uh oh!