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.

Sign up for GitHub

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

Jump to bottom

Changed image waiting method on fullPage option, +TravisCI fix #49

Closed
wants to merge 8 commits into from
Closed

Changed image waiting method on fullPage option, +TravisCI fix #49

Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2fc50fa
Changed image waiting method as per
sgtrusty Aug 11, 2020
de2cbda
Fixed an issue with async func missing
sgtrusty Aug 11, 2020
58c3902
ESLint skipped feats
sgtrusty Aug 11, 2020
40b4286
ESLint changes...
sgtrusty Aug 11, 2020
8be2e16
ESlint bruh
sgtrusty Aug 11, 2020
bb94ae2
Promisify & eslint-disable-next-line from blocks
sgtrusty Aug 14, 2020
84488c2
Fixed issues not allowing real use case scenario
sgtrusty Aug 14, 2020
6d0fe16
Changed Promise.all from individual promises
sgtrusty Aug 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 17 additions & 14 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,6 @@ const parseCookie = (url, cookie) => {
return returnValue;
};

const imagesHaveLoaded = () => [...document.images].map(element => element.complete);

const captureWebsite = async (input, options) => {
options = {
inputType: 'url',
Expand Down Expand Up @@ -321,34 +319,39 @@ const captureWebsite = async (input, options) => {
if (screenshotOptions.fullPage) {
// Get the height of the rendered page
const bodyHandle = await page.$('body');
const bodyBoundingHeight = await bodyHandle.boundingBox();
const {height: bodyBoundingHeight} = await bodyHandle.boundingBox();
await bodyHandle.dispose();

// Scroll one viewport at a time, pausing to let content load
const viewportHeight = viewportOptions.height;
let viewportIncrement = 0;
while (viewportIncrement + viewportHeight < bodyBoundingHeight) {
const navigationPromise = page.waitForNavigation({waitUntil: 'networkidle0'});
/* eslint-disable no-await-in-loop */
await page.evaluate(_viewportHeight => {
/* eslint-disable no-undef */
/* eslint-disable-next-line no-undef */
window.scrollBy(0, _viewportHeight);
/* eslint-enable no-undef */
}, viewportHeight);
await navigationPromise;
/* eslint-enable no-await-in-loop */
viewportIncrement += viewportHeight;
}

// Scroll back to top
await page.evaluate(_ => {
/* eslint-disable no-undef */
// Wait for images to be complete and scroll back to top
await page.evaluate(async _ => {
const selectors = [...document.images];
await Promise.all(selectors.map(img => {
if (img.complete) {
return;
}

// eslint-disable-next-line no-unused-vars
return new Promise((resolve, reject) => {
promisify(img.addEventListener)('load');
promisify(img.addEventListener)('error');
});
Copy link
Owner

Choose a reason for hiding this comment

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

This is not what I proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. I can get them out of the new Promise() scope, but we'd still have to check for if(img.complete){. I didn't notice that, thanks for pointing it out.

Copy link
Contributor Author

@sgtrusty sgtrusty Aug 18, 2020 •
edited

Choose a reason for hiding this comment

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

If I did:

	const selectors = [...document.images];
	await Promise.all(selectors.map(img => {
		if (img.complete) {
			return;
		}

		promisify(img.addEventListener)('load');
		promisify(img.addEventListener)('error');
	}));

I do not think it would be returning the promises that way. Let's confirm what you really had in mind; as the code you previously posted wouldn't really work, as it calls the img variable which is declared inside the arrow func.

Copy link
Owner

Choose a reason for hiding this comment

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

The above code is not functional.

Copy link
Owner

Choose a reason for hiding this comment

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

Just put selectors.filter(image => image.complete) before the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea what you mean. This promise is supposed to wait for images that aren't finished loading (not complete) and are either going to 'load' or present an 'error'. Let me know what your take is on this.

Copy link
Owner

Choose a reason for hiding this comment

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

I forgot to put a ! in front of the condition.

Copy link
Owner

Choose a reason for hiding this comment

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

But you do see how your example is flawed, right? You're not actually awaiting the promises, so it's not waiting for anything.

Copy link
Owner

Choose a reason for hiding this comment

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

This is what I had in mind:

const selectors = [...document.images];

await Promise.all(selectors.filter(img => !img.complete).map(img => {
	return Promise.race([
		promisify(img.addEventListener)('load'),
		promisify(img.addEventListener)('error')
	]
}));

First, we filter out already loaded images (that could happen, and they would never emit the events we need), then await the first event either load or error from all the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, gotcha! I wasn't able to understand but I do now. Will add them right away, and I believe PR will be ready after that. Maybe give it a few days so that I can setup environment back up and run a couple tests. Thanks for your resolution.

}));
/* eslint-disable-next-line no-undef */
window.scrollTo(0, 0);
/* eslint-enable no-undef */
});

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

const buffer = await page.screenshot(screenshotOptions);
Expand Down