Refactor Server class to handle error while converting

CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Fixed
 
 - Downgrade pkg to v4.3.x to fix segfault in the standalone build for Windows ([#111](https://github.com/marp-team/marp-cli/issues/111), [#112](https://github.com/marp-team/marp-cli/pull/112))
+- Improve error handling while running server ([#115](https://github.com/marp-team/marp-cli/pull/115))
 
 ## v0.11.0 - 2019-06-24
 

src/marp-cli.ts

@@ -287,9 +287,11 @@ export default async function(argv: string[] = []): Promise<number> {
 
       if (cvtOpts.server) {
         const server = new Server(converter, {
-          onConverted,
           directoryIndex: ['index.md', 'PITCHME.md'], // GitPitch compatible
         })
+        server.on('converted', onConverted)
+        server.on('error', e => cli.error(e.toString()))
+
         await server.start()
 
         const url = `http://localhost:${server.port}`

src/preview.ts

@@ -104,18 +104,18 @@ export class Preview extends TypedEventEmitter<Preview.Events> {
 }
 
 export namespace Preview {
-  export interface Options {
-    height: number
-    width: number
-  }
-
   export interface Events {
     close(window: any): void
     exit(): void
     launch(): void
     open(window: any, location: string): void
     opening(location: string): void
   }
+
+  export interface Options {
+    height: number
+    width: number
+  }
 }
 
 export function fileToURI(file: File, type: ConvertType) {

src/server.ts

@@ -8,25 +8,28 @@ import { promisify } from 'util'
 import { Converter, ConvertedCallback, ConvertType } from './converter'
 import { error, CLIError } from './error'
 import { File, markdownExtensions } from './file'
+import TypedEventEmitter from './utils/typed-event-emitter'
 import favicon from './assets/favicon.png'
 import serverIndex from './server/index.pug'
 import style from './server/index.scss'
 
 const stat = promisify(fs.stat)
 const readFile = promisify(fs.readFile)
 
-export class Server {
+export class Server extends TypedEventEmitter<Server.Events> {
   readonly converter: Converter
   readonly inputDir: string
   readonly options: Server.Options
   readonly port: Number
 
   directoryIndex: string[]
-  server?: Express
+  server: Express | undefined
 
   private static script: string | undefined
 
   constructor(converter: Converter, opts: Server.Options = {}) {
+    super()
+
     if (!converter.options.inputDir)
       error('Converter have to specify an input directory.')
 
@@ -61,8 +64,6 @@ export class Server {
     filename: string,
     query: querystring.ParsedUrlQuery = {}
   ) {
-    const file = new File(filename)
-
     this.converter.options.output = false
     this.converter.options.type = ((): ConvertType => {
       const queryKeys = Object.keys(query)
@@ -75,10 +76,10 @@ export class Server {
       return ConvertType.html
     })()
 
-    const converted = await this.converter.convertFile(file)
-    if (this.options.onConverted) this.options.onConverted(converted)
+    const ret = await this.converter.convertFile(new File(filename))
+    this.emit('converted', ret)
 
-    return converted
+    return ret
   }
 
   private async loadScript() {
@@ -96,8 +97,15 @@ export class Server {
     if (!pathname) return
 
     const qs = querystring.parse(query || '')
-    const response = async fn =>
-      res.end((await this.convertMarkdown(fn, qs)).newFile.buffer)
+    const response = async fn => {
+      try {
+        const ret = await this.convertMarkdown(fn, qs)
+        res.end(ret.newFile.buffer)
+      } catch (e) {
+        this.emit('error', e)
+        res.status(503).end(e.toString())
+      }
+    }
 
     const validated = await this.validateMarkdown(pathname)
 
@@ -187,12 +195,16 @@ export class Server {
 }
 
 export namespace Server {
-  export declare interface Options {
+  export interface Events {
+    converted: ConvertedCallback
+    error: (err: Error) => void
+  }
+
+  export interface Options {
     directoryIndex?: string[]
-    onConverted?: ConvertedCallback
   }
 
-  export declare interface ValidateResult {
+  export interface ValidateResult {
     path: string
     stats?: fs.Stats
     valid: boolean

test/server.ts

@@ -87,31 +87,26 @@ describe('Server', () => {
     context('when there is request to a served markdown file', () => {
       it('triggers conversion and returns the content of converted HTML', async () => {
         const server = await startServer()
-        const convertFile = jest.spyOn(server.converter, 'convertFile')
+        const cvt = jest.spyOn(server.converter, 'convertFile')
         const response = await request(server.server).get('/1.md')
 
         expect(response.status).toBe(200)
-        expect(convertFile).toBeCalledTimes(1)
-
-        const ret = await (<Promise<ConvertResult>>(
-          convertFile.mock.results[0].value
-        ))
+        expect(cvt).toBeCalledTimes(1)
 
+        const ret = await (cvt.mock.results[0].value as Promise<ConvertResult>)
         expect(response.text).toBe(ret.newFile.buffer!.toString())
       })
 
-      context('with onConverted option', () => {
-        it('runs onConverted callback after conversion', async () => {
-          const onConverted = jest.fn()
-          const server = await startServer({ onConverted })
-          const convertFile = jest.spyOn(server.converter, 'convertFile')
+      context('with listening `converted` event', () => {
+        it('emits `converted` event after conversion', async () => {
+          let ret: string | undefined
 
-          await request(server.server).get('/1.md')
+          const server = (await startServer()).on('converted', converted => {
+            ret = converted.newFile.buffer!.toString()
+          })
 
-          const ret = await (<Promise<ConvertResult>>(
-            convertFile.mock.results[0].value
-          ))
-          expect(onConverted).toBeCalledWith(ret)
+          const response = await request(server.server).get('/1.md')
+          expect(response.text).toBe(ret)
         })
       })
 
@@ -139,6 +134,21 @@ describe('Server', () => {
           expect(server.converter.options.type).toBe(ConvertType.jpeg)
         })
       })
+
+      context('when error raised while converting', () => {
+        it('returns 503 with error response and emitting event', async () => {
+          const err = new Error('test')
+          const event = jest.fn()
+          const { server, converter } = (await startServer()).on('error', event)
+
+          jest.spyOn(converter, 'convertFile').mockRejectedValue(err)
+
+          const response = await request(server).get('/1.md')
+          expect(event).toBeCalledWith(err)
+          expect(response.status).toBe(503)
+          expect(response.text).toBe('Error: test')
+        })
+      })
     })
 
     context('when there is request to served directory', () => {