From aa85856e1c7f03181066fbf82c5f3c1b2e2ca6c9 Mon Sep 17 00:00:00 2001 From: Brian Ford Date: Mon, 7 Dec 2015 16:05:57 -0800 Subject: [PATCH] fix(router): set correct redirect/default URL from hashchange Currently, hashchange events outside of Angular that cause navigation do not take into account cases where the initial route URL changes due to a redirect or a default route. Closes #5590 Closes #5683 --- modules/angular2/src/mock/location_mock.ts | 20 +++++++++- .../src/mock/mock_location_strategy.ts | 22 ++++++++--- .../src/router/hash_location_strategy.ts | 13 ++++++- modules/angular2/src/router/location.ts | 14 ++++++- .../angular2/src/router/location_strategy.ts | 1 + .../src/router/path_location_strategy.ts | 5 +++ .../angular2/src/router/platform_location.ts | 4 ++ modules/angular2/src/router/router.ts | 34 ++++++++++++++++- modules/angular2/test/router/router_spec.ts | 38 ++++++++++++++++++- 9 files changed, 139 insertions(+), 12 deletions(-) diff --git a/modules/angular2/src/mock/location_mock.ts b/modules/angular2/src/mock/location_mock.ts index ca61672f4c..551dc8cca7 100644 --- a/modules/angular2/src/mock/location_mock.ts +++ b/modules/angular2/src/mock/location_mock.ts @@ -21,7 +21,16 @@ export class SpyLocation implements Location { path(): string { return this._path; } - simulateUrlPop(pathname: string) { ObservableWrapper.callEmit(this._subject, {'url': pathname}); } + simulateUrlPop(pathname: string) { + ObservableWrapper.callEmit(this._subject, {'url': pathname, 'pop': true}); + } + + simulateHashChange(pathname: string) { + // Because we don't prevent the native event, the browser will independently update the path + this.setInitialPath(pathname); + this.urlChanges.push('hash: ' + pathname); + ObservableWrapper.callEmit(this._subject, {'url': pathname, 'pop': true, 'type': 'hashchange'}); + } prepareExternalUrl(url: string): string { if (url.length > 0 && !url.startsWith('/')) { @@ -42,6 +51,15 @@ export class SpyLocation implements Location { this.urlChanges.push(url); } + replaceState(path: string, query: string = '') { + path = this.prepareExternalUrl(path); + this._path = path; + this._query = query; + + var url = path + (query.length > 0 ? ('?' + query) : ''); + this.urlChanges.push('replace: ' + url); + } + forward() { // TODO } diff --git a/modules/angular2/src/mock/mock_location_strategy.ts b/modules/angular2/src/mock/mock_location_strategy.ts index 1c6b48d639..0bd8f7a746 100644 --- a/modules/angular2/src/mock/mock_location_strategy.ts +++ b/modules/angular2/src/mock/mock_location_strategy.ts @@ -15,7 +15,7 @@ export class MockLocationStrategy extends LocationStrategy { simulatePopState(url: string): void { this.internalPath = url; - ObservableWrapper.callEmit(this._subject, null); + ObservableWrapper.callEmit(this._subject, new MockPopStateEvent(this.path())); } path(): string { return this.internalPath; } @@ -27,10 +27,6 @@ export class MockLocationStrategy extends LocationStrategy { return this.internalBaseHref + internal; } - simulateUrlPop(pathname: string): void { - ObservableWrapper.callEmit(this._subject, {'url': pathname}); - } - pushState(ctx: any, title: string, path: string, query: string): void { this.internalTitle = title; @@ -41,6 +37,16 @@ export class MockLocationStrategy extends LocationStrategy { this.urlChanges.push(externalUrl); } + replaceState(ctx: any, title: string, path: string, query: string): void { + this.internalTitle = title; + + var url = path + (query.length > 0 ? ('?' + query) : ''); + this.internalPath = url; + + var externalUrl = this.prepareExternalUrl(url); + this.urlChanges.push('replace: ' + externalUrl); + } + onPopState(fn: (value: any) => void): void { ObservableWrapper.subscribe(this._subject, fn); } getBaseHref(): string { return this.internalBaseHref; } @@ -55,3 +61,9 @@ export class MockLocationStrategy extends LocationStrategy { forward(): void { throw 'not implemented'; } } + +class MockPopStateEvent { + pop: boolean = true; + type: string = 'popstate'; + constructor(public newUrl: string) {} +} diff --git a/modules/angular2/src/router/hash_location_strategy.ts b/modules/angular2/src/router/hash_location_strategy.ts index 333cd188d2..43b2a602e3 100644 --- a/modules/angular2/src/router/hash_location_strategy.ts +++ b/modules/angular2/src/router/hash_location_strategy.ts @@ -58,7 +58,10 @@ export class HashLocationStrategy extends LocationStrategy { } } - onPopState(fn: EventListener): void { this._platformLocation.onPopState(fn); } + onPopState(fn: EventListener): void { + this._platformLocation.onPopState(fn); + this._platformLocation.onHashChange(fn); + } getBaseHref(): string { return this._baseHref; } @@ -87,6 +90,14 @@ export class HashLocationStrategy extends LocationStrategy { this._platformLocation.pushState(state, title, url); } + replaceState(state: any, title: string, path: string, queryParams: string) { + var url = this.prepareExternalUrl(path + normalizeQueryParams(queryParams)); + if (url.length == 0) { + url = this._platformLocation.pathname; + } + this._platformLocation.replaceState(state, title, url); + } + forward(): void { this._platformLocation.forward(); } back(): void { this._platformLocation.back(); } diff --git a/modules/angular2/src/router/location.ts b/modules/angular2/src/router/location.ts index 74e460d8a9..1d509a94cb 100644 --- a/modules/angular2/src/router/location.ts +++ b/modules/angular2/src/router/location.ts @@ -52,8 +52,9 @@ export class Location { constructor(public platformStrategy: LocationStrategy) { var browserBaseHref = this.platformStrategy.getBaseHref(); this._baseHref = stripTrailingSlash(stripIndexHtml(browserBaseHref)); - this.platformStrategy.onPopState( - (_) => { ObservableWrapper.callEmit(this._subject, {'url': this.path(), 'pop': true}); }); + this.platformStrategy.onPopState((ev) => { + ObservableWrapper.callEmit(this._subject, {'url': this.path(), 'pop': true, 'type': ev.type}); + }); } /** @@ -82,6 +83,7 @@ export class Location { return this.platformStrategy.prepareExternalUrl(url); } + // TODO: rename this method to pushState /** * Changes the browsers URL to the normalized version of the given URL, and pushes a * new item onto the platform's history. @@ -90,6 +92,14 @@ export class Location { this.platformStrategy.pushState(null, '', path, query); } + /** + * Changes the browsers URL to the normalized version of the given URL, and replaces + * the top item on the platform's history stack. + */ + replaceState(path: string, query: string = ''): void { + this.platformStrategy.replaceState(null, '', path, query); + } + /** * Navigates forward in the platform's history. */ diff --git a/modules/angular2/src/router/location_strategy.ts b/modules/angular2/src/router/location_strategy.ts index 3975701630..7a46e2b5f0 100644 --- a/modules/angular2/src/router/location_strategy.ts +++ b/modules/angular2/src/router/location_strategy.ts @@ -21,6 +21,7 @@ export abstract class LocationStrategy { abstract path(): string; abstract prepareExternalUrl(internal: string): string; abstract pushState(state: any, title: string, url: string, queryParams: string): void; + abstract replaceState(state: any, title: string, url: string, queryParams: string): void; abstract forward(): void; abstract back(): void; abstract onPopState(fn: (_: any) => any): void; diff --git a/modules/angular2/src/router/path_location_strategy.ts b/modules/angular2/src/router/path_location_strategy.ts index d9c9c03be6..9e4dfb6b0f 100644 --- a/modules/angular2/src/router/path_location_strategy.ts +++ b/modules/angular2/src/router/path_location_strategy.ts @@ -93,6 +93,11 @@ export class PathLocationStrategy extends LocationStrategy { this._platformLocation.pushState(state, title, externalUrl); } + replaceState(state: any, title: string, url: string, queryParams: string) { + var externalUrl = this.prepareExternalUrl(url + normalizeQueryParams(queryParams)); + this._platformLocation.replaceState(state, title, externalUrl); + } + forward(): void { this._platformLocation.forward(); } back(): void { this._platformLocation.back(); } diff --git a/modules/angular2/src/router/platform_location.ts b/modules/angular2/src/router/platform_location.ts index 79591b12de..c58ffb4e6e 100644 --- a/modules/angular2/src/router/platform_location.ts +++ b/modules/angular2/src/router/platform_location.ts @@ -40,6 +40,10 @@ export class PlatformLocation { this._history.pushState(state, title, url); } + replaceState(state: any, title: string, url: string): void { + this._history.replaceState(state, title, url); + } + forward(): void { this._history.forward(); } back(): void { this._history.back(); } diff --git a/modules/angular2/src/router/router.ts b/modules/angular2/src/router/router.ts index 404762ff27..aec01f52d9 100644 --- a/modules/angular2/src/router/router.ts +++ b/modules/angular2/src/router/router.ts @@ -425,8 +425,38 @@ export class RootRouter extends Router { @Inject(ROUTER_PRIMARY_COMPONENT) primaryComponent: Type) { super(registry, null, primaryComponent); this._location = location; - this._locationSub = this._location.subscribe( - (change) => this.navigateByUrl(change['url'], isPresent(change['pop']))); + this._locationSub = this._location.subscribe((change) => { + // we call recognize ourselves + this.recognize(change['url']) + .then((instruction) => { + this.navigateByInstruction(instruction, isPresent(change['pop'])) + .then((_) => { + // this is a popstate event; no need to change the URL + if (isPresent(change['pop']) && change['type'] != 'hashchange') { + return; + } + var emitPath = instruction.toUrlPath(); + var emitQuery = instruction.toUrlQuery(); + if (emitPath.length > 0) { + emitPath = '/' + emitPath; + } + + // Because we've opted to use All hashchange events occur outside Angular. + // However, apps that are migrating might have hash links that operate outside + // angular to which routing must respond. + // To support these cases where we respond to hashchanges and redirect as a + // result, we need to replace the top item on the stack. + if (change['type'] == 'hashchange') { + if (instruction.toRootUrl() != this._location.path()) { + this._location.replaceState(emitPath, emitQuery); + } + } else { + this._location.go(emitPath, emitQuery); + } + }); + }); + }); + this.registry.configFromComponent(primaryComponent); this.navigateByUrl(location.path()); } diff --git a/modules/angular2/test/router/router_spec.ts b/modules/angular2/test/router/router_spec.ts index 335218fdd1..164213b6c7 100644 --- a/modules/angular2/test/router/router_spec.ts +++ b/modules/angular2/test/router/router_spec.ts @@ -20,7 +20,7 @@ import {SpyLocation} from 'angular2/src/mock/location_mock'; import {Location} from 'angular2/src/router/location'; import {RouteRegistry, ROUTER_PRIMARY_COMPONENT} from 'angular2/src/router/route_registry'; -import {RouteConfig, AsyncRoute, Route} from 'angular2/src/router/route_config_decorator'; +import {RouteConfig, AsyncRoute, Route, Redirect} from 'angular2/src/router/route_config_decorator'; import {DirectiveResolver} from 'angular2/src/core/linker/directive_resolver'; import {provide} from 'angular2/core'; @@ -99,6 +99,42 @@ export function main() { }); })); + // See https://github.com/angular/angular/issues/5590 + it('should replace history when triggered by a hashchange with a redirect', + inject([AsyncTestCompleter], (async) => { + var outlet = makeDummyOutlet(); + + router.registerPrimaryOutlet(outlet) + .then((_) => router.config([ + new Redirect({path: '/a', redirectTo: ['B']}), + new Route({path: '/b', component: DummyComponent, name: 'B'}) + ])) + .then((_) => { + router.subscribe((_) => { + expect(location.urlChanges).toEqual(['hash: a', 'replace: /b']); + async.done(); + }); + + location.simulateHashChange('a'); + }); + })); + + it('should push history when triggered by a hashchange without a redirect', + inject([AsyncTestCompleter], (async) => { + var outlet = makeDummyOutlet(); + + router.registerPrimaryOutlet(outlet) + .then((_) => router.config([new Route({path: '/a', component: DummyComponent})])) + .then((_) => { + router.subscribe((_) => { + expect(location.urlChanges).toEqual(['hash: a']); + async.done(); + }); + + location.simulateHashChange('a'); + }); + })); + it('should navigate after being configured', inject([AsyncTestCompleter], (async) => { var outlet = makeDummyOutlet();