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

[#50] Fixing fullPage #51

Closed
wants to merge 1 commit into from
Closed

[#50] Fixing fullPage #51

wants to merge 1 commit into from

Conversation

johnyvelho
Copy link

PR related to #50

…alue from the object, removing waitForNavigation
@sindresorhus
Copy link
Owner

There's already a PR for this: #49 Would be great if you could help review it.

await page.waitForFunction(imagesHaveLoaded, {timeout: 60});
try {
// Some extra delay to let images load
await page.waitForFunction(imagesHaveLoaded, {timeout: timeoutInSeconds});
Copy link
Contributor

@sgtrusty sgtrusty Aug 29, 2020

Choose a reason for hiding this comment

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

The use case scenario regarding the function being used in this context is now deprecated. Please check out the latest changes in #49

@johnyvelho
Copy link
Author

johnyvelho commented Aug 29, 2020

@sindresorhus, @netrules yeah, I checked before start it. honestly, I intended just to fix the zombie process. But since I got these other errors, I fixed. I believe what you are trying to do on #49 is a different feature. The tests are described as "lazy image loading" and that is what is supposed to do: scroll and let the images lazy images load/try in the timeout limit. When you set the capture-website settings, you can set a timeout, the client is expecting this timeout is going to be accomplished. the current way is fulfilling it, your changes could cause infinite processing or easily exceed the timeout configured. I would treat what you are trying to do as a different feature, with automatic scroll, keep scrolling if new elements are attached to the documents, option to wait to load all the images + xhr requests, etc. but as I said, I truly believe this should be part of a new option/feature

@sgtrusty
Copy link
Contributor

@johnyvelho Could you fork that repository and see if the error persists?

@sindresorhus
Copy link
Owner

Closing as this is not the correct way to fix it. Closing in favor of #58.

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.

3 participants