Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to wait for document render #36

Merged
merged 2 commits into from
Feb 4, 2022
Merged

Conversation

lidkxx
Copy link
Contributor

@lidkxx lidkxx commented Sep 6, 2021

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

helps when the script is rendering large pdfs
with a lot images.

this fixes kohheepeace#28
@juniorbotelho
Copy link

juniorbotelho commented Sep 18, 2021

🐱‍💻 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.

Copy link

@juniorbotelho juniorbotelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing it with (see conversation):

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,
  });
}

@lidkxx
Copy link
Contributor Author

lidkxx commented Sep 20, 2021

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
Copy link

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
Copy link

humphd commented Dec 30, 2021

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

@kohheepeace kohheepeace merged commit 4fa39ef into kohheepeace:master Feb 4, 2022
@kohheepeace
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some images fail to display in generated PDF
4 participants