added update method

[RickHolmes]

This branch adds an update() method that resets the entire elements list and detaches any previously attached events, hopefully limiting memory leaks. One use case is with the creation/destruction of a modal popup or slider when the entire thing is created dynamically, not just displayed with existing elements.

I also added the ability to pass additional options at the same time.

[thom4parisot]

Hello @RickHolmes, thanks for your proposal :-)

Can you explain a bit the internals of attachedEvents? Also, considering there is an add() method now, I would suggest to refactor the code as much as possible, so as they would be sensibly similar – one adds elements (this.divs = this.divs.concat(...)), one replaces them (this.divs = ...).

I saw you renamed some internal properties (which are public, so maybe people rely on them): is there any specific reason to do so?

Thank you :-)

[RickHolmes]

In this particular use case for the Imager -- a single-page-application
-- I'm starting out with an empty

and adding the page content
dynamically. So I needed either an add() method or an update() method,
since there's nothing there for the Imager to do when the page loads.

From that page (a "gallery" page), however, a couple things may happen:
you could "navigate" to another "page" with the particulars of the
gallery item (small screens only) or you could open a modal containing
the particulars. Both of these choices are also created dynamically and
do not originally exist in the DOM. But, both of these choices will be
destroyed when you either "navigate" back to the gallery page or close
the modal. Using an add() method here would leave references to elements
that no longer exists, potentially causing memory leaks.

So I added an update() method that deletes the element list and rebuilds
it. It turned out the easiest way (for me, anyway) to do that is to
re-run the existing initialization scheme, which attached scroll and
resize events on the window. So they would build up. While that might
not cause any leaks, since the elements all seem to be 'window', it's
still probably best to get rid of any unnecessary ones. (I created the
"options" object as a global in order to have a reference to "opts" in
the update() method.)

In order to remove an event with either the W3C or old IE method, you
need the element that has the event, the event itself and the name of
the handler function, which in this case doesn't exist. I created an
object with a reference to "fn" and the rest and stored it in an array.
Then the update method can loop through the array and successfully
delete all the attached events.

I'm actually using another version of the Imager based on your Oct. 26,
2014 pull request that uses the strategy pattern. I rewrote the
ImageElementStrategy function to allow for a data-background-type
attribute. Then I can have both s and

s with background-images
using the same Imager instance. I'll upload it if you're interested.

On 1/13/2015 4:50 AM, Thomas Parisot wrote:

Hello @RickHolmes https://github.com/RickHolmes, thanks for your
proposal :-)

Can you explain a bit the internals of |attachedEvents|? Also,
considering there is an |add()| method now, I would suggest to
refactor the code as much as possible, so as they would be sensibly
similar – one adds elements (|this.divs = this.divs.concat(...)|), one
replaces them (|this.divs = ...|).

I saw you renamed some internal properties (which are public, so maybe
people rely on them): is there any specific reason to do so?

Thank you :-)

—
Reply to this email directly or view it on GitHub
#120 (comment).

[thom4parisot]

OK gotcha. Detached DOM Nodes can definitely be a pain.

I have not looked yet but do you know if there is a way to detect if a given HTMLNode element is detached or not by looking at its properties?

I would rather prefer a cleanup() method, called during update() rather than even more memory allocated to store objects references through the attachedEvents variable.

It would make the update method easier to refactor, as it would be a call to add() + cleanup(), whatever cleanup does (either through attachedEvents or detached DOM nodes filtering).

[RickHolmes]

You can find detached nodes the hard way by comparing this.divs against a list of the divs in the current DOM with this.className. Then you can remove those that currently don't exist from this.divs. I'm not sure there's an easier way; other methods I've found with a quick search check the node's parents or its html or something. And I'm not sure that would be any easier/less expensive than a direct comparison of the lists.

Edit: I take that back. In order to compare the lists, you'd need to compare the data-src attributes, which might be harder, since you'd need to build an array of the data-src's of this.divs, then compare the current.

[thom4parisot]

Well thinking about it, the thing is if you replace the actual this.div through the update() method, you are not supposed to have any detached DOM elements anymore. If the elements passed as an argument of the update() method are detached, it is another problem, and not Imager's one. Right?

[RickHolmes]

True, that's why I did that. But the way I implemented things, I had to reinitialize the Imager, which attached additional (duplicate) events on "window". If things can be done without the init() step, all would be good. Adding additional options probably isn't necessary, and deleting events would only make sense if you set scroll and resize events on DOM elements, not the window itself.

[thom4parisot]

Can you share a snippet of code which demonstrates the initial setup of Imager, and how you reinitialise it? I wonder why would such a thing is needed (if we can design a cleaner path let's do it).

[RickHolmes]

All the js is being loaded by require.js and Backbone.Marionette is being used to create the page. In my "data-main" file, I create a Marionette.Application and set the Imager as a property:

App.Imager = new Imager({
     availableWidths: [320, 480, 640, 826],
 availablePixelRatios: [1, 1.5, 2],
 lazyload: true
});

This gives me a single instance of the Imager that I call from various places as needed. For instance, in the main page view (the "gallery") I set a listener for "dom:refresh" and simply call

App.Imager.update();

Then, should a gallery item be clicked, I will rerun App.Imager.update() in a dom-refresh listener on the view for either the modal or the single-page (which is only shown for small screens). Changing the modal (or going "back" for small screens) runs App.Imager.update() again.

It turns out the this.init() in the update() method is only needed if lazy loading is desired.

[thom4parisot]

OK I see. So I would suggest to not care about the possible detached elements in the update() method and to rather kick in the event(s) related to the lazyload.

Calling init() a second time is triggering a side effect and we should rather focus on not having to call init() then, because the update() function does its job ;-)

Imager.prototype.scrollCheck = function (force) {
    // ...

    if (Boolean(force) || this.scrolled) {
        // ...
    }
};

Imager.prototype.update = function(elementsOrSelector) {
    this.divs = [];
    this.add(elementsOrSelector);

    this.lazyload && this.scrollCheck(true);
}

Something like that.

It is not generally a good practice to call a constructor/init function more than once – it induces side effects we are not generally looking forward to solve :-)

[RickHolmes]

Do you mean not care about the detached elements or not care about the added events?

There could be dozens of detached elements, since each modal would have at least two to as many as seven elements handled by the Imager. This would add up quickly.

I totally agree about not calling init() more than once. I'll see if I can "reset" the lazy load another way.

Edit: I tried your suggestion and it seems to work fine.

[thom4parisot]

Well, this.divs = [] removes every reference to tracked responsive images DOM elements. Which means if this array contained one or many detached DOM element, they are not references anymore.

Tell me if I am wrong – otherwise let me know why and how detached DOM nodes could still exist if we empty out the this.div variable :-)

[RickHolmes]

No, you're right, I missed that line and was thinking that add() just added the new. Sorry.

[thom4parisot]

@RickHolmes it's alright :-) if you could reflect the changes with the suggested direction, and confirm it works well within your single page application, I'd be happy to merge the change and to release a new version of Imager.

[RickHolmes]

Will do, probably a bit later today.

[thom4parisot]

Well, you are changing the codestyle of the project – I would not recommend to do that.

I'd prefer you to rever to the previous var statement style. Hoisting is fine though.
If you want to address code style open a new PR and enforce it through eslint/jscs rather than mixing it with a feature PR.

Thanks :-)