Mapael plugin architecture enhancement

[Indigo744]

Dear Neveldo,

If I may, I would like to suggest some internal change about the plugin architecture.

Right now, the plugins architecture, from top to bottom, is:

• (some processing for lazy loading and such)
• Creation of a function named Mapael:
  • accepts an Options parameters, and parse it
  • performs a huge processing on each elements targeted
• Listing of Mapael internal function Mapael.xxx() used all along
• Listing of default values Mapael.defaultOptions and so on
• Assign $.fn.mapael to Mapael for jQuery

The biggest problem here is the Mapael function, and the huge foreach processing inside of it.

I would like to suggest to implements a slightly different architecture pattern: https://github.com/jquery-boilerplate/jquery-patterns/blob/master/patterns/jquery.basic.plugin-boilerplate.js
The difference will not be so big actually. The main thing is that there will be an Init() function, and that we check against an already existing Mapael on the container.

What do you think?

[neveldo]

Hello,

Thank your for the proposal. What would the advantages of this new architecture ? I understand the advantage regarding the prevention of multiple instanciation of Mapael on the same container, but what would the advantage of moving the plugin initialization into an init function prototype and then instanciate a new object when calling mapael() on a container ?

[Indigo744]

On top of my head:

1. better code organization
2. true Object-Oriented approach with prototyping
3. easier for new comers to dive into the project

The first point is important. It allows to navigate in the code with ease. And I must say, sometime, I get a little lost when searching something. My dev environment list all internal functions, but not the whole each() part. So I use a lot of scrolling to go to the part I want.

[neveldo]

Thank for the explanation. I agree with you, we can easily be lost among all the internal functions. It's ok for me to refactor the plugin architecture in a better way. Feel free to make a PR if you have time, in any case, I will add it to my todos. Of course, feel free to ask if you have any question about the code.

[Indigo744]

I just submitted the first step for the new arch.

The next step will be to move the defaults option out of Mapael object (in order to take less space on each element).

Then, we will add a destroy event. It will allow both:

• instanciate a new mapael on an element with an existing mapael
• and fix a leaking problem (see Mapael: leaking problems #118)

Next, will be to create the init() function

Next will be to move from constructor pattern to prototype pattern.

[neveldo]

Hello,

All the steps sound good to me, I will review and merge the first one, thank you !

[Indigo744]

The steps needs some rework:

1. Move the defaults option out of Mapael object (in order to take less space on each element).
2. Store all private vars used globally (the ones in the this.each() function) inside Mapael object (using Mapael._xxx)
3. Create the init() function
4. Move from constructor pattern to prototype pattern
5. Add a destroy function and event

[Indigo744]

The user shall still be able to:

• override the default options
• override a plugin method

After some research, the solution is to:

• still use the "basic" pattern (https://github.com/jquery-boilerplate/jquery-patterns/blob/master/patterns/jquery.basic.plugin-boilerplate.js) with true prototype
• mix in the "namespace" pattern (https://github.com/jquery-boilerplate/jquery-patterns/blob/master/patterns/jquery.namespace.plugin-boilerplate.js)

At the end:

• there will be a Mapael prototype stored in $.mapael (and not an instance of mapael)
• there will be a DOM attachment method in $.fn.mapael (this will only be a wrapper to attach mapael to the element by creating a mapael instance)
• each object instance of Mapael will be stored in $("mycontainer").data().mapael

Hence, how to override ? Simple:

• override the default options: modify $.mapael.defaultOptions
• override a plugin method: modify $.mapael.tooltip

When creating a new instance, mapael will use what is defined in $.mapael.

[neveldo]

Hello,

I think this is a good architecture that provides a better code ogarnisation and user extension. But I wonder if it is a common practice to add sub-namespaces directly in the jQuery namespace ?

[Indigo744]

Actually, the $.fn.xxx should be use for DOM related element, and the $.xxx should be use for generic element. Hence this is not surprising to use both for our plugin (just like the jQuery Namespace pattern).
Actually, a lot of plugin would just create a global var mapael and let the user play with it.
Since this is a jQuery plugin, it makes sense to extends the jQuery object with mapael instead of a global var.

[neveldo]

Thank you for the explanation !

[Indigo744]

By the way, we will also need to modify all maps to extends $.mapael instead of $.fn.mapael.
But in a first time, I suggest extending both, so we can keep some retrocompatibility between version. What do you think?

[neveldo]

Hello,

Hmm, I didn't thought about that point, indeed it will bring a huge non-backward compatible change. I'm not fond of extending both objects in the maps files because it adds complexity, one of the two 'extends' will be useless, ...

Maybe we could :

1 / Add an UPGRADING.md file in the mapael repository. Its purpose should be to explain to the users how to migrate from the version 1.. to 2.. with all the non backward-compatible changes. Here is an example : https://github.com/guzzle/guzzle/blob/master/UPGRADING.md.

2/ Separate the mapael-maps repository into two branches : one for the maps compatible with mapael 1.x, the other for the maps compatible with mapael 2.x (the master one ?), and add clear information for users into the Readme.md.

What do you think about this proposal ?

Any way, even if the next release will be the version '2.0.0', I think we should avoid as far as possible to introduce non-backward compatible changes. The ideal way should be to let maps extends jquery.fn.mapael, but I think it will be complicated with the new architecture.

[Indigo744]

Your first point is mandatory! It is important to provide a clear guideline for users migrating from older version.

Maybe in the map file we can provide a little wrapper to detect the Mapael version and extend the correct one. This would easily be done and all the maps would be compatible with both version.

...

if ($.mapael !== undefined) {
    // mapael version >= 2.x
    Mapael = $.mapael;
} else {
    // mapael version <= 1.x
    Mapael = $.fn.mapael;
}

$.extend(true, Mapael, 
...

[neveldo]

This workaround seems to be straightforward enough to be added to the maps. However, the weakness is see is that we will not be able to remove it until the version 3.* as it will break the backward compatibility. Moreover, it doesn't solve the issue of users who will upgrade mapael from 1.* to 2.* and have custom maps that will not be compatible anymore.

Other proposal that could fix both issues : what do you think about adding the workaround in jquery.mapael.js instead of each map, and throw a warning console.warn() if the current map extends $.fn.mapael instead of $.mapael ?

[Indigo744]

You are right, mapael could check both $.mapael and $.fn.mapael and warn the user if extending the later.
Regarding the console, we should be careful with IE8 (http://caniuse.com/#feat=console-basic).
I suggest the following (taken from QUnit own source code):

if ( window.console && window.console.warn ) {
    window.console.warn("Extending $.fn.mapael is deprecated for a map.")
}

[neveldo]

Thank you, I wasn't aware of that point on IE8, I'm ok with this solution.

[Indigo744]

I think we can close this issue now :)