fix(router): respect LocationStrategy when constructing hrefs in links

Note that this introduces more behavior for LocationStrategy which needs
yet more refactoring to test. See #4935.

Closes #4333

modules/angular2/src/mock/location_mock.ts

@@ -21,10 +21,15 @@ export class SpyLocation implements Location {
 
   simulateUrlPop(pathname: string) { ObservableWrapper.callNext(this._subject, {'url': pathname}); }
 
-  normalizeAbsolutely(url: string): string { return this._baseHref + url; }
+  prepareExternalUrl(url: string): string {
+    if (url.length > 0 && !url.startsWith('/')) {
+      url = '/' + url;
+    }
+    return this._baseHref + url;
+  }
 
   go(path: string, query: string = '') {
-    path = this.normalizeAbsolutely(path);
+    path = this.prepareExternalUrl(path);
     if (this._path == path && this._query == query) {
       return;
     }

modules/angular2/src/mock/mock_location_strategy.ts

@@ -18,6 +18,8 @@ export class MockLocationStrategy extends LocationStrategy {
 
   path(): string { return this.internalPath; }
 
+  prepareExternalUrl(internal: string): string { return internal; }
+
   simulateUrlPop(pathname: string): void {
     ObservableWrapper.callNext(this._subject, {'url': pathname});
   }

modules/angular2/src/router/hash_location_strategy.ts

@@ -65,12 +65,16 @@ export class HashLocationStrategy extends LocationStrategy {
            normalizeQueryParams(this._location.search);
   }
 
+  prepareExternalUrl(internal: string): string {
+    return internal.length > 0 ? ('#' + internal) : internal;
+  }
+
   pushState(state: any, title: string, path: string, queryParams: string) {
     var url = path + normalizeQueryParams(queryParams);
     if (url.length == 0) {
       url = this._location.pathname;
     } else {
-      url = '#' + url;
+      url = this.prepareExternalUrl(url);
     }
     this._history.pushState(state, title, url);
   }

modules/angular2/src/router/location.ts

@@ -103,22 +103,25 @@ export class Location {
   path(): string { return this.normalize(this.platformStrategy.path()); }
 
   /**
-   * Given a string representing a URL, returns the normalized URL path.
+   * Given a string representing a URL, returns the normalized URL path without leading or
+   * trailing slashes
    */
   normalize(url: string): string {
     return stripTrailingSlash(_stripBaseHref(this._baseHref, stripIndexHtml(url)));
   }
 
   /**
-   * Given a string representing a URL, returns the normalized URL path.
+   * Given a string representing a URL, returns the platform-specific external URL path.
    * If the given URL doesn't begin with a leading slash (`'/'`), this method adds one
-   * before normalizing.
+   * before normalizing. This method will also add a hash if `HashLocationStrategy` is
+   * used, or the `APP_BASE_HREF` if the `PathLocationStrategy` is in use.
    */
-  normalizeAbsolutely(url: string): string {
+  prepareExternalUrl(url: string): string {
     if (!url.startsWith('/')) {
       url = '/' + url;
     }
-    return stripTrailingSlash(_addBaseHref(this._baseHref, url));
+    return this.platformStrategy.prepareExternalUrl(
+        stripTrailingSlash(_addBaseHref(this._baseHref, url)));
   }
 
   /**
@@ -126,8 +129,7 @@ export class Location {
    * new item onto the platform's history.
    */
   go(path: string, query: string = ''): void {
-    var absolutePath = this.normalizeAbsolutely(path);
+    this.platformStrategy.pushState(null, '', path, query);
-    this.platformStrategy.pushState(null, '', absolutePath, query);
   }
 
   /**

modules/angular2/src/router/location_strategy.ts

@@ -16,6 +16,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 forward(): void;
   abstract back(): void;

modules/angular2/src/router/path_location_strategy.ts

@@ -67,6 +67,8 @@ export class PathLocationStrategy extends LocationStrategy {
 
   getBaseHref(): string { return this._baseHref; }
 
+  prepareExternalUrl(internal: string): string { return this._baseHref + internal; }
+
   path(): string { return this._location.pathname + normalizeQueryParams(this._location.search); }
 
   pushState(state: any, title: string, url: string, queryParams: string) {

modules/angular2/src/router/router_link.ts

@@ -60,9 +60,8 @@ export class RouterLink {
     this._routeParams = changes;
     this._navigationInstruction = this._router.generate(this._routeParams);
 
-    // TODO: is this the right spot for this?
+    var navigationHref = stringifyInstruction(this._navigationInstruction);
-    var navigationHref = '/' + stringifyInstruction(this._navigationInstruction);
+    this.visibleHref = this._location.prepareExternalUrl(navigationHref);
-    this.visibleHref = this._location.normalizeAbsolutely(navigationHref);
   }
 
   onClick(): boolean {

modules/angular2/test/router/integration/router_integration_spec.ts

@@ -163,7 +163,8 @@ export function main() {
                });
          }));
 
-      describe('custom app base ref', () => {
+      // TODO(btford): mock out level lower than LocationStrategy once that level exists
+      xdescribe('custom app base ref', () => {
         beforeEachBindings(() => { return [provide(APP_BASE_HREF, {useValue: '/my/app'})]; });
         it('should bootstrap',
            inject([AsyncTestCompleter, TestComponentBuilder],

modules/angular2/test/router/location_spec.ts

@@ -33,17 +33,12 @@ export function main() {
 
     beforeEach(makeLocation);
 
-    it('should normalize relative urls on navigate', () => {
-      location.go('user/btford');
-      expect(locationStrategy.path()).toEqual('/my/app/user/btford');
-    });
-
     it('should not prepend urls with starting slash when an empty URL is provided',
-       () => { expect(location.normalizeAbsolutely('')).toEqual(locationStrategy.getBaseHref()); });
+       () => { expect(location.prepareExternalUrl('')).toEqual(locationStrategy.getBaseHref()); });
 
     it('should not prepend path with an extra slash when a baseHref has a trailing slash', () => {
       let location = makeLocation('/my/slashed/app/');
-      expect(location.normalizeAbsolutely('/page')).toEqual('/my/slashed/app/page');
+      expect(location.prepareExternalUrl('/page')).toEqual('/my/slashed/app/page');
     });
 
     it('should not append urls with leading slash on navigate', () => {
@@ -51,12 +46,6 @@ export function main() {
       expect(locationStrategy.path()).toEqual('/my/app/user/btford');
     });
 
-    it('should remove index.html from base href', () => {
-      let location = makeLocation('/my/app/index.html');
-      location.go('user/btford');
-      expect(locationStrategy.path()).toEqual('/my/app/user/btford');
-    });
-
     it('should normalize urls on popstate', inject([AsyncTestCompleter], (async) => {
          locationStrategy.simulatePopState('/my/app/user/btford');
          location.subscribe((ev) => {
@@ -65,17 +54,6 @@ export function main() {
          })
        }));
 
-    it('should normalize location path', () => {
-      locationStrategy.internalPath = '/my/app/user/btford';
-      expect(location.path()).toEqual('/user/btford');
-    });
-
-    it('should use optional base href param', () => {
-      let location = makeLocation('/', provide(APP_BASE_HREF, {useValue: '/my/custom/href'}));
-      location.go('user/btford');
-      expect(locationStrategy.path()).toEqual('/my/custom/href/user/btford');
-    });
-
     it('should throw when no base href is provided', () => {
       var locationStrategy = new MockLocationStrategy();
       locationStrategy.internalBaseHref = null;

modules/angular2/test/router/router_link_spec.ts

@@ -53,7 +53,7 @@ export function main() {
              .then((testComponent) => {
                testComponent.detectChanges();
                let anchorElement = testComponent.debugElement.query(By.css('a')).nativeElement;
-               expect(DOM.getAttribute(anchorElement, 'href')).toEqual('/detail');
+               expect(DOM.getAttribute(anchorElement, 'href')).toEqual('detail');
                async.done();
              });
        }));
@@ -99,7 +99,7 @@ class TestComponent {
 
 function makeDummyLocation() {
   var dl = new SpyLocation();
-  dl.spy('normalizeAbsolutely').andCallFake((url) => url);
+  dl.spy('prepareExternalUrl').andCallFake((url) => url);
   return dl;
 }
 

modules/playground/e2e_test/routing/routing_spec.ts

@@ -23,7 +23,7 @@ describe('routing inbox-app', () => {
     it('should build a link which points to the detail page', () => {
       browser.get(URL);
       waitForElement('#item-15');
-      expect(element(by.css('#item-15')).getAttribute('href')).toMatch(/\/detail\/15$/);
+      expect(element(by.css('#item-15')).getAttribute('href')).toMatch(/#\/detail\/15$/);
       element(by.css('#item-15')).click();
       waitForElement('#record-id');
       expect(browser.getCurrentUrl()).toMatch(/\/detail\/15$/);
@@ -47,7 +47,7 @@ describe('routing inbox-app', () => {
       element(by.linkText('Drafts')).click();
       waitForElement('.inbox-item-record');
       expect(element.all(by.css('.inbox-item-record')).count()).toEqual(2);
-      expect(element(by.css('#item-201')).getAttribute('href')).toMatch(/\/detail\/201$/);
+      expect(element(by.css('#item-201')).getAttribute('href')).toMatch(/#\/detail\/201$/);
       element(by.css('#item-201')).click();
       waitForElement('#record-id');
       expect(browser.getCurrentUrl()).toMatch(/\/detail\/201$/);