-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(router): add prioritizedGuardValue operator optimization and allowing UrlTree return from guard #26478
feat(router): add prioritizedGuardValue operator optimization and allowing UrlTree return from guard #26478
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/** | ||
* @license | ||
* Copyright Google Inc. All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
import {Observable, OperatorFunction, combineLatest} from 'rxjs'; | ||
import {filter, scan, startWith, switchMap, take} from 'rxjs/operators'; | ||
|
||
import {UrlTree} from '../url_tree'; | ||
|
||
const INITIAL_VALUE = Symbol('INITIAL_VALUE'); | ||
declare type INTERIM_VALUES = typeof INITIAL_VALUE | boolean | UrlTree; | ||
|
||
export function prioritizedGuardValue(): | ||
OperatorFunction<Observable<boolean|UrlTree>[], boolean|UrlTree> { | ||
return switchMap(obs => { | ||
return combineLatest( | ||
...obs.map(o => o.pipe(take(1), startWith(INITIAL_VALUE as INTERIM_VALUES)))) | ||
.pipe( | ||
scan( | ||
(acc: INTERIM_VALUES, list: INTERIM_VALUES[]) => { | ||
let isPending = false; | ||
return list.reduce((innerAcc, val, i: number) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The reducer function inside of here could be pulled out into it's own static function (like above) |
||
if (innerAcc !== INITIAL_VALUE) return innerAcc; | ||
|
||
// Toggle pending flag if any values haven't been set yet | ||
if (val === INITIAL_VALUE) isPending = true; | ||
|
||
// Any other return values are only valid if we haven't yet hit a pending call. | ||
// This guarantees that in the case of a guard at the bottom of the tree that | ||
// returns a redirect, we will wait for the higher priority guard at the top to | ||
// finish before performing the redirect. | ||
if (!isPending) { | ||
// Early return when we hit a `false` value as that should always cancel | ||
// navigation | ||
if (val === false) return val; | ||
|
||
if (i === list.length - 1 || val instanceof UrlTree) { | ||
return val; | ||
} | ||
} | ||
|
||
return innerAcc; | ||
}, acc); | ||
}, | ||
INITIAL_VALUE), | ||
filter(item => item !== INITIAL_VALUE), take(1)) as Observable<boolean|UrlTree>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Adding a trailing comma will make clang format this cleaner. |
||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ ts_library( | |
"//packages/router/testing", | ||
"@rxjs", | ||
"@rxjs//operators", | ||
"@rxjs//testing", | ||
], | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,182 @@ | ||
/** | ||
* @license | ||
* Copyright Google Inc. All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
|
||
import {TestBed} from '@angular/core/testing'; | ||
import {Observable, Observer, of } from 'rxjs'; | ||
import {every, mergeMap} from 'rxjs/operators'; | ||
import {TestScheduler} from 'rxjs/testing'; | ||
|
||
import {prioritizedGuardValue} from '../../src/operators/prioritized_guard_value'; | ||
import {Router} from '../../src/router'; | ||
import {UrlTree} from '../../src/url_tree'; | ||
import {RouterTestingModule} from '../../testing/src/router_testing_module'; | ||
|
||
|
||
describe('prioritizedGuardValue operator', () => { | ||
let testScheduler: TestScheduler; | ||
let router: Router; | ||
const TF = {T: true, F: false}; | ||
|
||
beforeEach(() => { TestBed.configureTestingModule({imports: [RouterTestingModule]}); }); | ||
beforeEach(() => { testScheduler = new TestScheduler(assertDeepEquals); }); | ||
beforeEach(() => { router = TestBed.get(Router); }); | ||
|
||
it('should return true if all values are true', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ----------(T|)', TF); | ||
const c = cold(' ------(T|)', TF); | ||
const source = hot('---o--', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------T--'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, TF, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should return false if observables to the left of false have produced a value', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ----------(T|)', TF); | ||
const c = cold(' ------(F|)', TF); | ||
const source = hot('---o--', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------F--'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, TF, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should ignore results for unresolved sets of Observables', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' -------------(T|)', TF); | ||
const c = cold(' ------(F|)', TF); | ||
|
||
const z = cold(' ----(T|)', TF); | ||
|
||
const source = hot('---o----p----', {o: [a, b, c], p: [z]}); | ||
|
||
const expected = ' ------------T---'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, TF, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should return UrlTree if higher priority guards have resolved', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const urlTree = router.parseUrl('/'); | ||
|
||
const urlLookup = {U: urlTree}; | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ----------(U|)', urlLookup); | ||
const c = cold(' ------(T|)', TF); | ||
|
||
const source = hot('---o---', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------U---'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, urlLookup, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should return false even with UrlTree if UrlTree is lower priority', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const urlTree = router.parseUrl('/'); | ||
|
||
const urlLookup = {U: urlTree}; | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ----------(F|)', TF); | ||
const c = cold(' ------(U|)', urlLookup); | ||
|
||
const source = hot('---o---', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------F---'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, TF, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should return UrlTree even after a false if the false is lower priority', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const urlTree = router.parseUrl('/'); | ||
|
||
const urlLookup = {U: urlTree}; | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ----------(U|)', urlLookup); | ||
const c = cold(' ------(F|)', TF); | ||
|
||
const source = hot('---o---', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------U----'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, urlLookup, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should return the highest priority UrlTree', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const urlTreeU = router.parseUrl('/u'); | ||
const urlTreeR = router.parseUrl('/r'); | ||
const urlTreeL = router.parseUrl('/l'); | ||
|
||
const urlLookup = {U: urlTreeU, R: urlTreeR, L: urlTreeL}; | ||
|
||
const a = cold(' ----------(U|)', urlLookup); | ||
const b = cold(' -----(R|)', urlLookup); | ||
const c = cold(' --(L|)', urlLookup); | ||
|
||
const source = hot('---o---', {o: [a, b, c]}); | ||
|
||
const expected = ' -------------U---'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, urlLookup, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
it('should propagate errors', () => { | ||
testScheduler.run(({hot, cold, expectObservable}) => { | ||
|
||
const a = cold(' --(T|)', TF); | ||
const b = cold(' ------#', TF); | ||
const c = cold(' ----------(F|)', TF); | ||
const source = hot('---o------', {o: [a, b, c]}); | ||
|
||
const expected = ' ---------#'; | ||
|
||
expectObservable(source.pipe(prioritizedGuardValue())) | ||
.toBe(expected, TF, /* an error here maybe */); | ||
}); | ||
}); | ||
|
||
|
||
}); | ||
|
||
|
||
|
||
function assertDeepEquals(a: any, b: any) { | ||
return expect(a).toEqual(b); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
o => o.pipe(take(1), startWith(INITIAL_VALUE as INTERIM_VALUES)))
could be pulled out into it's own static function for better performance and lower memory pressure. (Also might help readability)