From eea2b0f288eec889d56afb07dad21f75e77f1241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 25 Dec 2018 21:37:36 -0800 Subject: [PATCH] revert: fix(router): ensure URL is updated after second redirect with UrlUpdateStrategy="eager" (#27523) --- packages/router/src/router.ts | 13 +- packages/router/test/integration.spec.ts | 147 ++++++++--------------- 2 files changed, 50 insertions(+), 110 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 1600fb9153..d398aa6fc2 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -291,7 +291,6 @@ 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; @@ -401,7 +400,6 @@ 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); @@ -463,7 +461,7 @@ export class Router { return of (t).pipe( switchMap(t => { const urlTransition = - !this.navigated || t.extractedUrl.toString() !== this.browserUrlTree.toString(); + !this.navigated || t.extractedUrl.toString() !== this.currentUrlTree.toString(); const processCurrentUrl = (this.onSameUrlNavigation === 'reload' ? true : urlTransition) && this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl); @@ -504,12 +502,8 @@ export class Router { this.paramsInheritanceStrategy, this.relativeLinkResolution), // Update URL if in `eager` update mode - tap(t => { - if (this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange) { - this.setBrowserUrl(t.urlAfterRedirects, !!t.extras.replaceUrl, t.id); - this.browserUrlTree = t.urlAfterRedirects; - } - }), + tap(t => this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange && + this.setBrowserUrl(t.urlAfterRedirects, !!t.extras.replaceUrl, t.id)), // Fire RoutesRecognized tap(t => { @@ -671,7 +665,6 @@ 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 9b8a0d326d..37fb3db386 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, 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 {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 {Observable, Observer, Subscription, of } from 'rxjs'; import {filter, first, map, tap} from 'rxjs/operators'; @@ -575,110 +575,64 @@ describe('Integration', () => { expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]'); }))); - 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); }); - } - }] - }); + 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'); - 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'); + expect(fixture.nativeElement).toHaveText('team 22 [ , 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'); - 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: ]'); + }))); - 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); + 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.urlUpdateStrategy = 'eager'; + router.resetConfig([{path: 'team/:id', component: TeamCmp}]); - router.resetConfig([ - {path: 'team/:id', component: SimpleCmp, canActivate: ['authGuardFail']}, - {path: 'login', component: AbsoluteSimpleLinkCmp} - ]); + router.navigateByUrl('/team/22'); + advance(fixture); + expect(location.path()).toEqual('/team/22'); - router.navigateByUrl('/team/22'); - advance(fixture); - expect(location.path()).toEqual('/team/22'); + expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]'); - // Redirects to /login - advance(fixture, 1); - expect(location.path()).toEqual('/login'); + router.urlUpdateStrategy = 'eager'; - // Perform the same logic again, and it should produce the same result - router.navigateByUrl('/team/22'); - advance(fixture); - expect(location.path()).toEqual('/team/22'); + let urlAtNavStart = ''; + let urlAtRoutesRecognized = ''; + router.events.subscribe(e => { + if (e instanceof NavigationStart) { + urlAtNavStart = location.path(); + } + if (e instanceof RoutesRecognized) { + urlAtRoutesRecognized = location.path(); + } + }); - // Redirects to /login - advance(fixture, 1); - expect(location.path()).toEqual('/login'); - }))); + 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); - - 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: ]'); - }))); - - }); + 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) => { @@ -4713,10 +4667,6 @@ class DummyLinkCmp { } } -@Component({selector: 'link-cmp', template: `link`}) -class AbsoluteSimpleLinkCmp { -} - @Component({selector: 'link-cmp', template: `link`}) class RelativeLinkCmp { } @@ -4897,8 +4847,8 @@ class ThrowingCmp { -function advance(fixture: ComponentFixture, millis?: number): void { - tick(millis); +function advance(fixture: ComponentFixture): void { + tick(); fixture.detectChanges(); } @@ -4926,7 +4876,6 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, - AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment, @@ -4955,7 +4904,6 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, - AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment, @@ -4986,7 +4934,6 @@ class LazyComponent { StringLinkCmp, DummyLinkCmp, AbsoluteLinkCmp, - AbsoluteSimpleLinkCmp, RelativeLinkCmp, DummyLinkWithParentCmp, LinkWithQueryParamsAndFragment,