fix(router): fragment can be null by MatthiasKunnen · Pull Request #37336 · angular/angular

MatthiasKunnen · 2020-05-28T20:40:08Z

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?

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?

ActivatedRoute.fragment is typed as Observable<string> but can emit both null and undefined due to incorrect non-null assertions.

Issue Number: #23894, #34197.

What is the new behavior?

The non-null assertions have been removed and fragment has been retyped to string | null. undefined fragments will no longer occur.

Does this PR introduce a breaking change?

Yes
No

BREAKING CHANGE:
Strict null checks will report on fragment potentially being null.
Migration path: add null check.

Other information

Fixes #23894, fixes #34197, fixes #29391.
Supersedes #37334, #29819.

MatthiasKunnen · 2020-05-28T20:44:44Z

@atscott, this is the superseding PR.

MatthiasKunnen · 2020-05-29T12:09:19Z

I just updated the golden file, forgot that yesterday.

atscott

Thanks for fixing this and putting the breaking change footer in! LGTM with some small change requests.

As this is a breaking change, we'll need to wait until v11

packages/router/src/create_url_tree.ts

packages/router/src/url_tree.ts

MatthiasKunnen · 2020-05-29T15:41:55Z

Made changes as requested

atscott · 2020-05-29T15:48:04Z

Ah, looks like your second commit doesn't have a commit message body that satisfies the 100 character requirement. Maybe squash your commits into one and just use the first?

atscott · 2020-05-29T15:48:36Z

Adding blocked flag, as we'll need a migration plan and have to wait for v11.

jelbourn

LGTM

Reviewed-for: public-api

MatthiasKunnen · 2020-11-20T16:38:40Z

Is there a reason this was not part of the Angular 11 release?

MatthiasKunnen · 2021-01-16T10:17:32Z

Resolved conflicts

crisbeto · 2021-03-05T16:17:01Z

Migration PR: #41092.

Adds a migration that casts the value of `ActivatedRouteSnapshot.fragment` to be non-nullable. Also moves some code from the `AbstractControl.parent` migration so that it can be reused. Relates to angular#37336.

atscott · 2021-03-15T19:32:53Z

~~global presubmit~~ (not triggered, will need to run manually after hours)

atscott · 2021-03-19T20:55:51Z

global presubmit

atscott · 2021-03-19T20:58:02Z

caretaker note: please merge and sync this one on its own. g3 cleanup has been performed but things may have changed since then.

atscott

reviewed-for: public-api

MatthiasKunnen · 2021-03-20T15:28:46Z

If you need me to do anything; rebase, ... let me know

Adds a migration that casts the value of `ActivatedRouteSnapshot.fragment` to be non-nullable. Also moves some code from the `AbstractControl.parent` migration so that it can be reused. Relates to #37336. PR Close #41092

ActivatedRoute.fragment was typed as Observable<string> but could emit both null and undefined due to incorrect non-null assertion. These non-null assertions have been removed and fragment has been retyped to string | null. BREAKING CHANGE: Strict null checks will report on fragment potentially being null. Migration path: add null check. Fixes angular#23894, fixes angular#34197. PR Close angular#37336

…lar#41092) Adds a migration that casts the value of `ActivatedRouteSnapshot.fragment` to be non-nullable. Also moves some code from the `AbstractControl.parent` migration so that it can be reused. Relates to angular#37336. PR Close angular#41092

angular-automatic-lock-bot · 2021-04-22T16:09:41Z

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

googlebot added the cla: yes label May 28, 2020

MatthiasKunnen force-pushed the issue-23894-fragment-nullable branch from 2c7b47f to deb0a53 Compare May 28, 2020 20:43

pullapprove bot requested a review from atscott May 29, 2020 01:36

MatthiasKunnen force-pushed the issue-23894-fragment-nullable branch from deb0a53 to 69c309d Compare May 29, 2020 12:08

pullapprove bot requested a review from IgorMinar May 29, 2020 12:08

atscott added area: router target: major This PR is targeted for the next major release labels May 29, 2020

ngbot bot added this to the needsTriage milestone May 29, 2020

atscott requested changes May 29, 2020

View reviewed changes

packages/router/src/create_url_tree.ts Outdated Show resolved Hide resolved

packages/router/src/url_tree.ts Outdated Show resolved Hide resolved

atscott added the breaking changes label May 29, 2020

atscott approved these changes May 29, 2020

View reviewed changes

atscott added the state: blocked label May 29, 2020

MatthiasKunnen force-pushed the issue-23894-fragment-nullable branch from 532e76c to 799ba4a Compare May 29, 2020 16:00

atscott modified the milestones: needsTriage, v11-candidates May 29, 2020

pullapprove bot requested review from alxhub, jelbourn and pkozlowski-opensource June 29, 2020 18:15

atscott mentioned this pull request Jun 29, 2020

fix(router): Set route snapshot fragment default value as null #37803

Closed

14 tasks

jelbourn approved these changes Jun 29, 2020

View reviewed changes

MatthiasKunnen mentioned this pull request Jul 6, 2020

fix(router): change undefined route snapshot fragment to null #29819

Closed

14 tasks

atscott modified the milestones: v11-candidates, v12-candidates Nov 20, 2020

atscott added the cross-cutting: types label Nov 20, 2020

crisbeto mentioned this pull request Mar 5, 2021

feat(router): add migration for ActivatedRouteSnapshot.fragment #41092

Closed

atscott removed request for IgorMinar, alxhub and pkozlowski-opensource March 19, 2021 20:58

atscott approved these changes Mar 19, 2021

View reviewed changes

mhevery closed this in b555160 Mar 22, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 22, 2021

Conversation

MatthiasKunnen commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

MatthiasKunnen commented May 28, 2020

Uh oh!

MatthiasKunnen commented May 29, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MatthiasKunnen commented May 29, 2020

Uh oh!

atscott commented May 29, 2020

Uh oh!

atscott commented May 29, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

MatthiasKunnen commented Nov 20, 2020

Uh oh!

MatthiasKunnen commented Jan 16, 2021

Uh oh!

crisbeto commented Mar 5, 2021

Uh oh!

atscott commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Mar 19, 2021

Uh oh!

atscott commented Mar 19, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

MatthiasKunnen commented Mar 20, 2021

Uh oh!

angular-automatic-lock-bot bot commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MatthiasKunnen commented May 28, 2020 •

edited

Loading

atscott commented Mar 15, 2021 •

edited

Loading