From 9c1318d7315027ebd67d112bf827a08a64ca7e26 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 21 Apr 2017 23:35:40 +0300 Subject: [PATCH] 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 | 2 +- aio/src/app/documents/document.service.ts | 2 +- .../current-location.component.spec.ts | 20 +- .../embedded/current-location.component.ts | 2 +- .../app/navigation/navigation.service.spec.ts | 3 - aio/src/app/navigation/navigation.service.ts | 13 +- aio/src/app/shared/location.service.spec.ts | 254 ++++++++++++++---- aio/src/app/shared/location.service.ts | 26 +- aio/src/testing/location.service.ts | 3 +- 9 files changed, 228 insertions(+), 97 deletions(-) diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index eda9502d5b..3d4237c9c3 100644 --- a/aio/src/app/app.component.ts +++ b/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; diff --git a/aio/src/app/documents/document.service.ts b/aio/src/app/documents/document.service.ts index 89ad9871d1..b4e520a8ef 100644 --- a/aio/src/app/documents/document.service.ts +++ b/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) { diff --git a/aio/src/app/embedded/current-location.component.spec.ts b/aio/src/app/embedded/current-location.component.spec.ts index ee0ccbcd0b..0e79ef3dbb 100644 --- a/aio/src/app/embedded/current-location.component.spec.ts +++ b/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'); }); }); diff --git a/aio/src/app/embedded/current-location.component.ts b/aio/src/app/embedded/current-location.component.ts index 0760ea1a21..57f4280faa 100644 --- a/aio/src/app/embedded/current-location.component.ts +++ b/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) { diff --git a/aio/src/app/navigation/navigation.service.spec.ts b/aio/src/app/navigation/navigation.service.spec.ts index 9f48a9aa4c..565bf02aa3 100644 --- a/aio/src/app/navigation/navigation.service.spec.ts +++ b/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'); diff --git a/aio/src/app/navigation/navigation.service.ts b/aio/src/app/navigation/navigation.service.ts index d80c233473..ec98cd0909 100644 --- a/aio/src/app/navigation/navigation.service.ts +++ b/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): Observable { const currentNode = combineLatest( navigationViews.map(this.computeUrlToNavNodesMap), - this.location.currentUrl, + this.location.currentPath, (navMap, url) => { - let urlKey = cleanUrl(url); - urlKey = urlKey.startsWith('api/') ? 'api' : urlKey; + const urlKey = url.startsWith('api/') ? 'api' : url; return navMap[urlKey] || { view: '', url: urlKey, nodes: [] }; }) .publishReplay(1); diff --git a/aio/src/app/shared/location.service.spec.ts b/aio/src/app/shared/location.service.spec.ts index 68629b440b..1c32343391 100644 --- a/aio/src/app/shared/location.service.spec.ts +++ b/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 service: LocationService = injector.get(LocationService); + const location: MockLocationStrategy = injector.get(LocationStrategy); + const service: LocationService = injector.get(LocationService); - location.simulatePopState('/initial-url1'); - location.simulatePopState('/initial-url2'); - location.simulatePopState('/initial-url3'); + location.simulatePopState('/initial-url1'); + location.simulatePopState('/initial-url2'); + location.simulatePopState('/initial-url3'); - const urls1 = []; - service.currentUrl.subscribe(url => urls1.push(url)); + const urls1 = []; + service.currentUrl.subscribe(url => urls1.push(url)); - location.simulatePopState('/next-url1'); - location.simulatePopState('/next-url2'); + location.simulatePopState('/next-url1'); + location.simulatePopState('/next-url2'); - const urls2 = []; - service.currentUrl.subscribe(url => urls2.push(url)); + const urls2 = []; + service.currentUrl.subscribe(url => urls2.push(url)); - location.simulatePopState('/next-url3'); + location.simulatePopState('/next-url3'); - expect(urls1).toEqual([ - 'initial-url3', - 'next-url1', - 'next-url2', - 'next-url3' - ]); + expect(urls1).toEqual([ + 'initial-url3', + 'next-url1', + 'next-url2', + 'next-url3' + ]); - expect(urls2).toEqual([ - 'next-url2', - 'next-url3' - ]); + expect(urls2).toEqual([ + 'next-url2', + '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 ask Location for the current path without the hash', () => { + it('should strip leading and trailing slashes', () => { 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` - // does not remove the hash from the path, even if we do pass `false`. + service.currentUrl.subscribe(u => url = u); + service.go('/some/url/'); - spyOn(location, 'path').and.callThrough(); - service.path(); - expect(location.path).toHaveBeenCalledWith(false); - }); - - 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'); + expect(location.internalPath).toEqual('some/url'); + expect(location.path(true)).toEqual('some/url'); + expect(url).toBe('some/url'); }); }); @@ -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); }); }); diff --git a/aio/src/app/shared/location.service.ts b/aio/src/app/shared/location.service.ts index 3919106525..8e41d54a4e 100644 --- a/aio/src/app/shared/location.service.ts +++ b/aio/src/app/shared/location.service.ts @@ -14,8 +14,11 @@ export class LocationService { private readonly urlParser = document.createElement('a'); private urlSubject = new Subject(); 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(initialUrl); + this.urlSubject.next(location.path(true)); this.location.subscribe(state => { - const url = this.stripLeadingSlashes(state.url); - return this.urlSubject.next(url); + return this.urlSubject.next(state.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) { - return url.replace(/^\/+/, ''); - } - - /** - * 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; + private stripSlashes(url: string) { + return url.replace(/^\/+/, '').replace(/\/+(\?|#|$)/, '$1'); } search(): { [index: string]: string; } { @@ -122,7 +114,7 @@ export class LocationService { return true; } - this.go(this.stripLeadingSlashes(relativeUrl)); + this.go(relativeUrl); return false; } } diff --git a/aio/src/testing/location.service.ts b/aio/src/testing/location.service.ts index c0ef6a6551..ae1df87038 100644 --- a/aio/src/testing/location.service.ts +++ b/aio/src/testing/location.service.ts @@ -3,12 +3,11 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject'; export class MockLocationService { urlSubject = new BehaviorSubject(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