From d2c64415610a5f26bdf7f6add5ad5584cca991a6 Mon Sep 17 00:00:00 2001 From: Yuki Hattori Date: Wed, 25 Sep 2024 23:44:09 +0900 Subject: [PATCH] Make the converter using new manager compatible with the behavior of v3 --- package.json | 1 + src/browser/finder.ts | 26 ++++++++++++++------------ src/browser/finders/chrome.ts | 13 ++++++++++--- src/browser/finders/firefox.ts | 10 +++++----- src/browser/manager.ts | 26 ++++++++++++++++++++------ src/config.ts | 22 +++++++++++++--------- src/converter.ts | 13 +++++++++---- src/engine.ts | 26 ++++++++++++++++++++++---- src/error.ts | 7 ++++++- src/marp-cli.ts | 9 +++++---- src/utils/debug.ts | 1 + yarn.lock | 12 ++++++++++++ 12 files changed, 118 insertions(+), 48 deletions(-) diff --git a/package.json b/package.json index b6132479..f6c18470 100644 --- a/package.json +++ b/package.json @@ -82,6 +82,7 @@ "@tsconfig/node20": "^20.1.4", "@tsconfig/recommended": "^1.0.7", "@types/cheerio": "^0.22.35", + "@types/debug": "^4.1.12", "@types/dom-view-transitions": "^1.0.5", "@types/express": "^4.17.21", "@types/jest": "^29.5.13", diff --git a/src/browser/finder.ts b/src/browser/finder.ts index f59e9202..5baa8e07 100644 --- a/src/browser/finder.ts +++ b/src/browser/finder.ts @@ -29,6 +29,12 @@ export const findBrowser = async ( finders: readonly FinderName[] = defaultFinders, opts: BrowserFinderOptions = {} ) => { + let found = false + + const debug = (...args: Parameters) => { + if (!found) return debugBrowserFinder(...args) + } + const finderCount = finders.length const normalizedOpts = { preferredPath: await (async () => { @@ -41,13 +47,10 @@ export const findBrowser = async ( } if (finderCount === 0) { - debugBrowserFinder('No browser finder specified.') + debug('No browser finder specified.') if (normalizedOpts.preferredPath) { - debugBrowserFinder( - 'Use preferred path as Chrome: %s', - normalizedOpts.preferredPath - ) + debug('Use preferred path as Chrome: %s', normalizedOpts.preferredPath) return await chrome(normalizedOpts) } @@ -58,10 +61,7 @@ export const findBrowser = async ( ) } - debugBrowserFinder( - `Start finding browser from ${finders.join(', ')} (%o)`, - normalizedOpts - ) + debug(`Start finding browser from ${finders.join(', ')} (%o)`, normalizedOpts) return new Promise((res, rej) => { const results = Array(finderCount) @@ -72,12 +72,12 @@ export const findBrowser = async ( finder(normalizedOpts) .then((ret) => { - debugBrowserFinder(`Found ${finderName}: %o`, ret) + debug(`Found ${finderName}: %o`, ret) results[index] = ret resolved[index] = true }) .catch((e) => { - debugBrowserFinder(`Finder ${finderName} was failed: %o`, e) + debug(`Finder ${finderName} was failed: %o`, e) resolved[index] = false }) .finally(() => { @@ -100,7 +100,9 @@ export const findBrowser = async ( }) }) }).then((result) => { - debugBrowserFinder('Use browser: %o', result) + debug('Use browser: %o', result) + found = true + return result }) } diff --git a/src/browser/finders/chrome.ts b/src/browser/finders/chrome.ts index 90de81a5..db3083f2 100644 --- a/src/browser/finders/chrome.ts +++ b/src/browser/finders/chrome.ts @@ -8,7 +8,12 @@ import { error, CLIErrorCode } from '../../error' import { ChromeBrowser } from '../browsers/chrome' import { ChromeCdpBrowser } from '../browsers/chrome-cdp' import type { BrowserFinder, BrowserFinderResult } from '../finder' -import { findExecutableBinary, getPlatform, isExecutable } from './utils' +import { + findExecutableBinary, + getPlatform, + isExecutable, + normalizeDarwinAppPath, +} from './utils' const chrome = (path: string): BrowserFinderResult => ({ path, @@ -18,8 +23,10 @@ const chrome = (path: string): BrowserFinderResult => ({ export const chromeFinder: BrowserFinder = async ({ preferredPath } = {}) => { if (preferredPath) return chrome(preferredPath) - if (process.env.CHROME_PATH && (await isExecutable(process.env.CHROME_PATH))) - return chrome(process.env.CHROME_PATH) + if (process.env.CHROME_PATH) { + const path = await normalizeDarwinAppPath(process.env.CHROME_PATH) + if (path && (await isExecutable(path))) return chrome(path) + } const platform = await getPlatform() const installation = await (async () => { diff --git a/src/browser/finders/firefox.ts b/src/browser/finders/firefox.ts index 8d2aea19..9bdc25af 100644 --- a/src/browser/finders/firefox.ts +++ b/src/browser/finders/firefox.ts @@ -7,6 +7,7 @@ import { findExecutable, findExecutableBinary, isExecutable, + normalizeDarwinAppPath, } from './utils' const firefox = (path: string): BrowserFinderResult => ({ @@ -22,11 +23,10 @@ const winFirefoxDefault = ['Mozilla Firefox', 'firefox.exe'] // Firefox stable, export const firefoxFinder: BrowserFinder = async ({ preferredPath } = {}) => { if (preferredPath) return firefox(preferredPath) - if ( - process.env.FIREFOX_PATH && - (await isExecutable(process.env.FIREFOX_PATH)) - ) - return firefox(process.env.FIREFOX_PATH) + if (process.env.FIREFOX_PATH) { + const nPath = await normalizeDarwinAppPath(process.env.FIREFOX_PATH) + if (nPath && (await isExecutable(nPath))) return firefox(nPath) + } const platform = await getPlatform() const installation = await (async () => { diff --git a/src/browser/manager.ts b/src/browser/manager.ts index 67ff4c90..17328299 100644 --- a/src/browser/manager.ts +++ b/src/browser/manager.ts @@ -53,6 +53,8 @@ export class BrowserManager implements AsyncDisposable { this._preferredProtocol = config.protocol } if (config.timeout !== undefined) this._timeout = config.timeout + + debugBrowser('Browser manager configured: %o', config) } async findBrowser() { @@ -65,10 +67,23 @@ export class BrowserManager implements AsyncDisposable { async browserForConversion(): Promise { if (!this._conversionBrowser) { const { acceptedBrowsers, path } = await this.findBrowser() - const browser = acceptedBrowsers.find( - (browser) => browser.protocol === this._preferredProtocol - ) + + const browser = + acceptedBrowsers.find( + ({ protocol }) => protocol === this._preferredProtocol + ) || + (() => { + if (acceptedBrowsers.length > 0) { + debugBrowser( + 'The available browsers do not support the preferred protocol "%s". Using the first available browser.', + this._preferredProtocol + ) + } + return acceptedBrowsers[0] + })() + if (!browser) error('No browser found for conversion') + debugBrowser('Use browser class for conversion: %o', browser) // @ts-expect-error ts2511: TS cannot create an instance of an abstract class this._conversionBrowser = new browser({ path, timeout: this._timeout }) @@ -80,9 +95,11 @@ export class BrowserManager implements AsyncDisposable { async browserForPreview(): Promise { if (!this._previewBrowser) { const { acceptedBrowsers, path } = await this.findBrowser() + if (!acceptedBrowsers.some((browser) => browser === ChromeCdpBrowser)) { error('No browser found for preview') } + debugBrowser('Use browser class for preview: %o', ChromeCdpBrowser) this._previewBrowser = new ChromeCdpBrowser({ path, @@ -103,6 +120,3 @@ export class BrowserManager implements AsyncDisposable { await this.dispose() } } - -export const browserManager = new BrowserManager() -export default browserManager diff --git a/src/config.ts b/src/config.ts index dc90b42f..599ee215 100644 --- a/src/config.ts +++ b/src/config.ts @@ -3,6 +3,7 @@ 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 { info, warn, error as cliError } from './cli' import { ConverterOption, ConvertType } from './converter' import { ResolvableEngine, ResolvedEngine } from './engine' @@ -254,22 +255,25 @@ export class MarpCLIConfig { return scale })() - const puppeteerTimeout = (() => { - if (process.env['PUPPETEER_TIMEOUT']) { - const envTimeout = Number.parseInt(process.env['PUPPETEER_TIMEOUT'], 10) - if (!Number.isNaN(envTimeout)) return envTimeout - } - return undefined - })() + 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, pdfNotes, pdfOutlines, preview, - puppeteerTimeout, server, template, templateOption, @@ -411,4 +415,4 @@ export class MarpCLIConfig { } } -export default MarpCLIConfig.fromArguments +export const { fromArguments } = MarpCLIConfig diff --git a/src/converter.ts b/src/converter.ts index ab46c2a9..826ad7bc 100644 --- a/src/converter.ts +++ b/src/converter.ts @@ -9,7 +9,7 @@ import type { Viewport, } from 'puppeteer-core' import type { Browser } from './browser/browser' -import { browserManager } from './browser/manager' +import type { BrowserManager } from './browser/manager' import { silence, warn } from './cli' import { Engine, ResolvedEngine } from './engine' import { generateOverrideGlobalDirectivesPlugin } from './engine/directive-plugin' @@ -64,6 +64,7 @@ export const mimeTypes = { export interface ConverterOption { allowLocalFiles: boolean baseUrl?: string + browserManager: BrowserManager engine: Engine globalDirectives: { theme?: string } & Partial html?: MarpOptions['html'] @@ -130,6 +131,10 @@ export class Converter { this.options = opts } + get browser(): Promise { + return this.options.browserManager.browserForConversion() + } + get template(): Template { const template = templates[this.options.template] if (!template) error(`Template "${this.options.template}" is not found.`) @@ -153,7 +158,7 @@ export class Converter { if (this.options.baseUrl) return this.options.baseUrl if (isFile(f) && type !== ConvertType.html) { - const browser = await browserManager.browserForConversion() + const browser = await this.browser return (await browser.browserInWSLHost()) ? `file:${await resolveWSLPathToHost(f.absolutePath)}` @@ -304,7 +309,7 @@ export class Converter { const ret = file.convert(this.options.output, { extension: 'pdf' }) // Generate PDF - const browser = await browserManager.browserForConversion() + const browser = await this.browser if (browser.kind === 'firefox' && !this._firefoxPDFConversionWarning) { this._firefoxPDFConversionWarning = true @@ -634,7 +639,7 @@ export class Converter { // eslint-disable-next-line @typescript-eslint/no-unused-vars using _tmpFile = tmpFile ?? { [Symbol.dispose]: () => void 0 } - const browser = await browserManager.browserForConversion() + const browser = await this.browser if (tmpFile) { if (await browser.browserInWSLHost()) { diff --git a/src/engine.ts b/src/engine.ts index 08647d82..925597f0 100644 --- a/src/engine.ts +++ b/src/engine.ts @@ -7,6 +7,7 @@ import importFrom from 'import-from' import { resolve as importMetaResolve } from 'import-meta-resolve' import { pkgUp } from 'pkg-up' import { error, isError } from './error' +import { debugEngine } from './utils/debug' type FunctionalEngine = ( constructorOptions: ConstructorParameters[0] & { readonly marp: Marp } @@ -137,21 +138,25 @@ export class ResolvedEngine { moduleId: string, from?: string ): Promise { + let normalizedModuleId = moduleId + const basePath = path.join(from || process.cwd(), '_.js') const dirPath = path.dirname(basePath) - const moduleFilePath = path.resolve(dirPath, moduleId) + const moduleFilePath = path.resolve(dirPath, normalizedModuleId) try { const stat = await fs.promises.stat(moduleFilePath) - if (stat.isFile()) moduleId = url.pathToFileURL(moduleFilePath).toString() + if (stat.isFile()) { + normalizedModuleId = url.pathToFileURL(moduleFilePath).toString() + } } catch { // No ops } try { const resolved = importMetaResolve( - moduleId, + normalizedModuleId, url.pathToFileURL(basePath).toString() ) @@ -169,7 +174,14 @@ export class ResolvedEngine { return await import(resolved) /* c8 ignore stop */ - } catch { + } catch (e) { + debugEngine( + 'Failed to import %s. (Normalized module id: %s)', + moduleId + (from ? ` from ${from}` : ''), + normalizedModuleId + ) + debugEngine('%O', e) + return null } } @@ -187,6 +199,12 @@ export class ResolvedEngine { /* c8 ignore start */ } catch (e) { + debugEngine( + 'Failed to require %s.', + moduleId + (from ? ` from ${from}` : '') + ) + debugEngine('%O', e) + if (isError(e) && e.code === 'ERR_REQUIRE_ESM') { // Show reason why `require()` failed in the current context if ('pkg' in process) { diff --git a/src/error.ts b/src/error.ts index 236260da..c93dda07 100644 --- a/src/error.ts +++ b/src/error.ts @@ -1,3 +1,5 @@ +import { debug } from './utils/debug' + export class CLIError extends Error { readonly errorCode: number readonly message: string @@ -28,7 +30,10 @@ export function error( msg: string, errorCode: number = CLIErrorCode.GENERAL_ERROR ): never { - throw new CLIError(msg, errorCode) + const cliError = new CLIError(msg, errorCode) + debug('%O', cliError) + + throw cliError } export const isError = (e: unknown): e is NodeJS.ErrnoException => diff --git a/src/marp-cli.ts b/src/marp-cli.ts index edf8f680..14df8629 100644 --- a/src/marp-cli.ts +++ b/src/marp-cli.ts @@ -1,7 +1,7 @@ import chalk from 'chalk' -import { browserManager } from './browser/manager' +import type { BrowserManager } from './browser/manager' import * as cli from './cli' -import fromArguments from './config' +import { fromArguments } from './config' import { Converter, ConvertedCallback, ConvertType } from './converter' import { CLIError, error, isError } from './error' import { File, FileType } from './file' @@ -47,6 +47,7 @@ export const marpCli = async ( argv: string[], { baseUrl, stdin: defaultStdin, throwErrorAlways }: MarpCLIInternalOptions ): Promise => { + let browserManager: BrowserManager | undefined let server: Server | undefined let watcherInstance: Watcher | undefined @@ -298,6 +299,7 @@ export const marpCli = async ( // Initialize converter const converter = new Converter(await config.converterOption()) const cvtOpts = converter.options + browserManager = cvtOpts.browserManager // Find target markdown files const finder = async (): Promise => { @@ -448,8 +450,7 @@ export const marpCli = async ( } finally { await Promise.all([ notifier.stop(), - // Converter.closeBrowser(), // TODO: Remove this line after replacing browser management into the new manager - browserManager.dispose(), + browserManager?.dispose(), server?.stop(), watcherInstance?.chokidar.close(), ]) diff --git a/src/utils/debug.ts b/src/utils/debug.ts index fd60a18f..9684b5c2 100644 --- a/src/utils/debug.ts +++ b/src/utils/debug.ts @@ -3,3 +3,4 @@ import dbg from 'debug' export const debug = dbg('marp-cli') export const debugBrowser = dbg('marp-cli:browser') export const debugBrowserFinder = dbg('marp-cli:browser:finder') +export const debugEngine = dbg('marp-cli:engine') diff --git a/yarn.lock b/yarn.lock index 00d1d44d..d69caf37 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1835,6 +1835,13 @@ resolved "https://registry.yarnpkg.com/@types/cookiejar/-/cookiejar-2.1.5.tgz#14a3e83fa641beb169a2dd8422d91c3c345a9a78" integrity sha512-he+DHOWReW0nghN24E1WUqM0efK4kI9oTqDm6XmK8ZPe2djZ90BSNdGnIyCLzCPw7/pogPlGbzI2wHGGmi4O/Q== +"@types/debug@^4.1.12": + version "4.1.12" + resolved "https://registry.yarnpkg.com/@types/debug/-/debug-4.1.12.tgz#a155f21690871953410df4b6b6f53187f0500917" + integrity sha512-vIChWdVG3LG1SMxEvI/AK+FWJthlrqlTu7fbrlywTkkaONwk/UAGaULXRlf8vkzFBLVm0zkMdCquhL5aOjhXPQ== + dependencies: + "@types/ms" "*" + "@types/dom-view-transitions@^1.0.5": version "1.0.5" resolved "https://registry.yarnpkg.com/@types/dom-view-transitions/-/dom-view-transitions-1.0.5.tgz#06070954f1ebc2f94bbea5a64618574772eb4c1d" @@ -1951,6 +1958,11 @@ resolved "https://registry.yarnpkg.com/@types/mime/-/mime-1.3.5.tgz#1ef302e01cf7d2b5a0fa526790c9123bf1d06690" integrity sha512-/pyBZWSLD2n0dcHE3hq8s8ZvcETHtEuF+3E7XVt0Ig2nvsVQXdghHVcEkIWjy9A0wKfTn97a/PSDYohKIlnP/w== +"@types/ms@*": + version "0.7.34" + resolved "https://registry.yarnpkg.com/@types/ms/-/ms-0.7.34.tgz#10964ba0dee6ac4cd462e2795b6bebd407303433" + integrity sha512-nG96G3Wp6acyAgJqGasjODb+acrI7KltPiRxzHPXnP3NgI28bpQDRv53olbqGXbfcgF5aiiHmO3xpwEpS5Ld9g== + "@types/node@*": version "22.7.0" resolved "https://registry.yarnpkg.com/@types/node/-/node-22.7.0.tgz#670aa1874bc836863e5c116f9f2c32416ff27e1f"