Adds noPolyfill option to immediately load all lazy images in unsupported browsers

[radfahrer]

Resolves #51

Now that most browsers support intersectionObserver, its much more
tempting to leave the polyfill out to avoid punishing users of supported
browsers with the download size of the polyfill.

[malchata]

Thanks for this, @radfahrer!

So, this brings up an interesting discussion. I had solved this before, but always backed off from committing the fix because of how options.events should be handled when images are loaded immediately.

I'm beginning to wonder if it might make more sense to drop options.events in favor of only allowing users to specify handlers for onload and onerror, and then bump this to a new minor version.

Thoughts?

[radfahrer]

@malchata I haven't looked at how options.events works...

[malchata]

Thanks for adding this. :) I may tweak the copy here after I pull this in.

[radfahrer]

@malchata thanks for your patience over the holidays. I think I have it all fixed up now.

[malchata]

So, I pulled this into my local repo for testing, and found that, while your code works and loads images immediately, it doesn't trigger the events in the repo's test page (local server spun up with npm test).

This specific issue is why this ticket has been lingering for so long. I've attempted to fix it, but triggering events is problematic for any number of reasons, depending on the events attached. I've been considering removing support for events altogether, or limiting events to onload and onerror, possibly.

Thoughts?

[radfahrer]

Which events specifically are not firing? And how does the test script work?

[radfahrer]

@malchata If you can provide some reproduction steps to recreate the scenario where you are not seeing events fire, I would be glad to dig into it.

[radfahrer]

@malchata I'm still looking on some guidance for testing the event firing scenario. Once I can reproduce/understand the issue, it shouldn't be too difficult to resolve.

[malchata]

Sorry for the delay on this. To repro:

1. Remove the polyfill.io script from test/index.html
2. Add noPolyfill: true option when you initialize yall.
3. Run npm test, which will spin up a functional test at http://localhost:8080
4. Test in a browser without IntersectionObserver support. When I use IE11 in this free Windows 10 VM.

You'll notice an error re: instanceof, because yallApplyFn breaks trying to check if the given function is an instance of IntersectionObserver, which doesn't exist in IE11. You can get around this by replacing yallApplyFn with the following code:

const yallApplyFn = (items, fn) => {
  for (let itemIndex = 0; itemIndex < items.length; itemIndex++) {
    if (noPolyfill) {
      fn(items[itemIndex]);
    } else {
      fn instanceof win[io] ? fn.observe(items[itemIndex]) : fn(items[itemIndex]);
    }
  }
};

Once you reload, you'll notice that while the images load, the events attached to them don't fire. Images that are at half opacity don't come to full opacity after loading. This is an event test. Most, if not all events specified in the events option, fail to fire in the absence of a polyfill.

This is why I've avoided trying to address this issue. I believe the underlying event system makes this extremely difficult to accommodate. If you want to try and fix this, I'd welcome it, but my eventual goal for a final version is to restrict events to onload and onerror, or drop event support entirely.

[radfahrer]

I moved this oppertation here so that the class gets removed regardless of weather or not intersection observer is being used.

[radfahrer]

@malchata I modified the test script as described, and loaded up IE via browser stack. I was able to use the inspector in IE and set breakpoints in the events. I think that the events are getting bound, and fired. However, the code to remove the lazyClass was inside intersectionObserve which never gets called when intersection observer isn't supported. The test code is checking for the lazy class and therefore skipping updates such as updating the opacity.

yall.js/test/index.html

Line 222 in 5afc160

if (!event.target.classList.contains("lazy") && event.target.nodeName == "IMG") {

I relocated the code that removes the lazyClass so that it happens just after the attributes get updated

Give it a spin and let me know

[radfahrer]

@malchata have you had a chance to review my latest change, I think it addresses your concerns regarding events.

[malchata]

Hi, @radfahrer.

I pulled this down and tested your latest changes in Browserstack in IE11. It appears to work when I add the option and remove the polyfill.io script link. However, now other browsers are breaking with this combination with some variation of this error:

Uncaught TypeError: n is not a function
    at yallApplyFn (yall.min.js:1)
    at yall.min.js:1
    at HTMLDocument.<anonymous> ((index):218)

As it stands, I'm not super confident about bringing this option in with the events system in place. It seems to be adding too much complexity in a problem space that's already very complex to begin with.

Let me know your thoughts.

[radfahrer]

@malchata I'll dig in as soon as I get a chance. Thanks for the feedback.

[malchata]

It's OK, there's no rush here. I'm going to prepare a final-ish release that could make this work easier to do and with less conditions to cope with. Thanks for sticking with me on this.

[radfahrer]

@malchata I fixed the bug in supported browsers. When I was running through the tests, I noticed a race condition with autoplay videos that lazy load their sources. I fixed that too.

[malchata]

Hey, @radfahrer! Thank you so much for this. I'm reviewing this now and may have some small changes to make, but I think we're very close. I'll keep you posted.

[malchata]

I had to tweak a few things to get this to work with mutationobserver and fix a couple small things, but it's merged! 🎉 Thank you so much for sticking with me through this @radfahrer. Your patience is legendary. :)