[READY] Constructor API

[thom4parisot]

The intention

Actual implementation of Imager registers events on the go and trigger image replacement immediately. IMO the constructor is doing too much things you can't replay later without calling numerous functions.

It also ships it own version of $. Every decent project including Imager might already have solved this problem (by including jQuery or something else).

The proposal

Explicit Elements Argument

By providing them with a pure DOM selection:

var els = document.getElementById('main').getElementsByClassName('delayed-image-load');
var imgr = new Imager(els);

By providing them from your own library:

var imgr = new Imager($('#main .delayed-image-load'));

For simplicity sake, a string selector can be provided, then using querySelectorAll internally:

var imgr = new Imager('#main .delayed-image-load');

Factory

The constructor instantiate the Imager instance while the factory instantiate and processes elements. We could have two possible syntax, each has its own pros and cons.

without new

var imgr = Imager('.delayed-image-load');

// equals to
// var imgr = new Imager('.delayed-image-load');
// img.changeDivsToEmptyImages();
// img.init();
// img.shallIDelayImageReplacement();

pros:

• no new static method name to remember
• shortest way of writing possible
• if you don't want the one liner way, you can fine tune it the way you want

cons:

• possibly confusing to remember which of new Imager() or Imager() I should use
• not very common pattern among JS projects so it might even be not so natural to use

create static method

var imgr = Imager.create('.delayed-image-load');

// equals to
// var imgr = new Imager('.delayed-image-load');
// img.changeDivsToEmptyImages();
// img.init();
// img.shallIDelayImageReplacement();

pros:

• explicitly different from the constructor
• if you don't want the one liner way, you can fine tune it the way you want
• people won't forget the new word and mess up the this context of Imager

cons:

• constructor vs factory debate really

[Integralist]

@oncletom

constructor is doing too much

...I totally agree. It should be simplified.

Every decent project including Imager might already have solved this problem (by including jQuery or something else)

...so just to be clear you're suggesting we should remove the need for a $ namespace and allow the developer to pass through whatever collection of elements they want (whether it be an Array or a HTMLCollection - both static and live variations). If so I like the idea of that (having Imager.js encapsulate the logic, as long as the internal implementation is kept simple and concise - want to avoid some ugly jQuery style logic going on behind the scenes).

In this instance I'd assume that we'd have something like (and this is just super quick pseudo code)...

if (typeof selector == 'string') {                                                                                                  
    // process String                                                                                                               
}                                                                                                                                   
else if (Object.prototype.toString.call(selector) === '[object Array]') {                                                           
    // process Array                                                                                                                
}                                                                                                                                   
else {                                                                                                                              
    // process HTMLCollection                                                                                                       
}

...effectively we'd just abstract this away into a separate method but you get the idea.

With regards to using a Factory to create a new Imager instance (regardless of whether we go the Constructor route or the Static method route): I'm not sure if we need a Factory or not, would we not want just one instance of Imager on the page?

Could you explain why we'd need more than one instance of Imager so I can better understand the reasoning for using a Factory?

If you can explain the need for a Factory I would prefer the Static method route as it feels more natural for when using a Factory.

[Integralist]

@oncletom I think I've just realised the need for multiple Imager instances: '#main .delayed-image-load' suggests that the user could lock down the loading of images using Imager to just a specific container. I assume this proposal includes that particular feature and not just the optimising of the Constructor API

[thom4parisot]

Yes you got the point. If I have a website loading Flickr picture and in-house picture, I might want the same features but not the same way:

var imgr = new Imager('#main .delayed-image-load');
var flickrImgr = new Imager('article img[src*="farm.flickr.com"]', { availableWidths: […] });

That's why it motivates me to propose the elements collection as a mandatory argument rather than an implicit query beside that.

Plus, if the user has a lazyload plugin, he might want to combine the usage of both libraries seamlessly. Everybody keeps control of what they target :-)

The factory function would be a way to setup events by default for example, but well, it could be rather a configuration option which makes things simpler in the end. So let's forget about the Factory and stick to the regular constructor :-)

[Integralist]

@oncletom OK that makes sense.

So, we'll enforce the elements collection so it becomes mandatory.

Regarding your comment on lazy loading, I implemented a form of that recently into Imager.js so I'm wondering whether this is something you think should be handled separately (i.e. the user implements this or loads a 3rd party library that handles it) rather than Imager incorporating it?

I personally think it would be great if Imager.js kept lazy loading internal (so it was an easy option for users to enable without too much overhead). But maybe we'll revisit it after the other proposals have been nailed down and implemented (as it may be that my implementation of lazy loading would need to be changed).

[thom4parisot]

@Integralist it is ready :-) I've even kept the initial syntax, without any parameter.

[Integralist]

@oncletom just one minor issue I've commented on in this PR

[thom4parisot]

@Integralist thanks for taking some Sunday time to review the code 👍 I'll apply the minor fixes on Monday morning (and I'll be in BH for some newsHack lunchtime demo so I'll stay over the afternoon, available to chat and give a hand :-))

[thom4parisot]

@Integralist I gave another attempt for a generic code copying the array values. I've headed this way because we can refactor some bit of codes iterating on data with it to save more bytes (like determineAppropriateResolution, checkImagesNeedReplacing and changeDivsToEmptyImages)

[Integralist]

@oncletom hmm, ok :-) so we still return an Array (like the Array#slice trick does) but in your applyEach function when you return the Array, each Array item is a value (which results from passing a value to returnFn).

So what's the point of executing the returnFn and passing it the item and then storing that back into new_collection? All that function does is just return the value right back again?

Could we not just do...

new_collection[i] = collection[i];

...or is this because you want the applyEach to be as flexible as possible (e.g. allow the dev to pass through a function - which unlike returnFn - actually does something with the value before returning it).

[thom4parisot]

I did that essentially because we can't use the Array.prototype.slice.call as it does not work on IE8. Right?

So instead of making a one time use creating an array from a NodeList, I made a reusable applyEach with a callback. Which in this case is simply returning a value. But in the case of other parts of the code can do more things with an array item (acting as forEach without having Array.prototype.forEach)

[thom4parisot]

Rebased and fixed the naming of returnFn.

[Integralist]

@oncletom this looks good, but I can't automatically merge at the moment. I assume you'll need to rebase from BBC-News:refactoring-base

[thom4parisot]

Done :-)