Skip to content

Commit

Permalink
fix(router): runs guards every time when unsuccessfully navigating to…
Browse files Browse the repository at this point in the history 
… the same url over and over again (#13209)
  • Loading branch information
@vsavkin @alxhub
vsavkin authored and alxhub committed Dec 2, 2016
1 parent bbb7a39 commit d46b8de
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 58 deletions.
42 changes: 25 additions & 17 deletions modules/@angular/router/src/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,11 @@ function defaultErrorHandler(error: any): any {
type NavigationParams = {
id: number,
rawUrl: UrlTree,
prevRawUrl: UrlTree,
extras: NavigationExtras,
resolve: any,
reject: any,
promise: Promise<boolean>
promise: Promise<boolean>,
imperative: boolean
};


Expand Down Expand Up @@ -387,8 +387,19 @@ export class Router {
// which does not work properly with zone.js in IE and Safari
this.locationSubscription = <any>this.location.subscribe(Zone.current.wrap((change: any) => {
const rawUrlTree = this.urlSerializer.parse(change['url']);
const lastNavigation = this.navigations.value;

// If the user triggers a navigation imperatively (e.g., by using navigateByUrl),
// and that navigation results in 'replaceState' that leads to the same URL,
// we should skip those.
if (lastNavigation && lastNavigation.imperative &&
lastNavigation.rawUrl.toString() === rawUrlTree.toString()) {
return;
}

setTimeout(() => {
this.scheduleNavigation(rawUrlTree, {skipLocationChange: change['pop'], replaceUrl: true});
this.scheduleNavigation(
rawUrlTree, false, {skipLocationChange: change['pop'], replaceUrl: true});
}, 0);
}));
}
Expand Down Expand Up @@ -510,11 +521,12 @@ export class Router {
navigateByUrl(url: string|UrlTree, extras: NavigationExtras = {skipLocationChange: false}):
Promise<boolean> {
if (url instanceof UrlTree) {
return this.scheduleNavigation(this.urlHandlingStrategy.merge(url, this.rawUrlTree), extras);
return this.scheduleNavigation(
this.urlHandlingStrategy.merge(url, this.rawUrlTree), true, extras);
} else {
const urlTree = this.urlSerializer.parse(url);
return this.scheduleNavigation(
this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree), extras);
this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree), true, extras);
}
}

Expand Down Expand Up @@ -596,12 +608,8 @@ export class Router {
.subscribe(() => {});
}

private scheduleNavigation(rawUrl: UrlTree, extras: NavigationExtras): Promise<boolean> {
const prevRawUrl = this.navigations.value ? this.navigations.value.rawUrl : null;
if (prevRawUrl && prevRawUrl.toString() === rawUrl.toString()) {
return this.navigations.value.promise;
}

private scheduleNavigation(rawUrl: UrlTree, imperative: boolean, extras: NavigationExtras):
Promise<boolean> {
let resolve: any = null;
let reject: any = null;

Expand All @@ -611,18 +619,17 @@ export class Router {
});

const id = ++this.navigationId;
this.navigations.next({id, rawUrl, prevRawUrl, extras, resolve, reject, promise});
this.navigations.next({id, imperative, rawUrl, extras, resolve, reject, promise});

// Make sure that the error is propagated even though `processNavigations` catch
// handler does not rethrow
return promise.catch((e: any) => Promise.reject(e));
}

private executeScheduledNavigation({id, rawUrl, prevRawUrl, extras, resolve,
reject}: NavigationParams): void {
private executeScheduledNavigation({id, rawUrl, extras, resolve, reject}: NavigationParams):
void {
const url = this.urlHandlingStrategy.extract(rawUrl);
const prevUrl = prevRawUrl ? this.urlHandlingStrategy.extract(prevRawUrl) : null;
const urlTransition = !prevUrl || url.toString() !== prevUrl.toString();
const urlTransition = !this.navigated || url.toString() !== this.currentUrlTree.toString();

if (urlTransition && this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) {
this.routerEvents.next(new NavigationStart(id, this.serializeUrl(url)));
Expand All @@ -635,7 +642,8 @@ export class Router {
// we cannot process the current URL, but we could process the previous one =>
// we need to do some cleanup
} else if (
urlTransition && prevRawUrl && this.urlHandlingStrategy.shouldProcessUrl(prevRawUrl)) {
urlTransition && this.rawUrlTree &&
this.urlHandlingStrategy.shouldProcessUrl(this.rawUrlTree)) {
this.routerEvents.next(new NavigationStart(id, this.serializeUrl(url)));
Promise.resolve()
.then(
Expand Down
114 changes: 73 additions & 41 deletions modules/@angular/router/test/integration.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1383,6 +1383,13 @@ describe('Integration', () => {
useValue:
(c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => { return false; }
},
{
provide: 'alwaysFalseAndLogging',
useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
log.push('called');
return false;
}
},
]
});
});
Expand Down Expand Up @@ -1532,62 +1539,86 @@ describe('Integration', () => {
expect(location.path()).toEqual('/main/component1');
})));

});

describe('should work when given a class', () => {
class AlwaysTrue implements CanDeactivate<TeamCmp> {
canDeactivate(
component: TeamCmp, route: ActivatedRouteSnapshot,
state: RouterStateSnapshot): boolean {
return true;
}
}
it('should call guards every time when navigating to the same url over and over again',
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, RootCmp);

beforeEach(() => { TestBed.configureTestingModule({providers: [AlwaysTrue]}); });
router.resetConfig([
{path: 'simple', component: SimpleCmp, canDeactivate: ['alwaysFalseAndLogging']},
{path: 'blank', component: BlankCmp}

it('works', fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, RootCmp);
]);

router.resetConfig(
[{path: 'team/:id', component: TeamCmp, canDeactivate: [AlwaysTrue]}]);
router.navigateByUrl('/simple');
advance(fixture);

router.navigateByUrl('/team/22');
router.navigateByUrl('/blank');
advance(fixture);
expect(location.path()).toEqual('/team/22');
expect(log).toEqual(['called']);
expect(location.path()).toEqual('/simple');

router.navigateByUrl('/team/33');
router.navigateByUrl('/blank');
advance(fixture);
expect(location.path()).toEqual('/team/33');
expect(log).toEqual(['called', 'called']);
expect(location.path()).toEqual('/simple');
})));
});


describe('should work when returns an observable', () => {
beforeEach(() => {
TestBed.configureTestingModule({
providers: [{
provide: 'CanDeactivate',
useValue: (c: TeamCmp, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
return Observable.create((observer: any) => { observer.next(false); });
}
}]
});
describe('should work when given a class', () => {
class AlwaysTrue implements CanDeactivate<TeamCmp> {
canDeactivate(
component: TeamCmp, route: ActivatedRouteSnapshot,
state: RouterStateSnapshot): boolean {
return true;
}
}

beforeEach(() => { TestBed.configureTestingModule({providers: [AlwaysTrue]}); });

it('works', fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, RootCmp);

router.resetConfig(
[{path: 'team/:id', component: TeamCmp, canDeactivate: [AlwaysTrue]}]);

router.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');

router.navigateByUrl('/team/33');
advance(fixture);
expect(location.path()).toEqual('/team/33');
})));
});

it('works', fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, RootCmp);

router.resetConfig(
[{path: 'team/:id', component: TeamCmp, canDeactivate: ['CanDeactivate']}]);
describe('should work when returns an observable', () => {
beforeEach(() => {
TestBed.configureTestingModule({
providers: [{
provide: 'CanDeactivate',
useValue: (c: TeamCmp, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
return Observable.create((observer: any) => { observer.next(false); });
}
}]
});
});

router.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');
it('works', fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, RootCmp);

router.navigateByUrl('/team/33');
advance(fixture);
expect(location.path()).toEqual('/team/22');
})));
router.resetConfig(
[{path: 'team/:id', component: TeamCmp, canDeactivate: ['CanDeactivate']}]);

router.navigateByUrl('/team/22');
advance(fixture);
expect(location.path()).toEqual('/team/22');

router.navigateByUrl('/team/33');
advance(fixture);
expect(location.path()).toEqual('/team/22');
})));
});
});
});

Expand Down Expand Up @@ -1826,6 +1857,7 @@ describe('Integration', () => {

TestBed.configureTestingModule({declarations: [RootCmpWithLink]});
const router: Router = TestBed.get(Router);
const loc: any = TestBed.get(Location);

const f = TestBed.createComponent(RootCmpWithLink);
advance(f);
Expand Down

0 comments on commit d46b8de

Please sign in to comment.