Refactor resolution of browser manager configuration

marp-team · Sep 25, 2024 · 8a1e878 · 8a1e878
1 parent cfd5e4b
commit 8a1e878
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 52 deletions.
diff --git a/src/config.ts b/src/config.ts
@@ -3,9 +3,9 @@ import path from 'node:path'
 import chalk from 'chalk'
 import { cosmiconfig, cosmiconfigSync } from 'cosmiconfig'
 import { osLocale } from 'os-locale'
-import { BrowserManager } from './browser/manager'
+import type { BrowserManagerConfig } from './browser/manager'
 import { info, warn, error as cliError } from './cli'
-import { ConverterOption, ConvertType } from './converter'
+import { ConvertType, type ConverterOption } from './converter'
 import { ResolvableEngine, ResolvedEngine } from './engine'
 import { keywordsAsArray } from './engine/meta-plugin'
 import { error, isError } from './error'
@@ -116,7 +116,23 @@ export class MarpCLIConfig {
 
   private constructor() {} // eslint-disable-line @typescript-eslint/no-empty-function
 
-  async converterOption(): Promise<ConverterOption> {
+  browserManagerOption() {
+    const timeout = (() => {
+      // TODO: Resolve timeout from args and configuration file
+      if (process.env.PUPPETEER_TIMEOUT) {
+        const envTimeout = Number.parseInt(process.env.PUPPETEER_TIMEOUT, 10)
+        if (!Number.isNaN(envTimeout)) return envTimeout
+      }
+      return undefined
+    })()
+
+    return {
+      protocol: 'cdp',
+      timeout,
+    } as const satisfies BrowserManagerConfig
+  }
+
+  async converterOption() {
     const inputDir = await this.inputDir()
     const server = this.args.server ?? this.conf.server ?? false
     const output = (() => {
@@ -255,19 +271,7 @@ export class MarpCLIConfig {
       return scale
     })()
 
-    const browserManager = new BrowserManager({
-      protocol: 'cdp',
-      timeout: (() => {
-        if (process.env.PUPPETEER_TIMEOUT) {
-          const envTimeout = Number.parseInt(process.env.PUPPETEER_TIMEOUT, 10)
-          if (!Number.isNaN(envTimeout)) return envTimeout
-        }
-        return undefined
-      })(),
-    })
-
     return {
-      browserManager,
       imageScale,
       inputDir,
       output,
@@ -298,7 +302,7 @@ export class MarpCLIConfig {
       options: this.conf.options || {},
       pages: !!(this.args.images || this.conf.images),
       watch: (this.args.watch ?? this.conf.watch) || preview || server || false,
-    }
+    } as const satisfies Partial<ConverterOption>
   }
 
   get files() {

diff --git a/src/marp-cli.ts b/src/marp-cli.ts
@@ -1,5 +1,5 @@
 import chalk from 'chalk'
-import type { BrowserManager } from './browser/manager'
+import { BrowserManager } from './browser/manager'
 import * as cli from './cli'
 import { fromArguments } from './config'
 import { Converter, ConvertedCallback, ConvertType } from './converter'
@@ -296,10 +296,15 @@ export const marpCli = async (
     const config = await fromArguments(args)
     if (args.version) return await version(config)
 
+    // Initialize browser manager
+    browserManager = new BrowserManager(config.browserManagerOption())
+
     // Initialize converter
-    const converter = new Converter(await config.converterOption())
+    const converter = new Converter({
+      ...(await config.converterOption()),
+      browserManager,
+    })
     const cvtOpts = converter.options
-    browserManager = cvtOpts.browserManager
 
     // Find target markdown files
     const finder = async (): Promise<File[]> => {

diff --git a/test/converter.ts b/test/converter.ts
@@ -10,6 +10,7 @@ import { imageSize } from 'image-size'
 import { PDFDocument, PDFDict, PDFName, PDFHexString, PDFNumber } from 'pdf-lib'
 import { TimeoutError } from 'puppeteer-core'
 import { fromBuffer as yauzlFromBuffer } from 'yauzl'
+import { BrowserManager } from '../src/browser/manager'
 import { Converter, ConvertType, ConverterOption } from '../src/converter'
 import { CLIError } from '../src/error'
 import { File, FileType } from '../src/file'
@@ -19,7 +20,7 @@ import { WatchNotifier } from '../src/watcher'
 
 const { CdpPage } = require('puppeteer-core/lib/cjs/puppeteer/cdp/Page') // eslint-disable-line @typescript-eslint/no-require-imports -- Puppeteer's internal module
 
-const puppeteerTimeoutMs = 60000
+const timeout = 60000
 
 let mkdirSpy: jest.SpiedFunction<typeof fs.promises.mkdir>
 
@@ -30,18 +31,27 @@ beforeEach(() => {
 })
 
 afterEach(() => mkdirSpy.mockRestore())
-afterAll(() => Converter.closeBrowser())
 
 describe('Converter', () => {
   const onePath = path.resolve(__dirname, '_files/1.md')
   const twoPath = path.resolve(__dirname, '_files/2.mdown')
   const threePath = path.resolve(__dirname, '_files/3.markdown')
 
+  let browserManager: BrowserManager
   let themeSet: ThemeSet
-  beforeEach(async () => (themeSet = await ThemeSet.initialize([])))
+
+  beforeEach(async () => {
+    browserManager = new BrowserManager({ timeout })
+    themeSet = await ThemeSet.initialize([])
+  })
+
+  afterEach(async () => {
+    await browserManager.dispose()
+  })
 
   const instance = (opts: Partial<ConverterOption> = {}) =>
     new Converter({
+      browserManager,
       themeSet,
       allowLocalFiles: false,
       engine: Marp,
@@ -57,8 +67,11 @@ describe('Converter', () => {
     })
 
   describe('#constructor', () => {
-    it('assigns initial options to options member', () => {
+    it('assigns initial options to options member', async () => {
+      await using browserManager = new BrowserManager({ timeout: 12345 })
+
       const options = {
+        browserManager,
         themeSet,
         allowLocalFiles: true,
         engine: Marp,
@@ -88,17 +101,6 @@ describe('Converter', () => {
     })
   })
 
-  describe('get #puppeteerTimeout', () => {
-    it('returns specified timeout', () => {
-      expect(instance({ puppeteerTimeout: 1000 }).puppeteerTimeout).toBe(1000)
-      expect(instance({ puppeteerTimeout: 0 }).puppeteerTimeout).toBe(0)
-    })
-
-    it('returns the default timeout 30000ms if not specified', () => {
-      expect(instance().puppeteerTimeout).toBe(30000)
-    })
-  })
-
   describe('#convert', () => {
     const md = '# <i>Hello!</i>'
     const dummyFile = new File(process.cwd())
@@ -555,7 +557,7 @@ describe('Converter', () => {
           expect(ret.newFile?.path).toBe('test.pdf')
           expect(ret.newFile?.buffer).toBe(pdf)
         },
-        puppeteerTimeoutMs
+        timeout
       )
 
       describe('with meta global directives', () => {
@@ -581,7 +583,7 @@ describe('Converter', () => {
             expect(pdf.getAuthor()).toBe('author')
             expect(pdf.getKeywords()).toBe('a; b; c')
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
 
@@ -624,7 +626,7 @@ describe('Converter', () => {
               warn.mockRestore()
             }
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
 
@@ -651,7 +653,7 @@ describe('Converter', () => {
               PDFHexString.fromText('presenter note')
             )
           },
-          puppeteerTimeoutMs
+          timeout
         )
 
         it('sets a comment author to notes if set author global directive', async () => {
@@ -788,7 +790,7 @@ describe('Converter', () => {
                 ]
               `)
             },
-            puppeteerTimeoutMs
+            timeout
           )
         })
 
@@ -833,7 +835,7 @@ describe('Converter', () => {
                 ]
               `)
             },
-            puppeteerTimeoutMs
+            timeout
           )
         })
 
@@ -898,19 +900,21 @@ describe('Converter', () => {
                 ]
               `)
             },
-            puppeteerTimeoutMs
+            timeout
           )
         })
       })
 
-      describe('with custom puppeteer timeout', () => {
+      describe('with custom timeout set by browser manager', () => {
         it('follows setting timeout', async () => {
           ;(fs as any).__mockWriteFile()
 
+          await using browserManager = new BrowserManager({ timeout: 1 })
+
           await expect(
             pdfInstance({
+              browserManager,
               output: 'test.pdf',
-              puppeteerTimeout: 1,
             }).convertFile(new File(onePath))
           ).rejects.toThrow(TimeoutError)
         })
@@ -990,7 +994,7 @@ describe('Converter', () => {
           const meta = await getPptxDocProps(pptx)
           expect(meta['dc:creator']).toBe('Created by Marp')
         },
-        puppeteerTimeoutMs
+        timeout
       )
 
       describe('with meta global directives', () => {
@@ -1020,7 +1024,7 @@ describe('Converter', () => {
               expect.objectContaining({ deviceScaleFactor: 1 })
             )
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
     })
@@ -1050,7 +1054,7 @@ describe('Converter', () => {
           expect(width).toBe(1280)
           expect(height).toBe(720)
         },
-        puppeteerTimeoutMs
+        timeout
       )
 
       describe('with 4:3 size global directive for Marp Core', () => {
@@ -1069,7 +1073,7 @@ describe('Converter', () => {
             expect(width).toBe(960)
             expect(height).toBe(720)
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
 
@@ -1090,7 +1094,7 @@ describe('Converter', () => {
             expect(width).toBe(640)
             expect(height).toBe(360)
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
     })
@@ -1121,7 +1125,7 @@ describe('Converter', () => {
           expect(width).toBe(1280)
           expect(height).toBe(720)
         },
-        puppeteerTimeoutMs
+        timeout
       )
 
       describe('with 4:3 size global directive for Marp Core', () => {
@@ -1141,7 +1145,7 @@ describe('Converter', () => {
             expect(width).toBe(960)
             expect(height).toBe(720)
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
 
@@ -1162,7 +1166,7 @@ describe('Converter', () => {
             expect(width).toBe(640)
             expect(height).toBe(360)
           },
-          puppeteerTimeoutMs
+          timeout
         )
       })
     })
@@ -1189,7 +1193,7 @@ describe('Converter', () => {
           expect(write.mock.calls[0][0]).toBe('c.001.png')
           expect(write.mock.calls[1][0]).toBe('c.002.png')
         },
-        puppeteerTimeoutMs
+        timeout
       )
     })