From 5e6a3ff6a7df27eb72d4b064bec9dc6c6a548cc9 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Thu, 20 Apr 2017 14:16:36 +0100 Subject: [PATCH] refactor(aio): rename DocumentContents `url` to `id` and tidy up (#16139) Dgeni is now providing the `id` for all the documents to be viewed. So we no longer need to add this to the DocumentContents object. There are some notable changes in the refactoring: `DocumentService`: * The id of the document to render is now obtained from `LocationService.path()`. * The `getFileNotFoundDoc` and `getErrorDoc` methods have been extracted from the `fetchDocument` method. `AppComponent`: * the `pageId` is now computed in a separate `setPageId` method. `AppComponen` spec file: * The `TestHttp` has had the hard coded documents removed and replaced with a function that will generate docs as needed. --- aio/src/app/app.component.spec.ts | 56 ++++++----------- aio/src/app/app.component.ts | 14 ++--- aio/src/app/documents/document-contents.ts | 5 +- .../app/documents/document.service.spec.ts | 9 ++- aio/src/app/documents/document.service.ts | 63 ++++++++++--------- .../doc-viewer/doc-viewer.component.spec.ts | 24 +++---- 6 files changed, 78 insertions(+), 93 deletions(-) diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index 94e29dbedb..4b5ca7cb42 100644 --- a/aio/src/app/app.component.spec.ts +++ b/aio/src/app/app.component.spec.ts @@ -207,7 +207,7 @@ describe('AppComponent', () => { describe('pageId', () => { - it('should set the id of the doc viewer container based on the current url', () => { + it('should set the id of the doc viewer container based on the current doc', () => { const container = fixture.debugElement.query(By.css('section.sidenav-content')); locationService.go('guide/pipes'); @@ -226,7 +226,7 @@ describe('AppComponent', () => { expect(container.properties['id']).toEqual('home'); }); - it('should not be affected by changes to the query or hash', () => { + it('should not be affected by changes to the query', () => { const container = fixture.debugElement.query(By.css('section.sidenav-content')); locationService.go('guide/pipes'); @@ -236,11 +236,6 @@ describe('AppComponent', () => { fixture.detectChanges(); expect(component.pageId).toEqual('guide-other'); expect(container.properties['id']).toEqual('guide-other'); - - locationService.go('guide/http#anchor-1'); - fixture.detectChanges(); - expect(component.pageId).toEqual('guide-http'); - expect(container.properties['id']).toEqual('guide-http'); }); }); @@ -261,7 +256,7 @@ describe('AppComponent', () => { it('should display a marketing page', () => { locationService.go('features'); fixture.detectChanges(); - expect(docViewer.innerText).toMatch(/Test Doc/i); + expect(docViewer.innerText).toMatch(/Features Doc/i); }); it('should update the document title', () => { @@ -275,7 +270,7 @@ describe('AppComponent', () => { it('should update the document title, with a default value if the document has no title', () => { const titleService = TestBed.get(Title); spyOn(titleService, 'setTitle'); - locationService.go('file-not-found'); + locationService.go('no-title'); fixture.detectChanges(); expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); }); @@ -431,35 +426,20 @@ class TestHttp { } }; - apiDoc = { - "title": "API", - "contents": "

API Doc

" - }; - - pipesDoc = { - "title": "Pipes", - "contents": "

Pipes Doc

" - }; - - testDoc = { - "title": "Test", - "contents": "

Test Doc

" - }; - - fileNotFoundDoc = { - "title": "", - "contents": "Page not found" - }; - - // get = jasmine.createSpy('get').and.callFake((url: string) => { ... }); get(url: string) { - const json = - /navigation.json/.test(url) ? this.navJson : - /api/.test(url) ? this.apiDoc : - /pipes/.test(url) ? this.pipesDoc : - /file-not-found/.test(url) ? this.fileNotFoundDoc : - this.testDoc; - return of({ json: () => json }); + let data; + if (/navigation\.json/.test(url)) { + data = this.navJson; + } else { + const match = /content\/docs\/(.+)\.json/.exec(url); + const id = match[1]; + const title = id.split('/').pop().replace(/^([a-z])/, (_, letter) => letter.toUpperCase()); + const contents = `

${title} Doc

`; + data = { id, title, contents }; + if (id === 'no-title') { + data.title = ''; + } + } + return of({ json: () => data }); } - } diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index f3c263c8e6..eda9502d5b 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -72,12 +72,7 @@ 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'; - } + this.setPageId(doc.id); }); // scroll even if only the hash fragment changed @@ -151,11 +146,16 @@ export class AppComponent implements OnInit { this.sidenav.toggle(value); } - setDocumentTitle(title) { + setDocumentTitle(title: string) { if (title.trim()) { this.titleService.setTitle(`Angular - ${title}`); } else { this.titleService.setTitle('Angular'); } } + + setPageId(id: string) { + // Special case the home page + this.pageId = (id === 'index') ? 'home' : id.replace('/', '-'); + } } diff --git a/aio/src/app/documents/document-contents.ts b/aio/src/app/documents/document-contents.ts index 53471eca07..a4ac90e008 100644 --- a/aio/src/app/documents/document-contents.ts +++ b/aio/src/app/documents/document-contents.ts @@ -1,5 +1,8 @@ export interface DocumentContents { - url: string; + /** The unique identifier for this document */ + id: string; + /** The string to display in the browser tab when this document is being viewed */ title: string; + /** The HTML to display in the doc viewer */ contents: string; } diff --git a/aio/src/app/documents/document.service.spec.ts b/aio/src/app/documents/document.service.spec.ts index a7702b6d34..296a55878d 100644 --- a/aio/src/app/documents/document.service.spec.ts +++ b/aio/src/app/documents/document.service.spec.ts @@ -86,15 +86,14 @@ describe('DocumentService', () => { 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' }; + const fileNotFoundDoc = { id: 'file-not-found', title: 'Page Not Found', contents: '

Page Not Found

' }; connections[1].mockRespond(createResponse(fileNotFoundDoc)); - expect(currentDocument).toEqual(jasmine.objectContaining(fileNotFoundDoc)); - expect(currentDocument.url).toEqual('file-not-found'); + expect(currentDocument).toEqual(fileNotFoundDoc); }); 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: DocumentContents = { title: 'Not Found', contents: 'Document not found', url: 'file-not-found' }; + const notFoundDoc: DocumentContents = { title: 'Not Found', contents: 'Document not found', id: 'file-not-found' }; const nextDoc = { title: 'Next Doc', url: 'new/url' }; const { docService, backend, locationService } = getServices('file-not-found'); const connections = backend.connectionsArray; @@ -178,7 +177,7 @@ describe('DocumentService', () => { }); it('should map the "folder" locations to the correct document request', () => { - const { docService, backend, locationService} = getServices('guide/'); + const { docService, backend, locationService} = getServices('guide'); docService.currentDocument.subscribe(); expect(backend.connectionsArray[0].request.url).toEqual(CONTENT_URL_PREFIX + 'guide.json'); diff --git a/aio/src/app/documents/document.service.ts b/aio/src/app/documents/document.service.ts index 3b6882cb9d..94e72bdd50 100644 --- a/aio/src/app/documents/document.service.ts +++ b/aio/src/app/documents/document.service.ts @@ -15,6 +15,7 @@ import { LocationService } from 'app/shared/location.service'; import { Logger } from 'app/shared/logger.service'; const FILE_NOT_FOUND_URL = 'file-not-found'; +const FETCHING_ERROR_URL = 'fetching-error'; @Injectable() export class DocumentService { @@ -28,50 +29,52 @@ export class DocumentService { private http: Http, location: LocationService) { // Whenever the URL changes we try to get the appropriate doc - this.currentDocument = location.currentUrl.switchMap(url => this.getDocument(url)); + this.currentDocument = location.currentUrl.switchMap(() => this.getDocument(location.path())); } private getDocument(url: string) { - this.logger.log('getting document', url); - url = this.cleanUrl(url); - if ( !this.cache.has(url)) { - this.cache.set(url, this.fetchDocument(url)); + const id = url || 'index'; + this.logger.log('getting document', id); + if ( !this.cache.has(id)) { + this.cache.set(id, this.fetchDocument(id)); } - return this.cache.get(url); + return this.cache.get(id); } - private fetchDocument(url: string) { - this.logger.log('fetching document from', url); + private fetchDocument(id: string): Observable { + const requestPath = `content/docs/${id}.json`; + this.logger.log('fetching document from', requestPath); const subject = new AsyncSubject(); this.http - .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) + .get(requestPath) + .map(response => response.json()) .catch((error: Response) => { - if (error.status === 404) { - 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', 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.', url}); - } + return error.status === 404 ? this.getFileNotFoundDoc(id) : this.getErrorDoc(error); }) .subscribe(subject); return subject.asObservable(); } - 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'; + private getFileNotFoundDoc(url: string): Observable { + 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', + id: FILE_NOT_FOUND_URL + }); } - return url; + } + + private getErrorDoc(error: Response): Observable { + this.logger.error('Error fetching document', error); + return Observable.of({ + title: 'Error fetching document', + contents: 'Sorry we were not able to fetch that document.', + id: FETCHING_ERROR_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 5111f5ee15..4e8bf3031f 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: '', url: 'a/b' }; + component.currentDoc = { title: 'fake title', contents: '', id: '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, url: 'a/b' }; + component.currentDoc = { title: 'fake title', contents, id: '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, url: 'a/b' }; + component.currentDoc = { title: 'fake title', contents, id: 'a/b' }; fixture.detectChanges(); - component.currentDoc = { title: 'fake title', contents: '', url: 'a/c' }; + component.currentDoc = { title: 'fake title', contents: '', id: 'a/c' }; fixture.detectChanges(); expect(docViewerEl.innerHTML).toEqual(''); }); @@ -149,7 +149,7 @@ describe('DocViewerComponent', () => {

Below Foo

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

Bottom

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