fix(aio): strip leading slashes from path (and improve DRY-ness) (#16238)

Previously, the path returned by `LocationService.path()` preserved leading
slashes, which resulted in requests with consequtive slashes in the URL. Such
requests would fail (with a 404) on staging.

This commit fixes it, by removing leading slashes from the path. It also
refactors `LocationService` a bit, converting path to an observable,
`currentPath` (similar to `currentUrl`), and applies certain clean-ups (e.g.
stripping slashes, query, hash) in one place, which simplifies consumption.

Closes #16230

aio/src/app/app.component.ts

@@ -76,7 +76,7 @@ export class AppComponent implements OnInit {
     });
 
     // scroll even if only the hash fragment changed
-    this.locationService.currentUrl.subscribe(url => this.autoScroll());
+    this.locationService.currentUrl.subscribe(() => this.autoScroll());
 
     this.navigationService.currentNode.subscribe(currentNode => {
       this.currentNode = currentNode;

aio/src/app/documents/document.service.ts

@@ -39,7 +39,7 @@ export class DocumentService {
     private http: Http,
     location: LocationService) {
     // Whenever the URL changes we try to get the appropriate doc
-    this.currentDocument = location.currentUrl.switchMap(() => this.getDocument(location.path()));
+    this.currentDocument = location.currentPath.switchMap(path => this.getDocument(path));
   }
 
   private getDocument(url: string) {

aio/src/app/embedded/current-location.component.spec.ts

@@ -1,18 +1,19 @@
 import { async, ComponentFixture, TestBed } from '@angular/core/testing';
 import { LocationService } from 'app/shared/location.service';
+import { MockLocationService } from 'testing/location.service';
 import { CurrentLocationComponent } from './current-location.component';
 
-let currentPath: string;
-class MockLocation {
-  path() { return currentPath; }
-}
 
 describe('CurrentLocationComponent', () => {
+  let locationService: MockLocationService;
+
   beforeEach(async(() => {
+    locationService = new MockLocationService('initial/url');
+
     TestBed.configureTestingModule({
       declarations: [ CurrentLocationComponent ],
       providers: [
-        { provide: LocationService, useClass: MockLocation }
+        { provide: LocationService, useValue: locationService }
       ]
     });
     TestBed.compileComponents();
@@ -21,8 +22,13 @@ describe('CurrentLocationComponent', () => {
   it('should render the current location', () => {
     const fixture = TestBed.createComponent(CurrentLocationComponent);
     const element: HTMLElement = fixture.nativeElement;
-    currentPath = 'a/b/c';
+
     fixture.detectChanges();
-    expect(element.innerText).toEqual('a/b/c');
+    expect(element.innerText).toEqual('initial/url');
+
+    locationService.urlSubject.next('next/url');
+
+    fixture.detectChanges();
+    expect(element.innerText).toEqual('next/url');
   });
 });

aio/src/app/embedded/current-location.component.ts

@@ -7,7 +7,7 @@ import { LocationService } from 'app/shared/location.service';
  */
 @Component({
   selector: 'current-location',
-  template: '{{location.path()}}'
+  template: '{{ location.currentPath | async }}'
 })
 export class CurrentLocationComponent {
   constructor(public location: LocationService) {

aio/src/app/navigation/navigation.service.spec.ts

@@ -178,9 +178,6 @@ describe('NavigationService', () => {
       locationService.go('c');
       expect(currentNode).toEqual(cnode, 'location: c');
 
-      locationService.go('c/');
-      expect(currentNode).toEqual(cnode, 'location: c/');
-
       locationService.go('c#foo');
       expect(currentNode).toEqual(cnode, 'location: c#foo');
 

aio/src/app/navigation/navigation.service.ts

@@ -16,14 +16,6 @@ export { CurrentNode, NavigationNode, NavigationResponse, NavigationViews, Versi
 
 const navigationPath = 'content/navigation.json';
 
-const urlParser = document.createElement('a');
-function cleanUrl(url: string) {
-  // remove hash (#) and query params (?)
-  urlParser.href = '/' + url;
-  // strip leading and trailing slashes
-  return urlParser.pathname.replace(/^\/+|\/$/g, '');
-}
-
 @Injectable()
 export class NavigationService {
   /**
@@ -91,10 +83,9 @@ export class NavigationService {
   private getCurrentNode(navigationViews: Observable<NavigationViews>): Observable<CurrentNode> {
     const currentNode = combineLatest(
       navigationViews.map(this.computeUrlToNavNodesMap),
-      this.location.currentUrl,
+      this.location.currentPath,
       (navMap, url) => {
-        let urlKey = cleanUrl(url);
+        const urlKey = url.startsWith('api/') ? 'api' : url;
-        urlKey = urlKey.startsWith('api/') ? 'api' : urlKey;
         return navMap[urlKey] || { view: '', url: urlKey, nodes: [] };
       })
       .publishReplay(1);

aio/src/app/shared/location.service.spec.ts

@@ -19,7 +19,7 @@ describe('LocationService', () => {
     ]);
   });
 
-  describe('urlStream', () => {
+  describe('currentUrl', () => {
     it('should emit the latest url at the time it is subscribed to', () => {
 
       const location: MockLocationStrategy = injector.get(LocationStrategy);
@@ -63,35 +63,199 @@ describe('LocationService', () => {
     });
 
     it('should pass only the latest and later urls to each subscriber', () => {
-        const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
-        const service: LocationService = injector.get(LocationService);
+      const service: LocationService = injector.get(LocationService);
 
-        location.simulatePopState('/initial-url1');
+      location.simulatePopState('/initial-url1');
-        location.simulatePopState('/initial-url2');
+      location.simulatePopState('/initial-url2');
-        location.simulatePopState('/initial-url3');
+      location.simulatePopState('/initial-url3');
 
-        const urls1 = [];
+      const urls1 = [];
-        service.currentUrl.subscribe(url => urls1.push(url));
+      service.currentUrl.subscribe(url => urls1.push(url));
 
-        location.simulatePopState('/next-url1');
+      location.simulatePopState('/next-url1');
-        location.simulatePopState('/next-url2');
+      location.simulatePopState('/next-url2');
 
-        const urls2 = [];
+      const urls2 = [];
-        service.currentUrl.subscribe(url => urls2.push(url));
+      service.currentUrl.subscribe(url => urls2.push(url));
 
-        location.simulatePopState('/next-url3');
+      location.simulatePopState('/next-url3');
 
-        expect(urls1).toEqual([
+      expect(urls1).toEqual([
-          'initial-url3',
+        'initial-url3',
-          'next-url1',
+        'next-url1',
-          'next-url2',
+        'next-url2',
-          'next-url3'
+        'next-url3'
-        ]);
+      ]);
 
-        expect(urls2).toEqual([
+      expect(urls2).toEqual([
-          'next-url2',
+        'next-url2',
-          'next-url3'
+        'next-url3'
-        ]);
+      ]);
+    });
+
+    it('should strip leading and trailing slashes', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+      const urls: string[] = [];
+
+      service.currentUrl.subscribe(u => urls.push(u));
+
+      location.simulatePopState('///some/url1///');
+      location.simulatePopState('///some/url2///?foo=bar');
+      location.simulatePopState('///some/url3///#baz');
+      location.simulatePopState('///some/url4///?foo=bar#baz');
+
+      expect(urls.slice(-4)).toEqual([
+        'some/url1',
+        'some/url2?foo=bar',
+        'some/url3#baz',
+        'some/url4?foo=bar#baz'
+      ]);
+    });
+  });
+
+  describe('currentPath', () => {
+    it('should strip leading and trailing slashes off the url', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+      const paths: string[] = [];
+
+      service.currentPath.subscribe(p => paths.push(p));
+
+      location.simulatePopState('///initial/url1///');
+      location.simulatePopState('///initial/url2///?foo=bar');
+      location.simulatePopState('///initial/url3///#baz');
+      location.simulatePopState('///initial/url4///?foo=bar#baz');
+
+      expect(paths.slice(-4)).toEqual([
+        'initial/url1',
+        'initial/url2',
+        'initial/url3',
+        'initial/url4'
+      ]);
+    });
+
+    it('should not strip other slashes off the url', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+      const paths: string[] = [];
+
+      service.currentPath.subscribe(p => paths.push(p));
+
+      location.simulatePopState('initial///url1');
+      location.simulatePopState('initial///url2?foo=bar');
+      location.simulatePopState('initial///url3#baz');
+      location.simulatePopState('initial///url4?foo=bar#baz');
+
+      expect(paths.slice(-4)).toEqual([
+        'initial///url1',
+        'initial///url2',
+        'initial///url3',
+        'initial///url4'
+      ]);
+    });
+
+    it('should strip the query off the url', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+      let path: string;
+
+      service.currentPath.subscribe(p => path = p);
+
+      location.simulatePopState('/initial/url1?foo=bar');
+
+      expect(path).toBe('initial/url1');
+    });
+
+    it('should strip the hash fragment off the url', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+      const paths: string[] = [];
+
+      service.currentPath.subscribe(p => paths.push(p));
+
+      location.simulatePopState('/initial/url1#foo');
+      location.simulatePopState('/initial/url2?foo=bar#baz');
+
+      expect(paths.slice(-2)).toEqual([
+        'initial/url1',
+        'initial/url2'
+      ]);
+    });
+
+    it('should emit the latest path at the time it is subscribed to', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+
+      location.simulatePopState('/initial/url1');
+      location.simulatePopState('/initial/url2');
+      location.simulatePopState('/initial/url3');
+
+      const service: LocationService = injector.get(LocationService);
+
+      location.simulatePopState('/next/url1');
+      location.simulatePopState('/next/url2');
+      location.simulatePopState('/next/url3');
+
+      let initialPath: string;
+      service.currentPath.subscribe(path => initialPath = path);
+
+      expect(initialPath).toEqual('next/url3');
+    });
+
+    it('should emit all location changes after it has been subscribed to', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+
+      location.simulatePopState('/initial/url1');
+      location.simulatePopState('/initial/url2');
+      location.simulatePopState('/initial/url3');
+
+      const paths: string[] = [];
+      service.currentPath.subscribe(path => paths.push(path));
+
+      location.simulatePopState('/next/url1');
+      location.simulatePopState('/next/url2');
+      location.simulatePopState('/next/url3');
+
+      expect(paths).toEqual([
+        'initial/url3',
+        'next/url1',
+        'next/url2',
+        'next/url3'
+      ]);
+    });
+
+    it('should pass only the latest and later paths to each subscriber', () => {
+      const location: MockLocationStrategy = injector.get(LocationStrategy);
+      const service: LocationService = injector.get(LocationService);
+
+      location.simulatePopState('/initial/url1');
+      location.simulatePopState('/initial/url2');
+      location.simulatePopState('/initial/url3');
+
+      const paths1: string[] = [];
+      service.currentPath.subscribe(path => paths1.push(path));
+
+      location.simulatePopState('/next/url1');
+      location.simulatePopState('/next/url2');
+
+      const paths2: string[] = [];
+      service.currentPath.subscribe(path => paths2.push(path));
+
+      location.simulatePopState('/next/url3');
+
+      expect(paths1).toEqual([
+        'initial/url3',
+        'next/url1',
+        'next/url2',
+        'next/url3'
+      ]);
+
+      expect(paths2).toEqual([
+        'next/url2',
+        'next/url3'
+      ]);
     });
   });
 
@@ -122,36 +286,18 @@ describe('LocationService', () => {
         'some-new-url'
       ]);
     });
-  });
 
-  describe('path', () => {
+    it('should strip leading and trailing slashes', () => {
-    it('should ask Location for the current path without the hash', () => {
       const location: MockLocationStrategy = injector.get(LocationStrategy);
       const service: LocationService = injector.get(LocationService);
+      let url: string;
 
-      // We cannot test this directly in the following test because the `MockLocationStrategy`
+      service.currentUrl.subscribe(u => url = u);
-      // does not remove the hash from the path, even if we do pass `false`.
+      service.go('/some/url/');
 
-      spyOn(location, 'path').and.callThrough();
+      expect(location.internalPath).toEqual('some/url');
-      service.path();
+      expect(location.path(true)).toEqual('some/url');
-      expect(location.path).toHaveBeenCalledWith(false);
+      expect(url).toBe('some/url');
-    });
-
-    it('should return the current location.path, with the query and trailing slash stripped off', () => {
-      const location: MockLocationStrategy = injector.get(LocationStrategy);
-      const service: LocationService = injector.get(LocationService);
-
-      location.simulatePopState('a/b/c?foo=bar&moo=car');
-      expect(service.path()).toEqual('a/b/c');
-
-      location.simulatePopState('c/d/e');
-      expect(service.path()).toEqual('c/d/e');
-
-      location.simulatePopState('a/b/c/?foo=bar&moo=car');
-      expect(service.path()).toEqual('a/b/c');
-
-      location.simulatePopState('c/d/e/');
-      expect(service.path()).toEqual('c/d/e');
     });
   });
 
@@ -263,7 +409,7 @@ describe('LocationService', () => {
         anchor.href = 'some/local/url';
         spyOn(service, 'go');
         const result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url');
         expect(result).toBe(false);
       });
 
@@ -271,7 +417,7 @@ describe('LocationService', () => {
         anchor.href = '/some/local/url';
         spyOn(service, 'go');
         const result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url');
         expect(result).toBe(false);
       });
 
@@ -279,7 +425,7 @@ describe('LocationService', () => {
         anchor.href = 'some/local/url?query=xxx&other=yyy';
         spyOn(service, 'go');
         const result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url?query=xxx&other=yyy');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url?query=xxx&other=yyy');
         expect(result).toBe(false);
       });
 
@@ -287,7 +433,7 @@ describe('LocationService', () => {
         anchor.href = 'some/local/url#somefragment';
         spyOn(service, 'go');
         const result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url#somefragment');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url#somefragment');
         expect(result).toBe(false);
       });
 
@@ -295,7 +441,7 @@ describe('LocationService', () => {
         anchor.href = 'some/local/url?query=xxx&other=yyy#somefragment';
         spyOn(service, 'go');
         const result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url?query=xxx&other=yyy#somefragment');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url?query=xxx&other=yyy#somefragment');
         expect(result).toBe(false);
       });
     });
@@ -365,7 +511,7 @@ describe('LocationService', () => {
 
         anchor.target = '_self';
         result = service.handleAnchorClick(anchor, 0, false, false);
-        expect(service.go).toHaveBeenCalledWith('some/local/url');
+        expect(service.go).toHaveBeenCalledWith('/some/local/url');
         expect(result).toBe(false);
       });
     });

aio/src/app/shared/location.service.ts

@@ -14,8 +14,11 @@ export class LocationService {
   private readonly urlParser = document.createElement('a');
   private urlSubject = new Subject<string>();
   currentUrl = this.urlSubject
+    .map(url => this.stripSlashes(url))
     .do(url => this.gaService.locationChanged(url))
     .publishReplay(1);
+  currentPath = this.currentUrl
+    .map(url => url.match(/[^?#]*/)[0]);   // strip off query and hash
 
   constructor(
     private gaService: GaService,
@@ -23,33 +26,22 @@ export class LocationService {
     private platformLocation: PlatformLocation) {
 
     this.currentUrl.connect();
-    const initialUrl = this.stripLeadingSlashes(location.path(true));
+    this.urlSubject.next(location.path(true));
-    this.urlSubject.next(initialUrl);
 
     this.location.subscribe(state => {
-      const url = this.stripLeadingSlashes(state.url);
+      return this.urlSubject.next(state.url);
-      return this.urlSubject.next(url);
     });
   }
 
   // TODO?: ignore if url-without-hash-or-search matches current location?
   go(url: string) {
+    url = this.stripSlashes(url);
     this.location.go(url);
     this.urlSubject.next(url);
   }
 
-  private stripLeadingSlashes(url: string) {
+  private stripSlashes(url: string) {
-    return url.replace(/^\/+/, '');
+    return url.replace(/^\/+/, '').replace(/\/+(\?|#|$)/, '$1');
-  }
-
-  /**
-   * Get the current path, without trailing slash, hash fragment or query params
-   */
-  path(): string {
-    let path = this.location.path(false);
-    path = path.match(/[^?]*/)[0]; // strip off query
-    path = path.replace(/\/$/, ''); // strip off trailing slash
-    return path;
   }
 
   search(): { [index: string]: string; } {
@@ -122,7 +114,7 @@ export class LocationService {
       return true;
     }
 
-    this.go(this.stripLeadingSlashes(relativeUrl));
+    this.go(relativeUrl);
     return false;
   }
 }

aio/src/testing/location.service.ts

@@ -3,12 +3,11 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject';
 export class MockLocationService {
   urlSubject = new BehaviorSubject<string>(this.initialUrl);
   currentUrl = this.urlSubject.asObservable();
+  currentPath = this.currentUrl.map(url => url.match(/[^?#]*/)[0]);
   search = jasmine.createSpy('search').and.returnValue({});
   setSearch = jasmine.createSpy('setSearch');
   go = jasmine.createSpy('Location.go').and
               .callFake((url: string) => this.urlSubject.next(url));
-  path = jasmine.createSpy('Location.path').and
-              .callFake(() => this.urlSubject.getValue().split('?')[0]);
   handleAnchorClick = jasmine.createSpy('Location.handleAnchorClick')
       .and.returnValue(false); // prevent click from causing a browser navigation