Skip to content

Commit bbb46e5

Browse files
atscottmmalerba
authored andcommitted
refactor(router): Add firstValueFrom helper (angular#63485)
This fixes a bug introduced in angular#62994 where `toPromise` was used. This doesn't work in all situations here because some observables don't complete. This only affected the redirect path, since the others are already behind other rxjs code which takes the first value from the user guards. Note: This is marked as a refactor rather than fix because the commit above is not in any release yet. PR Close angular#63485
1 parent d1edb71 commit bbb46e5

File tree

5 files changed

+59
-22
lines changed

5 files changed

+59
-22
lines changed

packages/core/test/bundling/router/bundle.golden_symbols.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,7 @@
621621
"fireActivationStart",
622622
"fireChildActivationStart",
623623
"first",
624+
"firstValueFrom",
624625
"flatten",
625626
"flattenRouteTree",
626627
"forEachSingleProvider",

packages/router/src/apply_redirects.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {ActivatedRouteSnapshot} from './router_state';
1616
import {Params, PRIMARY_OUTLET} from './shared';
1717
import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree';
1818
import {wrapIntoObservable} from './utils/collection';
19+
import {firstValueFrom} from './utils/first_value_from';
1920

2021
export const NO_MATCH_ERROR_NAME = 'ɵNoMatch';
2122
export class NoMatch extends Error {
@@ -195,9 +196,11 @@ function getRedirectResult(
195196
}
196197
const redirectToFn = redirectTo;
197198
const {queryParams, fragment, routeConfig, url, outlet, params, data, title} = currentSnapshot;
198-
return wrapIntoObservable(
199-
runInInjectionContext(injector, () =>
200-
redirectToFn({params, data, queryParams, fragment, routeConfig, url, outlet, title}),
199+
return firstValueFrom(
200+
wrapIntoObservable(
201+
runInInjectionContext(injector, () =>
202+
redirectToFn({params, data, queryParams, fragment, routeConfig, url, outlet, title}),
203+
),
201204
),
202-
).toPromise() as Promise<string | UrlTree>;
205+
);
203206
}

packages/router/src/recognize.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import {
3838
split,
3939
} from './utils/config_matching';
4040
import {TreeNode} from './utils/tree';
41+
import {firstValueFrom} from './utils/first_value_from';
4142

4243
/**
4344
* Class used to indicate there were no additional route config matches but that all segments of
@@ -400,14 +401,9 @@ export class Recognizer {
400401
if (this.abortSignal.aborted) {
401402
throw new Error(this.abortSignal.reason);
402403
}
403-
const result = await matchWithChecks(
404-
rawSegment,
405-
route,
406-
segments,
407-
injector,
408-
this.urlSerializer,
409-
this.abortSignal,
410-
).toPromise();
404+
const result = await firstValueFrom(
405+
matchWithChecks(rawSegment, route, segments, injector, this.urlSerializer, this.abortSignal),
406+
);
411407
if (route.path === '**') {
412408
// Prior versions of the route matching algorithm would stop matching at the wildcard route.
413409
// We should investigate a better strategy for any existing children. Otherwise, these
@@ -500,15 +496,11 @@ export class Recognizer {
500496
if (this.abortSignal.aborted) {
501497
throw new Error(this.abortSignal.reason);
502498
}
503-
const shouldLoadResult = await runCanLoadGuards(
504-
injector,
505-
route,
506-
segments,
507-
this.urlSerializer,
508-
this.abortSignal,
509-
).toPromise();
499+
const shouldLoadResult = await firstValueFrom(
500+
runCanLoadGuards(injector, route, segments, this.urlSerializer, this.abortSignal),
501+
);
510502
if (shouldLoadResult) {
511-
const cfg = await this.configLoader.loadChildren(injector, route).toPromise();
503+
const cfg = await firstValueFrom(this.configLoader.loadChildren(injector, route));
512504
if (!cfg) {
513505
throw canLoadFails(route);
514506
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import {Observable} from 'rxjs';
10+
import {first} from 'rxjs/operators';
11+
12+
/** replacement for firstValueFrom in rxjs 7. We must support rxjs v6 so we cannot use it */
13+
export function firstValueFrom<T>(source: Observable<T>): Promise<T> {
14+
return new Promise<T>((resolve, reject) => {
15+
source.pipe(first()).subscribe({
16+
next: (value) => resolve(value),
17+
error: (err) => reject(err),
18+
});
19+
});
20+
}

packages/router/test/apply_redirects.spec.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99
import {EnvironmentInjector, inject, Injectable, Type} from '@angular/core';
1010
import {TestBed} from '@angular/core/testing';
11-
import {firstValueFrom, Observable, of} from 'rxjs';
12-
import {switchMap, tap} from 'rxjs/operators';
11+
import {firstValueFrom, interval, Observable, of} from 'rxjs';
12+
import {map, switchMap, tap} from 'rxjs/operators';
1313

1414
import {Route, Routes} from '../src/models';
1515
import {recognize} from '../src/recognize';
@@ -1819,6 +1819,27 @@ describe('redirects', () => {
18191819
},
18201820
);
18211821
});
1822+
1823+
it('works when the returned redirect observable does not complete', async () => {
1824+
await checkRedirect(
1825+
[
1826+
{
1827+
path: 'a',
1828+
children: [
1829+
{
1830+
path: 'b',
1831+
redirectTo: () => interval(100).pipe(map(() => '/redirected')),
1832+
},
1833+
],
1834+
},
1835+
{path: '**', component: ComponentC},
1836+
],
1837+
'/a;k1=v1;k2=v2/b;k3=v3;k4=v4',
1838+
(t: UrlTree) => {
1839+
expectTreeToBe(t, 'redirected');
1840+
},
1841+
);
1842+
});
18221843
});
18231844

18241845
// internal failure b/165719418

0 commit comments

Comments
 (0)