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

@@ -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; }

aio/src/app/documents/document-contents.ts

@@ -1,4 +1,5 @@
 export interface DocumentContents {
+  url: string;
   title: string;
   contents: string;
 }

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 doc0 = { title: 'doc 0', url: 'initial/url' };
-      const doc1 = { title: 'doc 1' };
+      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 notFoundDoc: DocumentContents = { title: 'Not Found', contents: 'Document not found', url: 'file-not-found' };
-      const nextDoc = { title: 'Next Doc' };
+      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();
     });
   });

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<string, Observable<DocumentContents>>();
-  private fileNotFoundPath = this.computePath(FILE_NOT_FOUND_URL);
 
   currentDocument: Observable<DocumentContents>;
 
@@ -34,44 +33,45 @@ export class DocumentService {
 
   private getDocument(url: string) {
     this.logger.log('getting document', url);
-    const path = this.computePath(url);
+    url = this.cleanUrl(url);
-    if ( !this.cache.has(path)) {
+    if ( !this.cache.has(url)) {
-      this.cache.set(path, this.fetchDocument(path));
+      this.cache.set(url, this.fetchDocument(url));
     }
-    return this.cache.get(path);
+    return this.cache.get(url);
   }
 
-  private fetchDocument(path: string) {
+  private fetchDocument(url: string) {
-    this.logger.log('fetching document from', path);
+    this.logger.log('fetching document from', url);
     const subject = new AsyncSubject();
     this.http
-      .get(path)
+      .get(`content/docs/${url}.json`)
-      .map(res => res.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) {
+          if (url !== FILE_NOT_FOUND_URL) {
-            this.logger.error(`Document file not found at '${path}'`);
+            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;
   }
 }

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 = '<p>Howdy, doc viewer</p>';
-    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 = '<p>Howdy, doc viewer</p>';
-    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', () => {
       <p><aio-foo></aio-foo></p>
       <p>Below Foo</p>
     `;
-    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', () => {
       </div>
       <p>Below Foo</p>
     `;
-    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', () => {
       <aio-bar></aio-bar>
       <p>Below Bar</p>
     `;
-    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', () => {
       <aio-bar>###bar content###</aio-bar>
       <p>Below Bar</p>
     `;
-    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', () => {
       <p><aio-foo></aio-foo></p>
       <p>Bottom</p>
     `;
-    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', () => {
       <p><aio-foo></aio-foo><p>
       <p>Bottom</p>
     `;
-    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', () => {
       <p><aio-foo></aio-foo></p>
       <p>Bottom</p>
     `;
-    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', () => {
       <p><aio-baz>---More baz--</aio-baz></p>
       <p>Bottom</p>
     `;
-    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();