fix(router): do not call `location.go` when skipping a navigation (#19463)

Closes #18036

PR Close #19463
This commit is contained in:
vsavkin 2017-09-29 11:03:16 -04:00 committed by Chuck Jazdzewski
parent 43c5b638b9
commit 6651541230
2 changed files with 84 additions and 26 deletions

View File

@ -580,10 +580,9 @@ export class Router {
} }
private runNavigate( private runNavigate(
url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean, url: UrlTree, rawUrl: UrlTree, skipLocationChange: boolean, replaceUrl: boolean, id: number,
id: number, precreatedState: RouterStateSnapshot|null): Promise<boolean> { precreatedState: RouterStateSnapshot|null): Promise<boolean> {
if (id !== this.navigationId) { if (id !== this.navigationId) {
this.location.go(this.urlSerializer.serialize(this.currentUrlTree));
(this.events as Subject<Event>) (this.events as Subject<Event>)
.next(new NavigationCancel( .next(new NavigationCancel(
id, this.serializeUrl(url), id, this.serializeUrl(url),
@ -705,9 +704,9 @@ export class Router {
(this as{routerState: RouterState}).routerState = state; (this as{routerState: RouterState}).routerState = state;
if (!shouldPreventPushState) { if (!skipLocationChange) {
const path = this.urlSerializer.serialize(this.rawUrlTree); const path = this.urlSerializer.serialize(this.rawUrlTree);
if (this.location.isCurrentPathEqualTo(path) || shouldReplaceUrl) { if (this.location.isCurrentPathEqualTo(path) || replaceUrl) {
this.location.replaceState(path); this.location.replaceState(path);
} else { } else {
this.location.go(path); this.location.go(path);
@ -755,14 +754,13 @@ export class Router {
(this as{routerState: RouterState}).routerState = storedState; (this as{routerState: RouterState}).routerState = storedState;
this.currentUrlTree = storedUrl; this.currentUrlTree = storedUrl;
this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl);
this.location.replaceState(this.serializeUrl(this.rawUrlTree)); this.resetUrlToCurrentUrlTree();
}); });
}); });
} }
private resetUrlToCurrentUrlTree(): void { private resetUrlToCurrentUrlTree(): void {
const path = this.urlSerializer.serialize(this.rawUrlTree); this.location.replaceState(this.urlSerializer.serialize(this.rawUrlTree));
this.location.replaceState(path);
} }
} }

View File

@ -12,6 +12,7 @@ import {ComponentFixture, TestBed, fakeAsync, inject, tick} from '@angular/core/
import {By} from '@angular/platform-browser/src/dom/debug/by'; import {By} from '@angular/platform-browser/src/dom/debug/by';
import {expect} from '@angular/platform-browser/testing/src/matchers'; import {expect} from '@angular/platform-browser/testing/src/matchers';
import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, 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, UrlTree} from '@angular/router'; import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, 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, UrlTree} from '@angular/router';
import {SpyLocation} from 'common/testing';
import {Observable} from 'rxjs/Observable'; import {Observable} from 'rxjs/Observable';
import {Observer} from 'rxjs/Observer'; import {Observer} from 'rxjs/Observer';
import {of } from 'rxjs/observable/of'; import {of } from 'rxjs/observable/of';
@ -50,28 +51,87 @@ describe('Integration', () => {
expect(fixture.nativeElement).toHaveText('route'); expect(fixture.nativeElement).toHaveText('route');
}))); })));
it('should navigate to the current URL', describe('navigation', function() {
fakeAsync(inject([Router, Location], (router: Router) => { it('should navigate to the current URL', fakeAsync(inject([Router], (router: Router) => {
router.resetConfig([ router.resetConfig([
{path: '', component: SimpleCmp}, {path: '', component: SimpleCmp},
{path: 'simple', component: SimpleCmp}, {path: 'simple', component: SimpleCmp},
]); ]);
const fixture = createRoot(router, RootCmp); const fixture = createRoot(router, RootCmp);
const events: Event[] = []; const events: Event[] = [];
router.events.subscribe(e => onlyNavigationStartAndEnd(e) && events.push(e)); router.events.subscribe(e => onlyNavigationStartAndEnd(e) && events.push(e));
router.navigateByUrl('/simple'); router.navigateByUrl('/simple');
tick(); tick();
router.navigateByUrl('/simple'); router.navigateByUrl('/simple');
tick(); tick();
expectEvents(events, [ expectEvents(events, [
[NavigationStart, '/simple'], [NavigationEnd, '/simple'], [NavigationStart, '/simple'], [NavigationStart, '/simple'], [NavigationEnd, '/simple'], [NavigationStart, '/simple'],
[NavigationEnd, '/simple'] [NavigationEnd, '/simple']
]); ]);
}))); })));
it('should not pollute browser history when replaceUrl is set to true',
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
router.resetConfig([
{path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp},
{path: 'b', component: SimpleCmp}
]);
const fixture = createRoot(router, RootCmp);
router.navigateByUrl('/a', {replaceUrl: true});
router.navigateByUrl('/b', {replaceUrl: true});
tick();
expect(location.urlChanges).toEqual(['replace: /', 'replace: /b']);
})));
it('should skip navigation if another navigation is already scheduled',
fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
router.resetConfig([
{path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp},
{path: 'b', component: SimpleCmp}
]);
const fixture = createRoot(router, RootCmp);
router.navigate(
['/a'], {queryParams: {a: true}, queryParamsHandling: 'merge', replaceUrl: true});
router.navigate(
['/b'], {queryParams: {b: true}, queryParamsHandling: 'merge', replaceUrl: true});
tick();
/**
* Why do we have '/b?b=true' and not '/b?a=true&b=true'?
*
* This is because the router has the right to stop a navigation mid-flight if another
* navigation has been already scheduled. This is why we can use a top-level guard
* to perform redirects. Calling `navigate` in such a guard will stop the navigation, and
* the components won't be instantiated.
*
* This is a fundamental property of the router: it only cares about its latest state.
*
* This means that components should only map params to something else, not reduce them.
* In other words, the following component is asking for trouble:
*
* ```
* class MyComponent {
* constructor(a: ActivatedRoute) {
* a.params.scan(...)
* }
* }
* ```
*
* This also means "queryParamsHandling: 'merge'" should only be used to merge with
* long-living query parameters (e.g., debug).
*/
expect(router.url).toEqual('/b?b=true');
})));
});
describe('should execute navigations serially', () => { describe('should execute navigations serially', () => {
let log: any[] = []; let log: any[] = [];