Changed image waiting method on fullPage option, +TravisCI fix

[sgtrusty]

#40 (comment)

Trying to fix last reports not running as well as fixing out what was pointed in the discussion above. The function for fullPage on lazy loading elements checks for pending images at the end of the conditional block by using a waitForFunction, when it could've been improved to the code currently PR'd.

[sgtrusty]

I think we want to just ignore if an image load fails. So this should be resolve too. Which means you could just use util.promisify on both these.

Lol, care to elaborate? Should we be using your https://github.com/sindresorhus/p-event library? Because we still need to wait for the events responses in order to deliver promise

[sindresorhus]

Lol, care to elaborate? Should we be using your sindresorhus/p-event library? Because we still need to wait for the events responses in order to deliver promise

If a single image fails to load because of server/network problems, it shouldn't cause the whole script to crash.

I was thinking:

return Promise.all(
	promisify(img.addEventListener)('load'));
	promisify(img.addEventListener)('error'));
);

[sgtrusty]

I had no idea you could call addEventListener like that, honestly. Thanks for educating me. I am going to make the changes and test it on a brand new environment I've been patching up in a Dockerfile container. Let's see if everything runs smoothly, besides CI/CD, obviously.

[sgtrusty]

Hmm. I've made a couple tests myself, outside of the regression tests and I'm not satisfied with the outcomes. I would recommend trying the new features before officially adding all support -- not to mention endless scrollers. Fixed #50 issues.

[sindresorhus]

I've made a couple tests myself, outside of the regression tests and I'm not satisfied with the outcomes.

What specifically?

[sindresorhus]

This is not what I proposed.

[sgtrusty]

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.

[sgtrusty]

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.

[sindresorhus]

The above code is not functional.

[sindresorhus]

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

[sgtrusty]

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.

[sindresorhus]

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

[sindresorhus]

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

[sindresorhus]

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.

[sgtrusty]

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.

[sgtrusty]

I've made a couple tests myself, outside of the regression tests and I'm not satisfied with the outcomes.

What specifically?

I already made a few fixes, but I noticed the script wasn't really working the way it was supposed to. I guess there are other things to take into account when loading fullpage, such as iframes, additional scripts, loading bars, etc.

[sgtrusty]

So, I'm getting this error:

On this part of the code:

		await page.evaluate(async _ => {
			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')
				]);
			}));
			/* eslint-disable-next-line no-undef */
			window.scrollTo(0, 0);
		});

@ https://github.com/netrules/capture-website/blob/patch-1/index.js#L344-L345

Apparently the "promisify" alias is escaping from the arrow function scope. Any clues what I can do here?

[sindresorhus]

I think the error is pretty clear. page.evaluate evaluates the code in the browser where the promisify function is not available.

[sindresorhus]

Closing as this is not moving forward. #58

[sgtrusty]

What is the alternative? Is there any way to include a polyfill script that makes the promisify work, use a function other than page.evaluate, or quit using promisify and return to one of the older alternatives?

Let me know, I'll fix it right away. Thanks.

[ghost]

@netrules

Update (I misunderstood the purpose of the Promise.all use case).

As far as understand you want to wait for either error or load even on each image in a Promise.
For this it should be sufficient to do it like this:

new Promise((resolve) => {
  img.addEventListener('error', resolve);
  img.addEventListener('load', resolve);
});

[sgtrusty]

It was like that previously (https://github.com/netrules/capture-website/blob/8be2e16e06f4a2582f6f2481af1ce4da76a1af65/index.js#L350-L351 😄 ) but Sindre insisted it be with promisify for some reason. Maybe there's a way to indicate the headless browser to include promisify?