feat: use browser context #138 #139
Conversation
This feature uses puppeteer.BrowserContext for the convertion tasks. Now, not for each task, an own browser instance is instantiated, but only one. This one and only is used, and only browserContexts are passed down to the convertion functions. Reduces CPU Load, eliminates EventEmitter warnings and speed up conversion of large amount of files by ~50%.
|
Is Node 12 on windows-latest CI flaky?? |
I think Node 12 needs to be dropped anyway. |
| export async function generateOutput(html: string, relativePath: string, config: Config): Promise<Output> { | ||
| const browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options }); | ||
|
|
||
| export async function generateOutput({ html, relativePath, config, browser }: GenerateOutputProps): Promise<Output> { |
There was a problem hiding this comment.
I'm not sure you understood the purpose of these overloads? They give better types for certain inputs, e.g. if you pass a PdfConfig as config you'd get a PdfOutput result which is more specific than Output.
There was a problem hiding this comment.
Yes, you are right.
Thanks for the explanation, didn't saw the different return types.
| const pdf = await convertMdToPdf(input, mergedConfig); | ||
| const browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options }); | ||
| const md2pdfContext = await browser.createIncognitoBrowserContext(); | ||
| const pdf = await convertMdToPdf(input, mergedConfig, md2pdfContext); |
There was a problem hiding this comment.
Missed the browser.close() here, i think?
There was a problem hiding this comment.
Thanks for all the work here, however I don't like that the browser context is created in the top level of the CLI, because it has to be passed around so much. I have a better idea of creating the browser instance in the top level of lib/generate-output.ts, and exporting it so browser.close() can be called later.
Let me open another PR and maybe you can test the performance of that, whether it's any worse than yours? (:
|
BTW I just removed you as a collaborator because I couldn't see the PRs page of this repo (would get a 500 error), and it only worked again after removing the master branch protection rules. |
I got also the 500, thought it was an GitHub issue. No Problem, I will fork it then. |
|
Closing in favor of #141. |
This feature uses puppeteer.BrowserContext for the convertion tasks. Now, not for each task, an own browser instance is instantiated, but only one. This one and only is used, and only browserContexts are passed down to the convertion functions.
Reduces CPU Load, eliminates EventEmitter warnings and speed up conversion of large amount of files by ~50%.