lazy creates/destroys scroll handlers even if there are no images left to load

[na-oma]

Hi, great plugin! It's nice not having to rely on other plugins' rudimentary lazy-load support.

We are using lazy together with https://flickity.metafizzy.co/ to scrolls through containers which contain lazy images (rather <picture>s).

Our setup (for ALL lazy images on the page, not only in flickity):

    $('.lazy').Lazy({
        effect: "fadeIn",
        effectTime: 500,
        threshold: 0
    });

Handling flickety scroll events potentially revealing lazy images (calls lazy for all lazy images in the whole flickety container):

$('.main-carousel').on('scroll.flickity', function (event, progress) {
    $(event.target).find('.lazy').lazy('update', {useThrottle: true});
});

It works like a charm, but is it even correct to instantiate lazy multiple times like this???

The question: When flickity fires a scroll event, and all lazy images in this container have already been loaded by lazy, lazy still creates and instantly destroys an instance (_lazyLoadItems() => instance.destroy() This is not a problem per se, but before lazy checks if there are any elements to handle, it already registers a scroll handler $(config.appendScroll).on('scroll.' + namespace + ' resize.' + namespace, events.e); and unbinds it in destroy: $(_config.appendScroll).off('.' + _namespace, _events.e); $(window).off('.' + _namespace); The unbinding (jQuery remove) is especially taxing: it takes 10% of cpu (chrome profiler, on desktop!) while scrolling.

Is it possible to alter the code to only bind/unbind events, when there are actually any items to be loaded?

Alternatives:

• Do our own throttling (since useThrottle does not do anything for instance creation/destruction) when calling lazy in scroll handler
• copy the internal lazy code to determine if there are any lazy images still to load (https://github.com/eisbehr-/jquery.lazy/blob/master/jquery.lazy.js#L274) and test for elements before calling lazy, but that's ugly
• Or remove the .lazy class at some point?
• somehow use autoDestroy to not trigger destroy

P.S. Another potential problem: In the same case(no elements to load), _lazyLoadItems triggers unbinding the events via instance.destroy() and only after the events are unbound, the new events are bound inside _initialize!

[dkern]

Because I'm not sure about your call, could you please update your code like below and tell me if this works better? Thanks you.

var instance = $('.lazy').Lazy({
    effect: "fadeIn",
    effectTime: 500,
    threshold: 0,
    chainable: false
});

$('.main-carousel').on('scroll.flickity', function() {
    instance.update(true);
});

[na-oma]

Sorry for the late response. I thought that getting a lazy instance via chainable: false and data("plugin_lazy") is exactly the same, but one is leaking memory rapidly and the other works:

1. Getting instance via chainable false :rage: :rage: :rage: produces memory leaks(50MB/min), maybe the instance is not properly cleaned up? and many unnecessary remove function calls. See Chrome Profile/Memory Codepen

$('#testcontainer .lazy').Lazy();

setInterval(() => {
  //leaks memory even if you don't call instance.update
  var instance = $('#testcontainer .lazy').Lazy({ chainable: false });
  instance.update(true);
}, 1);

2. Getting instance via plugin_lazy works :+1: Codepen

$('#testcontainer .lazy').Lazy();

setInterval(() => {
  var instance = $('#testcontainer .lazy').data("plugin_lazy");
  instance.update(true);
}, 1);

3. Saving global instance reference once works :+1: Codepen

var instance = $('#testcontainer .lazy').Lazy({
    chainable: false
});

setInterval(() => {
  instance.update(true);
}, 1);

Is there really such a difference between the methods 1 and 2?

Of course nobody would use setInterval(_, 1). But doing the call in a scroll handler generates the same amount of mem leaks and CPU waste.

[dkern]

Using chainable doesn't consume any memory. In fact, it should be a bist faster then using data too. The problem simply is, that you did it wrong. In your first example you create a new Lazy instance with all elements of the selector and update is directly.

I'll repeat: You create a whole new instance of the plugin every 1ms!!! What do you expect?!

To correct approach is your third example. As you see in my post above. Create an instance once and reuse it.

[na-oma]

I know I used it wrong, I got that now :) But there is still a memory leak when creating an instance with no element in the selector.

What do you expect?!

I expect the instances to be properly cleaned up, even if I create 1000s of them: auto destroy is true by default and there are no more images to be loaded.

In detail:

• _initialize calls _lazyLoadItems calls instance.destroy() (when there are no items) which deletes event listeners.
• But after _lazyLoadItems new events are bound via $(config.appendScroll).on('scroll.' + namespace + ' resize.' + namespace, events.e); in _initialize.
• They never get cleaned up.

Maybe I misunderstand what autoDestroy means. I expect it to clean the whole instance as if it was never created.