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 b67d574a95
commit fdfa31798b
2 changed files with 84 additions and 26 deletions

View File

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

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 {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 {SpyLocation} from 'common/testing';
import {Observable} from 'rxjs/Observable';
import {Observer} from 'rxjs/Observer';
import {of } from 'rxjs/observable/of';
@ -50,8 +51,8 @@ describe('Integration', () => {
expect(fixture.nativeElement).toHaveText('route');
})));
it('should navigate to the current URL',
fakeAsync(inject([Router, Location], (router: Router) => {
describe('navigation', function() {
it('should navigate to the current URL', fakeAsync(inject([Router], (router: Router) => {
router.resetConfig([
{path: '', component: SimpleCmp},
{path: 'simple', component: SimpleCmp},
@ -73,6 +74,65 @@ describe('Integration', () => {
]);
})));
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', () => {
let log: any[] = [];