From ad26cd6d0cae093beb4d936609e9d805b3528505 Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Thu, 6 Dec 2018 15:09:12 -0800 Subject: [PATCH] fix(router): ensure URL is updated after second redirect with UrlUpdateStrategy="eager" (#27523) Navigating to a route such as `/users`, you may get redirected to `/login`. Previously, if you go then route to `/users` again the URL will end up showing `/users` after the second redirect. This only happened in `UrlUpdateStrategy="eager"`. This is now fixed so after the second redirect, the URL shows the correct page. Fixes #27116 PR Close #27523 --- packages/router/src/router.ts | 13 +- packages/router/test/integration.spec.ts | 147 +++++++++++++++-------- 2 files changed, 110 insertions(+), 50 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index d398aa6fc2..1600fb9153 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -291,6 +291,7 @@ function defaultRouterHook(snapshot: RouterStateSnapshot, runExtras: { export class Router { private currentUrlTree: UrlTree; private rawUrlTree: UrlTree; + private browserUrlTree: UrlTree; private readonly transitions: BehaviorSubject; private navigations: Observable; private lastSuccessfulNavigation: Navigation|null = null; @@ -400,6 +401,7 @@ export class Router { this.resetConfig(config); this.currentUrlTree = createEmptyUrlTree(); this.rawUrlTree = this.currentUrlTree; + this.browserUrlTree = this.parseUrl(this.location.path()); this.configLoader = new RouterConfigLoader(loader, compiler, onLoadStart, onLoadEnd); this.routerState = createEmptyState(this.currentUrlTree, this.rootComponentType); @@ -461,7 +463,7 @@ export class Router { return of (t).pipe( switchMap(t => { const urlTransition = - !this.navigated || t.extractedUrl.toString() !== this.currentUrlTree.toString(); + !this.navigated || t.extractedUrl.toString() !== this.browserUrlTree.toString(); const processCurrentUrl = (this.onSameUrlNavigation === 'reload' ? true : urlTransition) && this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl); @@ -502,8 +504,12 @@ export class Router { this.paramsInheritanceStrategy, this.relativeLinkResolution), // Update URL if in `eager` update mode - tap(t => this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange && - this.setBrowserUrl(t.urlAfterRedirects, !!t.extras.replaceUrl, t.id)), + tap(t => { + if (this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange) { + this.setBrowserUrl(t.urlAfterRedirects, !!t.extras.replaceUrl, t.id); + this.browserUrlTree = t.urlAfterRedirects; + } + }), // Fire RoutesRecognized tap(t => { @@ -665,6 +671,7 @@ export class Router { if (this.urlUpdateStrategy === 'deferred' && !t.extras.skipLocationChange) { this.setBrowserUrl(this.rawUrlTree, !!t.extras.replaceUrl, t.id, t.extras.state); + this.browserUrlTree = t.urlAfterRedirects; } }), diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index b6483873c5..aaa25564ed 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -13,7 +13,7 @@ import {ComponentFixture, TestBed, fakeAsync, inject, tick} from '@angular/core/ import {By} from '@angular/platform-browser/src/dom/debug/by'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {fixmeIvy} from '@angular/private/testing'; -import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, Navigation, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, PRIMARY_OUTLET, ParamMap, Params, PreloadAllModules, PreloadingStrategy, Resolve, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, RouteReuseStrategy, Router, RouterEvent, RouterModule, RouterPreloader, RouterStateSnapshot, RoutesRecognized, RunGuardsAndResolvers, UrlHandlingStrategy, UrlSegmentGroup, UrlSerializer, UrlTree} from '@angular/router'; +import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DefaultUrlSerializer, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, Navigation, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, PRIMARY_OUTLET, ParamMap, Params, PreloadAllModules, PreloadingStrategy, Resolve, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, RouteReuseStrategy, Router, RouterEvent, RouterModule, RouterPreloader, RouterStateSnapshot, RoutesRecognized, RunGuardsAndResolvers, UrlHandlingStrategy, UrlSegmentGroup, UrlSerializer, UrlTree} from '@angular/router'; import {Observable, Observer, Subscription, of } from 'rxjs'; import {filter, first, map, tap} from 'rxjs/operators'; @@ -575,64 +575,110 @@ describe('Integration', () => { expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); }))); - it('should eagerly update the URL with urlUpdateStrategy="eagar"', - fakeAsync(inject([Router, Location], (router: Router, location: Location) => { - const fixture = TestBed.createComponent(RootCmp); - advance(fixture); + describe('"eager" urlUpdateStrategy', () => { + beforeEach(() => { + const serializer = new DefaultUrlSerializer(); + TestBed.configureTestingModule({ + providers: [{ + provide: 'authGuardFail', + useValue: (a: any, b: any) => { + return new Promise(res => { setTimeout(() => res(serializer.parse('/login')), 1); }); + } + }] + }); - router.resetConfig([{path: 'team/:id', component: TeamCmp}]); + }); - router.navigateByUrl('/team/22'); - advance(fixture); - expect(location.path()).toEqual('/team/22'); - expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); + it('should eagerly update the URL with urlUpdateStrategy="eagar"', + fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = TestBed.createComponent(RootCmp); + advance(fixture); + + router.resetConfig([{path: 'team/:id', component: TeamCmp}]); + + router.navigateByUrl('/team/22'); + advance(fixture); + expect(location.path()).toEqual('/team/22'); - router.urlUpdateStrategy = 'eager'; - (router as any).hooks.beforePreactivation = () => { - expect(location.path()).toEqual('/team/33'); expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); - return of (null); - }; - router.navigateByUrl('/team/33'); - advance(fixture); - expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); - }))); + router.urlUpdateStrategy = 'eager'; + (router as any).hooks.beforePreactivation = () => { + expect(location.path()).toEqual('/team/33'); + expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); + return of (null); + }; + router.navigateByUrl('/team/33'); - it('should eagerly update URL after redirects are applied with urlUpdateStrategy="eagar"', - fakeAsync(inject([Router, Location], (router: Router, location: Location) => { - const fixture = TestBed.createComponent(RootCmp); - advance(fixture); + advance(fixture); + expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); + }))); + it('should eagerly update the URL with urlUpdateStrategy="eagar"', + fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = TestBed.createComponent(RootCmp); + advance(fixture); - router.resetConfig([{path: 'team/:id', component: TeamCmp}]); + router.urlUpdateStrategy = 'eager'; - router.navigateByUrl('/team/22'); - advance(fixture); - expect(location.path()).toEqual('/team/22'); + router.resetConfig([ + {path: 'team/:id', component: SimpleCmp, canActivate: ['authGuardFail']}, + {path: 'login', component: AbsoluteSimpleLinkCmp} + ]); - expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); + router.navigateByUrl('/team/22'); + advance(fixture); + expect(location.path()).toEqual('/team/22'); - router.urlUpdateStrategy = 'eager'; + // Redirects to /login + advance(fixture, 1); + expect(location.path()).toEqual('/login'); - let urlAtNavStart = ''; - let urlAtRoutesRecognized = ''; - router.events.subscribe(e => { - if (e instanceof NavigationStart) { - urlAtNavStart = location.path(); - } - if (e instanceof RoutesRecognized) { - urlAtRoutesRecognized = location.path(); - } - }); + // Perform the same logic again, and it should produce the same result + router.navigateByUrl('/team/22'); + advance(fixture); + expect(location.path()).toEqual('/team/22'); - router.navigateByUrl('/team/33'); + // Redirects to /login + advance(fixture, 1); + expect(location.path()).toEqual('/login'); + }))); - advance(fixture); - expect(urlAtNavStart).toBe('/team/22'); - expect(urlAtRoutesRecognized).toBe('/team/33'); - expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); - }))); + it('should eagerly update URL after redirects are applied with urlUpdateStrategy="eagar"', + fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = TestBed.createComponent(RootCmp); + advance(fixture); + + router.resetConfig([{path: 'team/:id', component: TeamCmp}]); + + router.navigateByUrl('/team/22'); + advance(fixture); + expect(location.path()).toEqual('/team/22'); + + expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); + + router.urlUpdateStrategy = 'eager'; + + let urlAtNavStart = ''; + let urlAtRoutesRecognized = ''; + router.events.subscribe(e => { + if (e instanceof NavigationStart) { + urlAtNavStart = location.path(); + } + if (e instanceof RoutesRecognized) { + urlAtRoutesRecognized = location.path(); + } + }); + + router.navigateByUrl('/team/33'); + + advance(fixture); + expect(urlAtNavStart).toBe('/team/22'); + expect(urlAtRoutesRecognized).toBe('/team/33'); + expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); + }))); + + }); it('should navigate back and forward', fakeAsync(inject([Router, Location], (router: Router, location: Location) => { @@ -4627,6 +4673,10 @@ class DummyLinkCmp { } } +@Component({selector: 'link-cmp', template: `link`}) +class AbsoluteSimpleLinkCmp { +} + @Component({selector: 'link-cmp', template: `link`}) class RelativeLinkCmp { } @@ -4807,8 +4857,8 @@ class ThrowingCmp { -function advance(fixture: ComponentFixture): void { - tick(); +function advance(fixture: ComponentFixture, millis?: number): void { + tick(millis); fixture.detectChanges(); } @@ -4836,6 +4886,7 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, + AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment, @@ -4864,6 +4915,7 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, + AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment, @@ -4894,6 +4946,7 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, + AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment,