Skip to content

Commit

Permalink
revert: fix(router): ensure URL is updated after second redirect with…
Browse files Browse the repository at this point in the history 
… UrlUpdateStrategy="eager" (angular#27523)
  • Loading branch information
@matsko @ngfelixl
matsko authored and ngfelixl committed Jan 27, 2019
1 parent c8df245 commit 8f8959f
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 110 deletions.
13 changes: 3 additions & 10 deletions packages/router/src/router.ts
View file
Expand Up @@ -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<NavigationTransition>;
private navigations: Observable<NavigationTransition>;
private lastSuccessfulNavigation: Navigation|null = null;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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;
}
}),

Expand Down
147 changes: 47 additions & 100 deletions packages/router/test/integration.spec.ts
View file
Expand Up @@ -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';

Expand Down Expand Up @@ -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);

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.resetConfig([{path: 'team/:id', component: TeamCmp}]);
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: ]');

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: ]');
})));
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.urlUpdateStrategy = 'eager';

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');

// Redirects to /login
advance(fixture, 1);
expect(location.path()).toEqual('/login');

// Perform the same logic again, and it should produce the same result
router.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');

// Redirects to /login
advance(fixture, 1);
expect(location.path()).toEqual('/login');
})));

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: ]');
})));

router.resetConfig([{path: 'team/:id', component: TeamCmp}]);
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.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');
router.resetConfig([{path: 'team/:id', component: TeamCmp}]);

expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]');
router.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');

router.urlUpdateStrategy = 'eager';
expect(fixture.nativeElement).toHaveText('team 22 [ , right: ]');

let urlAtNavStart = '';
let urlAtRoutesRecognized = '';
router.events.subscribe(e => {
if (e instanceof NavigationStart) {
urlAtNavStart = location.path();
}
if (e instanceof RoutesRecognized) {
urlAtRoutesRecognized = location.path();
}
});
router.urlUpdateStrategy = 'eager';

router.navigateByUrl('/team/33');
let urlAtNavStart = '';
let urlAtRoutesRecognized = '';
router.events.subscribe(e => {
if (e instanceof NavigationStart) {
urlAtNavStart = location.path();
}
if (e instanceof RoutesRecognized) {
urlAtRoutesRecognized = location.path();
}
});

advance(fixture);
expect(urlAtNavStart).toBe('/team/22');
expect(urlAtRoutesRecognized).toBe('/team/33');
expect(fixture.nativeElement).toHaveText('team 33 [ , right: ]');
})));
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) => {
Expand Down Expand Up @@ -4713,10 +4667,6 @@ class DummyLinkCmp {
}
}

@Component({selector: 'link-cmp', template: `<a [routerLink]="['/simple']">link</a>`})
class AbsoluteSimpleLinkCmp {
}

@Component({selector: 'link-cmp', template: `<a [routerLink]="['../simple']">link</a>`})
class RelativeLinkCmp {
}
Expand Down Expand Up @@ -4897,8 +4847,8 @@ class ThrowingCmp {



function advance(fixture: ComponentFixture<any>, millis?: number): void {
tick(millis);
function advance(fixture: ComponentFixture<any>): void {
tick();
fixture.detectChanges();
}

Expand Down Expand Up @@ -4926,7 +4876,6 @@ class LazyComponent {
StringLinkCmp,
DummyLinkCmp,
AbsoluteLinkCmp,
AbsoluteSimpleLinkCmp,
RelativeLinkCmp,
DummyLinkWithParentCmp,
LinkWithQueryParamsAndFragment,
Expand Down Expand Up @@ -4955,7 +4904,6 @@ class LazyComponent {
StringLinkCmp,
DummyLinkCmp,
AbsoluteLinkCmp,
AbsoluteSimpleLinkCmp,
RelativeLinkCmp,
DummyLinkWithParentCmp,
LinkWithQueryParamsAndFragment,
Expand Down Expand Up @@ -4986,7 +4934,6 @@ class LazyComponent {
StringLinkCmp,
DummyLinkCmp,
AbsoluteLinkCmp,
AbsoluteSimpleLinkCmp,
RelativeLinkCmp,
DummyLinkWithParentCmp,
LinkWithQueryParamsAndFragment,
Expand Down

0 comments on commit 8f8959f

Please sign in to comment.