kohheepeace / mr-pdf

Generate PDF for document website.
https://www.npmjs.com/package/mr-pdf
MIT License
125 stars 41 forks source link

add option to wait for document render #36

Closed lidkxx closed 2 years ago

lidkxx commented 3 years ago

Our app documentation is around 140 pdf pages with a lot of images. During a pdf generation using mr-pdf script I would encounter two problems:

  1. Some large images not being rendered fully.
  2. Any images past ~40th page would not get rendered in the document at all.

Seems we just need a bit of a timeout to let the script glue all the html content it downloads from the docusaurus pages.

I've added --waitForRender option that takes a number value of ms in case anyone encounters a similar problem. (I am sure this won't be necessary for lightweight docs that don't have too many images).

This fixes #28

juniorbotelho commented 3 years ago

🐱‍💻 Fixing...

Result:

After the correction I made, the images were rendered correctly:

image-correct

In your approach there were some bugs that I fixed. Try to make a new PR.

🚀 Correct approach

The problem can be fixed using the native puppeteer function called waitFor().

if (waitForRender) {
  // Todo: remove this sentence
  // await new Promise((r) => setTimeout(r, waitFor));
  await page.goto(`${nextPageURL}`);
  console.log(chalk.green('Rendering...'));
  await page.waitFor(waitFor);
} else {
  // Go to the page specified by nextPageURL
  await page.goto(`${nextPageURL}`, {
    waitUntil: 'networkidle0',
    timeout: 0,
  });
}

fix-it

Make the same changes as above and create a new PR.

lidkxx commented 3 years ago

Yeah, your solution is a bit more sophisticated. Thanks for looking at mine! Amended in my last commit.

I've left the waitForRender option as number still, or do you think it's better off as a string?

juniorbotelho commented 3 years ago

I'm glad you liked my suggestions 😅.

About the waitForRender attribute typing by default it is a string, this is how commander sets the values.

humphd commented 2 years ago

Would love to see this get merged, as we're running into problems with images not rendering.

kohheepeace commented 2 years ago

@lidkxx super thanks for your PR, and sorry for my late response.