Skip to content

Commit b555160

Browse files
MatthiasKunnenmhevery
MatthiasKunnen
authored and
mhevery
committed
fix(router): fragment can be null (#37336)
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 #23894, fixes #34197. PR Close #37336
1 parent e7b1d43 commit b555160

File tree

10 files changed

+46
-18
lines changed

10 files changed

+46
-18
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ export declare class ActivatedRoute {
33
component: Type<any> | string | null;
44
data: Observable<Data>;
55
get firstChild(): ActivatedRoute | null;
6-
fragment: Observable<string>;
6+
fragment: Observable<string | null>;
77
outlet: string;
88
get paramMap(): Observable<ParamMap>;
99
params: Observable<Params>;
@@ -23,7 +23,7 @@ export declare class ActivatedRouteSnapshot {
2323
component: Type<any> | string | null;
2424
data: Data;
2525
get firstChild(): ActivatedRouteSnapshot | null;
26-
fragment: string;
26+
fragment: string | null;
2727
outlet: string;
2828
get paramMap(): ParamMap;
2929
params: Params;

‎packages/router/src/apply_redirects.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ class ApplyRedirects {
9191
this.expandSegmentGroup(this.ngModule, this.config, rootSegmentGroup, PRIMARY_OUTLET);
9292
const urlTrees$ = expanded$.pipe(map((rootSegmentGroup: UrlSegmentGroup) => {
9393
return this.createUrlTree(
94-
squashSegmentGroup(rootSegmentGroup), this.urlTree.queryParams, this.urlTree.fragment!);
94+
squashSegmentGroup(rootSegmentGroup), this.urlTree.queryParams, this.urlTree.fragment);
9595
}));
9696
return urlTrees$.pipe(catchError((e: any) => {
9797
if (e instanceof AbsoluteRedirect) {
@@ -114,7 +114,7 @@ class ApplyRedirects {
114114
this.expandSegmentGroup(this.ngModule, this.config, tree.root, PRIMARY_OUTLET);
115115
const mapped$ = expanded$.pipe(map((rootSegmentGroup: UrlSegmentGroup) => {
116116
return this.createUrlTree(
117-
squashSegmentGroup(rootSegmentGroup), tree.queryParams, tree.fragment!);
117+
squashSegmentGroup(rootSegmentGroup), tree.queryParams, tree.fragment);
118118
}));
119119
return mapped$.pipe(catchError((e: any): Observable<UrlTree> => {
120120
if (e instanceof NoMatch) {
@@ -129,7 +129,7 @@ class ApplyRedirects {
129129
return new Error(`Cannot match any routes. URL Segment: '${e.segmentGroup}'`);
130130
}
131131

132-
private createUrlTree(rootCandidate: UrlSegmentGroup, queryParams: Params, fragment: string):
132+
private createUrlTree(rootCandidate: UrlSegmentGroup, queryParams: Params, fragment: string|null):
133133
UrlTree {
134134
const root = rootCandidate.segments.length > 0 ?
135135
new UrlSegmentGroup([], {[PRIMARY_OUTLET]: rootCandidate}) :

‎packages/router/src/create_url_tree.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ import {UrlSegment, UrlSegmentGroup, UrlTree} from './url_tree';
1212
import {forEach, last, shallowEqual} from './utils/collection';
1313

1414
export function createUrlTree(
15-
route: ActivatedRoute, urlTree: UrlTree, commands: any[], queryParams: Params,
16-
fragment: string): UrlTree {
15+
route: ActivatedRoute, urlTree: UrlTree, commands: any[], queryParams: Params|null,
16+
fragment: string|null): UrlTree {
1717
if (commands.length === 0) {
1818
return tree(urlTree.root, urlTree.root, urlTree, queryParams, fragment);
1919
}
@@ -47,7 +47,7 @@ function isCommandWithOutlets(command: any): command is {outlets: {[key: string]
4747

4848
function tree(
4949
oldSegmentGroup: UrlSegmentGroup, newSegmentGroup: UrlSegmentGroup, urlTree: UrlTree,
50-
queryParams: Params, fragment: string): UrlTree {
50+
queryParams: Params|null, fragment: string|null): UrlTree {
5151
let qp: any = {};
5252
if (queryParams) {
5353
forEach(queryParams, (value: any, name: any) => {

‎packages/router/src/recognize.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export class Recognizer {
6767
// Use Object.freeze to prevent readers of the Router state from modifying it outside of a
6868
// navigation, resulting in the router being out of sync with the browser.
6969
const root = new ActivatedRouteSnapshot(
70-
[], Object.freeze({}), Object.freeze({...this.urlTree.queryParams}), this.urlTree.fragment!,
70+
[], Object.freeze({}), Object.freeze({...this.urlTree.queryParams}), this.urlTree.fragment,
7171
{}, PRIMARY_OUTLET, this.rootComponentType, null, this.urlTree.root, -1, {});
7272

7373
const rootNode = new TreeNode<ActivatedRouteSnapshot>(root, children);
@@ -160,7 +160,7 @@ export class Recognizer {
160160
if (route.path === '**') {
161161
const params = segments.length > 0 ? last(segments)!.parameters : {};
162162
snapshot = new ActivatedRouteSnapshot(
163-
segments, params, Object.freeze({...this.urlTree.queryParams}), this.urlTree.fragment!,
163+
segments, params, Object.freeze({...this.urlTree.queryParams}), this.urlTree.fragment,
164164
getData(route), getOutlet(route), route.component!, route,
165165
getSourceSegmentGroup(rawSegment), getPathIndexShift(rawSegment) + segments.length,
166166
getResolve(route));
@@ -174,7 +174,7 @@ export class Recognizer {
174174

175175
snapshot = new ActivatedRouteSnapshot(
176176
consumedSegments, result.parameters, Object.freeze({...this.urlTree.queryParams}),
177-
this.urlTree.fragment!, getData(route), getOutlet(route), route.component!, route,
177+
this.urlTree.fragment, getData(route), getOutlet(route), route.component!, route,
178178
getSourceSegmentGroup(rawSegment),
179179
getPathIndexShift(rawSegment) + consumedSegments.length, getResolve(route));
180180
}

‎packages/router/src/router.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1142,7 +1142,7 @@ export class Router {
11421142
if (q !== null) {
11431143
q = this.removeEmptyProps(q);
11441144
}
1145-
return createUrlTree(a, this.currentUrlTree, commands, q!, f!);
1145+
return createUrlTree(a, this.currentUrlTree, commands, q, f ?? null);
11461146
}
11471147

11481148
/**

‎packages/router/src/router_state.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ export class ActivatedRoute {
126126
/** An observable of the query parameters shared by all the routes. */
127127
public queryParams: Observable<Params>,
128128
/** An observable of the URL fragment shared by all the routes. */
129-
public fragment: Observable<string>,
129+
public fragment: Observable<string|null>,
130130
/** An observable of the static and resolved data of this route. */
131131
public data: Observable<Data>,
132132
/** The outlet name of the route, a constant. */
@@ -321,7 +321,7 @@ export class ActivatedRouteSnapshot {
321321
/** The query parameters shared by all the routes */
322322
public queryParams: Params,
323323
/** The URL fragment shared by all the routes */
324-
public fragment: string,
324+
public fragment: string|null,
325325
/** The static and resolved data of this route */
326326
public data: Data,
327327
/** The outlet name of the route */

‎packages/router/src/url_tree.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ export class DefaultUrlSerializer implements UrlSerializer {
382382
const segment = `/${serializeSegment(tree.root, true)}`;
383383
const query = serializeQueryParams(tree.queryParams);
384384
const fragment =
385-
typeof tree.fragment === `string` ? `#${encodeUriFragment(tree.fragment!)}` : '';
385+
typeof tree.fragment === `string` ? `#${encodeUriFragment(tree.fragment)}` : '';
386386

387387
return `${segment}${query}${fragment}`;
388388
}

‎packages/router/test/create_url_tree.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ function createRoot(tree: UrlTree, commands: any[], queryParams?: Params, fragme
406406
new BehaviorSubject(null!), new BehaviorSubject(null!), new BehaviorSubject(null!),
407407
new BehaviorSubject(null!), new BehaviorSubject(null!), PRIMARY_OUTLET, 'someComponent', s);
408408
advanceActivatedRoute(a);
409-
return createUrlTree(a, tree, commands, queryParams!, fragment!);
409+
return createUrlTree(a, tree, commands, queryParams ?? null, fragment ?? null);
410410
}
411411

412412
function create(
@@ -422,5 +422,5 @@ function create(
422422
new BehaviorSubject(null!), new BehaviorSubject(null!), new BehaviorSubject(null!),
423423
new BehaviorSubject(null!), new BehaviorSubject(null!), PRIMARY_OUTLET, 'someComponent', s);
424424
advanceActivatedRoute(a);
425-
return createUrlTree(a, tree, commands, queryParams!, fragment!);
425+
return createUrlTree(a, tree, commands, queryParams ?? null, fragment ?? null);
426426
}

‎packages/router/test/integration.spec.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1273,6 +1273,20 @@ describe('Integration', () => {
12731273
expect(fixture.nativeElement).toHaveText('query: 2 fragment: fragment2');
12741274
})));
12751275

1276+
it('should handle empty or missing fragments', fakeAsync(inject([Router], (router: Router) => {
1277+
const fixture = createRoot(router, RootCmp);
1278+
1279+
router.resetConfig([{path: 'query', component: QueryParamsAndFragmentCmp}]);
1280+
1281+
router.navigateByUrl('/query#');
1282+
advance(fixture);
1283+
expect(fixture.nativeElement).toHaveText('query: fragment: ');
1284+
1285+
router.navigateByUrl('/query');
1286+
advance(fixture);
1287+
expect(fixture.nativeElement).toHaveText('query: fragment: null');
1288+
})));
1289+
12761290
it('should ignore null and undefined query params',
12771291
fakeAsync(inject([Router], (router: Router) => {
12781292
const fixture = createRoot(router, RootCmp);
@@ -6058,7 +6072,15 @@ class QueryParamsAndFragmentCmp {
60586072

60596073
constructor(route: ActivatedRoute) {
60606074
this.name = route.queryParamMap.pipe(map((p: ParamMap) => p.get('name')));
6061-
this.fragment = route.fragment;
6075+
this.fragment = route.fragment.pipe(map((p: string|null|undefined) => {
6076+
if (p === undefined) {
6077+
return 'undefined';
6078+
} else if (p === null) {
6079+
return 'null';
6080+
} else {
6081+
return p;
6082+
}
6083+
}));
60626084
}
60636085
}
60646086

‎packages/router/test/url_serializer.spec.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,12 @@ describe('url serializer', () => {
186186
expect(url.serialize(tree)).toEqual('/one#');
187187
});
188188

189+
it('should parse no fragment', () => {
190+
const tree = url.parse('/one');
191+
expect(tree.fragment).toEqual(null);
192+
expect(url.serialize(tree)).toEqual('/one');
193+
});
194+
189195
describe('encoding/decoding', () => {
190196
it('should encode/decode path segments and parameters', () => {
191197
const u = `/${encodeUriSegment('one two')};${encodeUriSegment('p 1')}=${

0 commit comments

Comments
 (0)