From e951612af2ea4bfe03cd1957decb316f507900fe Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 18 Apr 2017 16:18:18 +0100 Subject: [PATCH] fix(aio): set the pageId to the file-not-found URL if the doc is not available Previously, the `AppComponent.pageId` was set via the current URL, rather than the document being displayed. This is only really noticeable when the URL does not match a valid doc and we are actually displaying a 404 page. Now we compute the `pageId` from the URL of the document being viewed, which is returned from the `DocumentService.currentDocument` observable instead. --- aio/src/app/app.component.ts | 7 ++++- aio/src/app/documents/document-contents.ts | 1 + .../app/documents/document.service.spec.ts | 25 +++++++++------- aio/src/app/documents/document.service.ts | 30 +++++++++---------- .../doc-viewer/doc-viewer.component.spec.ts | 24 +++++++-------- 5 files changed, 49 insertions(+), 38 deletions(-) diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index 3515c2b65d..4ae7718f21 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -71,6 +71,12 @@ export class AppComponent implements OnInit { this.documentService.currentDocument.subscribe(doc => { this.currentDocument = doc; this.setDocumentTitle(doc.title); + this.pageId = this.currentDocument.url.replace('/', '-'); + + // Special case the home page + if (this.pageId === 'index') { + this.pageId = 'home'; + } }); // scroll even if only the hash fragment changed @@ -78,7 +84,6 @@ export class AppComponent implements OnInit { this.navigationService.currentNode.subscribe(currentNode => { this.currentNode = currentNode; - this.pageId = this.currentNode.url.replace('/', '-') || 'home'; // Toggle the sidenav if side-by-side and the kind of view changed if (this.previousNavView === currentNode.view) { return; } diff --git a/aio/src/app/documents/document-contents.ts b/aio/src/app/documents/document-contents.ts index 80b03a4f2e..53471eca07 100644 --- a/aio/src/app/documents/document-contents.ts +++ b/aio/src/app/documents/document-contents.ts @@ -1,4 +1,5 @@ export interface DocumentContents { + url: string; title: string; contents: string; } diff --git a/aio/src/app/documents/document.service.spec.ts b/aio/src/app/documents/document.service.spec.ts index 94859bc8bb..a7702b6d34 100644 --- a/aio/src/app/documents/document.service.spec.ts +++ b/aio/src/app/documents/document.service.spec.ts @@ -61,8 +61,8 @@ describe('DocumentService', () => { it('should emit a document each time the location changes', () => { let latestDocument: DocumentContents; - const doc0 = { title: 'doc 0' }; - const doc1 = { title: 'doc 1' }; + const doc0 = { title: 'doc 0', url: 'initial/url' }; + const doc1 = { title: 'doc 1', url: 'new/url' }; const { docService, backend, locationService } = getServices('initial/url'); const connections = backend.connectionsArray; @@ -80,17 +80,22 @@ describe('DocumentService', () => { it('should emit the not-found document if the document is not found on the server', () => { const { docService, backend } = getServices('missing/url'); const connections = backend.connectionsArray; - docService.currentDocument.subscribe(); + let currentDocument: DocumentContents; + docService.currentDocument.subscribe(doc => currentDocument = doc); connections[0].mockError(new Response(new ResponseOptions({ status: 404, statusText: 'NOT FOUND'})) as any); expect(connections.length).toEqual(2); expect(connections[1].request.url).toEqual(CONTENT_URL_PREFIX + 'file-not-found.json'); + const fileNotFoundDoc = { title: 'File Not Found' }; + connections[1].mockRespond(createResponse(fileNotFoundDoc)); + expect(currentDocument).toEqual(jasmine.objectContaining(fileNotFoundDoc)); + expect(currentDocument.url).toEqual('file-not-found'); }); it('should emit a hard-coded not-found document if the not-found document is not found on the server', () => { let currentDocument: DocumentContents; - const notFoundDoc = { title: 'Not Found', contents: 'Document not found' }; - const nextDoc = { title: 'Next Doc' }; + const notFoundDoc: DocumentContents = { title: 'Not Found', contents: 'Document not found', url: 'file-not-found' }; + const nextDoc = { title: 'Next Doc', url: 'new/url' }; const { docService, backend, locationService } = getServices('file-not-found'); const connections = backend.connectionsArray; docService.currentDocument.subscribe(doc => currentDocument = doc); @@ -118,7 +123,7 @@ describe('DocumentService', () => { const doc1 = { title: 'doc 1' }; locationService.go('new/url'); connections[1].mockRespond(createResponse(doc1)); - expect(latestDocument).toEqual(doc1); + expect(latestDocument).toEqual(jasmine.objectContaining(doc1)); }); it('should not make a request to the server if the doc is in the cache already', () => { @@ -133,7 +138,7 @@ describe('DocumentService', () => { subscription = docService.currentDocument.subscribe(doc => latestDocument = doc); expect(connections.length).toEqual(1); connections[0].mockRespond(createResponse(doc0)); - expect(latestDocument).toEqual(doc0); + expect(latestDocument).toEqual(jasmine.objectContaining(doc0)); subscription.unsubscribe(); // modify the response so we can check that future subscriptions do not trigger another request @@ -143,7 +148,7 @@ describe('DocumentService', () => { locationService.go('url/1'); expect(connections.length).toEqual(2); connections[1].mockRespond(createResponse(doc1)); - expect(latestDocument).toEqual(doc1); + expect(latestDocument).toEqual(jasmine.objectContaining(doc1)); subscription.unsubscribe(); // modify the response so we can check that future subscriptions do not trigger another request @@ -152,13 +157,13 @@ describe('DocumentService', () => { subscription = docService.currentDocument.subscribe(doc => latestDocument = doc); locationService.go('url/0'); expect(connections.length).toEqual(2); - expect(latestDocument).toEqual(doc0); + expect(latestDocument).toEqual(jasmine.objectContaining(doc0)); subscription.unsubscribe(); subscription = docService.currentDocument.subscribe(doc => latestDocument = doc); locationService.go('url/1'); expect(connections.length).toEqual(2); - expect(latestDocument).toEqual(doc1); + expect(latestDocument).toEqual(jasmine.objectContaining(doc1)); subscription.unsubscribe(); }); }); diff --git a/aio/src/app/documents/document.service.ts b/aio/src/app/documents/document.service.ts index 949d60c1f0..3b6882cb9d 100644 --- a/aio/src/app/documents/document.service.ts +++ b/aio/src/app/documents/document.service.ts @@ -20,7 +20,6 @@ const FILE_NOT_FOUND_URL = 'file-not-found'; export class DocumentService { private cache = new Map>(); - private fileNotFoundPath = this.computePath(FILE_NOT_FOUND_URL); currentDocument: Observable; @@ -34,44 +33,45 @@ export class DocumentService { private getDocument(url: string) { this.logger.log('getting document', url); - const path = this.computePath(url); - if ( !this.cache.has(path)) { - this.cache.set(path, this.fetchDocument(path)); + url = this.cleanUrl(url); + if ( !this.cache.has(url)) { + this.cache.set(url, this.fetchDocument(url)); } - return this.cache.get(path); + return this.cache.get(url); } - private fetchDocument(path: string) { - this.logger.log('fetching document from', path); + private fetchDocument(url: string) { + this.logger.log('fetching document from', url); const subject = new AsyncSubject(); this.http - .get(path) - .map(res => res.json()) + .get(`content/docs/${url}.json`) + // Add the document's url to the DocumentContents provided to the rest of the app + .map(res => Object.assign(res.json(), { url }) as DocumentContents) .catch((error: Response) => { if (error.status === 404) { - if (path !== this.fileNotFoundPath) { - this.logger.error(`Document file not found at '${path}'`); + if (url !== FILE_NOT_FOUND_URL) { + this.logger.error(`Document file not found at '${url}'`); // using `getDocument` means that we can fetch the 404 doc contents from the server and cache it return this.getDocument(FILE_NOT_FOUND_URL); } else { - return of({ title: 'Not Found', contents: 'Document not found' }); + return of({ title: 'Not Found', contents: 'Document not found', url: FILE_NOT_FOUND_URL }); } } else { this.logger.error('Error fetching document', error); - return Observable.of({ title: 'Error fetching document', contents: 'Sorry we were not able to fetch that document.' }); + return Observable.of({ title: 'Error fetching document', contents: 'Sorry we were not able to fetch that document.', url}); } }) .subscribe(subject); return subject.asObservable(); } - private computePath(url: string) { + private cleanUrl(url: string) { url = url.match(/[^#?]*/)[0]; // strip off fragment and query url = url.replace(/\/$/, ''); // strip off trailing slash if (url === '') { // deal with root url url = 'index'; } - return 'content/docs/' + url + '.json'; + return url; } } diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts index 35ee26608c..5111f5ee15 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts @@ -122,23 +122,23 @@ describe('DocViewerComponent', () => { }); it(('should display nothing when set currentDoc has no content'), () => { - component.currentDoc = { title: 'fake title', contents: '' }; + component.currentDoc = { title: 'fake title', contents: '', url: 'a/b' }; fixture.detectChanges(); expect(docViewerEl.innerHTML).toBe(''); }); it(('should display simple static content doc'), () => { const contents = '

Howdy, doc viewer

'; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; fixture.detectChanges(); expect(docViewerEl.innerHTML).toEqual(contents); }); it(('should display nothing after reset static content doc'), () => { const contents = '

Howdy, doc viewer

'; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; fixture.detectChanges(); - component.currentDoc = { title: 'fake title', contents: '' }; + component.currentDoc = { title: 'fake title', contents: '', url: 'a/c' }; fixture.detectChanges(); expect(docViewerEl.innerHTML).toEqual(''); }); @@ -149,7 +149,7 @@ describe('DocViewerComponent', () => {

Below Foo

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; fixture.detectChanges(); const fooHtml = docViewerEl.querySelector('aio-foo').innerHTML; expect(fooHtml).toContain('Foo Component'); @@ -165,7 +165,7 @@ describe('DocViewerComponent', () => {

Below Foo

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; fixture.detectChanges(); const foos = docViewerEl.querySelectorAll('aio-foo'); expect(foos.length).toBe(2); @@ -177,7 +177,7 @@ describe('DocViewerComponent', () => {

Below Bar

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; fixture.detectChanges(); const barHtml = docViewerEl.querySelector('aio-bar').innerHTML; expect(barHtml).toContain('Bar Component'); @@ -189,7 +189,7 @@ describe('DocViewerComponent', () => { ###bar content###

Below Bar

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; // necessary to trigger projection within ngOnInit fixture.detectChanges(); @@ -207,7 +207,7 @@ describe('DocViewerComponent', () => {

Bottom

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; // necessary to trigger Bar's projection within ngOnInit fixture.detectChanges(); @@ -230,7 +230,7 @@ describe('DocViewerComponent', () => {

Bottom

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; // necessary to trigger Bar's projection within ngOnInit fixture.detectChanges(); @@ -254,7 +254,7 @@ describe('DocViewerComponent', () => {

Bottom

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; // necessary to trigger Bar's projection within ngOnInit fixture.detectChanges(); @@ -282,7 +282,7 @@ describe('DocViewerComponent', () => {

---More baz--

Bottom

`; - component.currentDoc = { title: 'fake title', contents }; + component.currentDoc = { title: 'fake title', contents, url: 'a/b' }; // necessary to trigger Bar's projection within ngOnInit fixture.detectChanges();