Split the project on multiple files so it can be more maintenable

[tombatossals]

Hi guys, last days I was splitting the leaflet-directive webpage on multiple files, so it would be easy for us to add new examples to the demo webpage.

I was thinking about splitting the directive code in multiple files too, so it would be easy to maintain/add new features to the project.

The Leaflet project does something like this, making the project more object oriented.

Do you know which would be the best method to accomplish this in our directive? Ideally, we would structure the project like this:

src/core.js src/center.js src/controls.js src/layers.js src/bounds.js src/markers.js src/path.js

Grunt could concat all the files and finally generate the angular-leaflet-directive.min.js file. But it's very difficult to mantain the scope between files, I have to think about the structure a little more.

What do you thinks about this idea?

[tombatossals]

I'm taking a look at the google-maps-directive code, they are doing something similar on a new branch:

https://github.com/nlaplante/angular-google-maps/tree/r1-dev

They are splitting the project in multiple files, each of one is a directive by itself. Maybe i will try to adopt something similar in a new branch.

Please tell me what you think about this.

[JaumeFigueras]

+1 I think its a good idea. Also I have no idea on how to do it.

[houqp]

Yeah, we really need to split things up (including the tests).

But I am not sure about the multi-directive approach though. Firstly, it means totally API breakage. Secondly, directive is expensive, last time (2 months ago) I try using a LOT of directives on one page, I got a huge mem leak and user's browser exploded. Maybe it's doing gc better now ;p I haven't tested recently.

[tombatossals]

Hi, I'm making a lot of progress on this. Let me explain to you.

Like @houqp says, we need that the API does not break. First of all, I thought that the multi-directive approach would not fit our needs, but I have found a way.

We can define a "master directive", which will have children directives, each of these ones could access the parent controller methods. The children directives would be of the "attribute type", so it would mantain our current API.

I have ported to this schema some of our code and it's fitting like a charm, everything works like expected and the directive would be more maintenable and extensible.

Please, take a look at the code of the new branch I have created: https://github.com/tombatossals/angular-leaflet-directive/tree/newdev/src

There you will see these files:

src/main.js: Main directive common functions and directive variables needed. There I have defined some functions we need on the rest of the code like: "isNumber", "equals", "getMapDefaults", etc.

src/directives/map.js: This is the main directive code. Its responsability is to create the map object and make it available to the rest of the directives.

src/directives/center.js: This directive includes the code we had on "setupCenter", it has access to the "center" attribute of the HTML code, and can define its listeners on the "link" method.

src/directives/tiles.js src/directives/maxbounds.js .... -> The same as "center.js" but with its own responsability.

These examples are working perfect with the new code: simple-map-example.html center-example.html custom-parameters-example.html maxbounds-example.html

There is some work to do yet, new directives like "path", "markers", "events", "bounds", but everything is being adopted very easy from the original code.

I think this is a important step we need to give to our directive, if you agree I could do the adaption of all the code this week, re-work the tests splitting them in multiple files too, and if everything works like expected, we could move this branch to the master branch.

[houqp]

wow, very clever hack! It splits the directive up into multiple module while still maintains the scalability, i.e. only one directive for markers instead of multiple marker directives :)

Great work :+1:

Looking forward to have it merged into master and we should have a huge version bump :)

BTW: changelog has been outdated for a long time ;p

[tombatossals]

Thanks @houqp, i have been working on it today, and I have everything working on the new code, except some little functionality. I needed to look at the actual code because it has grown up a lot lately thanks to all of you. :)

• The markers directive is not migrated yet, I have left this for the last part, because its one of the most complex, but I think it will not be a problem.
• The paths functionality is not migrated yet.
• The "map object expose" functionality is not migrated yet.
• The tests are not re-worked yet.

The great news is that 80% of the directive is migrated to the new schema. It would help us a lot having the parts splitted up on little directives.

I would like to talk with you of some design doubts:

• Are you ok about having only one "markers" property? I think it would be more consistent to only have "markers" and give up "marker". It could be merged together in one directive, but I wanted to talk with you about this, what do you think?
• We have "events" property and "event-broadcast" property (it would be two different directives), but the code of this two features are mixed. It would be great to unify this on one property only, or segment the code. Maybe @grantlucas could give us some hint about how he would do it.

[houqp]

Are you ok about having only one "markers" property? I think it would be more consistent to only have "markers" and give up "marker". It could be merged together in one directive, but I wanted to talk with you about this, what do you think?

We had discussed on this before. My point is the code for handling marker and markers are almost the same. But in some of the real map applications, it's necessary to have a main marker for map interaction. Consider one use case of google map:

You can show multiple restaurants on the map using markers, but if you want to mark a new restaurant at the same time, you need a main marker to locate it on the map. This main marker is different from other markers because it's created for "write/create" purpose, not "read".

That said, it will be great if we can come up with a design that can unify marker and markers sub-directive code while addresses the above issue at the same time.

[grantlucas]

Hello! Sorry I haven't been able to chime in earlier. Have been out all day but have been reading the email notifications for this thread closely! :)

I'm all for merging markers and marker somehow. Would tidy things up.

As for the events and events-broadcast, I only did events-broadcast so that I wouldn't break existing code that was using events. If this can be simplified, I'm all for it. Is the current use of the "events" property still needed or can it be replaced with the new event code?

[grantlucas]

And overall, with this directive growing so much recently I'm all for splitting it up!!

[tombatossals]

Ok @houqp I catch you, let's mantain the "markers" and the "marker" separated if you consider it important this way. The two directives will need to share some code, but it will not be difficult to implement.

Thanks for the respone @grantlucas, so as you say we will mantain only "event-broadcast", the "events" property wasn't really needed.

There is something more that will be interesting (and easy) to implement, and that is a leafletService where every directive can register its information (the map object, the markers -> a marker array, the layers -> a layers and overlays array, etc.) so it can be queried by any controller simply by injecting this service.

Ah and yes, we need to update the Changelog a little. Lot of new changes but no explanation of them. Let's keep the Changelog updated from the next version.

I will keep you informed about the improvements, I expect to have everything working again before next weekend.

[houqp]

+1 for the leafletService idea, sounds even better, haha

[JaumeFigueras]

+1 +1 +1 to the service Idea!!

[tombatossals]

Hi guys, I have this refactor ready and working, it has been hard but I think it will be great for our development. Let me share my conclusions:

• Some attributes of our main directive are sub-directives, dependent of the main directive ("require" attribute on the directive). So for example:

<leaflet markers="markers" center="center"></leaflet>

This would have 3 directives called, which source code can be found here in the repository:

src/directives/leaflet.js
src/directives/center.js
src/directives/markers.js

• The Gruntfile concats and minify all the related files, so the final angular-leaflet-directive.min.js file is compatible with our last version.
• I have the examples, the tests (they needed re-work) and the web mainpage example working 100% with the new code. It would be great if you could tests the new code on your projects, because there are a lot of code changes, and maybe there are some little bug. It could be tested simply by copying the new angular-leaflet-directive.min.js into your project code.
• I have used some new features of Angular we haven't used before on the directive, like the "controller" and sub-directives, and the $q promises (very useful).
• We have a "leafletData" service which can be injected on a controller and have access to the map, the markers, the layers, etc. easily, it works but a little more coding is needed to have it 100%. Take a look at this because it can be very powerful.

src/services/leafletData.js

Now the bad news :P I had to remove some old code, don't feel bad if it was your code and you needed it for some reason. Let's talk about it because there are new ways of get things done more effective.

Finally, I know I am repetitive with this but I think that we need to re-think the "marker" and "markers" functionality. In the actual code, maintaining this means to have duplicate (and very extensive) code on two files:

src/directives/marker.js
src/directives/markers.js

I get @houqp point about having the "read-only" group of markers, and the "read-write" main marker, but I think this could be accomplished on the controller-side. Like for example having an structure of:

markers: {
   mainMarker: ...
   group1-marker1: ...  
   group1-marker2: ...  
   group1-marker3: ... 
} 

For example, and make the management of this on the controller, so we wouldn't need to mantain two separate directives. Maybe we can use also support another type of structure:

markers: {
   mainMarker: ...
   group1:  [
       marker1: ...  
       marker2: ...  
       marker3: ... 
    ]
} 

Please take a look at the actual code and you will see the problem I'm refering, marker.js and markers.js are almost the same. How do you think we can solve that?

And that's all! I'm very happy to accomplish this upgrade to our library, because later additions/modifications will be very easy to manage from now on.

If no major problems are found, I will merge the "newdev" branch with the "master" branch in 1 week or so, or when you think it's ready enough

Sorry for the large volume text

[houqp]

Nice work!

The grouping for marker sounds like a really good idea if it can be implemented efficiently! It not only solves the code redundant problem, but also makes it a lot easier to categorize markers and manipulate them in batch.

[grantlucas]

Awesome to see! I will give this a try this weekend with my project and see how it all goes. Thanks for the work on this @tombatossals.

[grantlucas]

Just wanted to update you that I'm currently giving the newdev branch a shot with my project. It'll be a few days in the time I have to get stuff working ironed out. Mainly need to get the tests for my project updated to use the new leafletData. That change alone is a hell of a change but I think it'll be great in the end. :smile: Just need to get familiar with it.

[tombatossals]

Great @grantlucas, like you say the changes are extensive, it could be painful rework all your custom test :P But we have modularized the project, and adding new features will be a breeze.

Please, give me one more week to finish it completely, I have almost all test working again (with the new leafletData service), I'm going to deprecate the "marker" property and merge its functionality with "markers", and if everything goes well I will put it online on the mainpage.

I expect you agree with the changes, there's a lot of code changed, but I'm already thinking about what can we accomplish with this, like easily adding leaflet plugins.

[grantlucas]

Great @grantlucas, like you say the changes are extensive, it could be painful rework all your custom test :P But we have modularized the project, and adding new features will be a breeze.

Hey no problem. My tests needed some reworking anyways now that I understand a bit more this whole Angular thing. :)

Please, give me one more week to finish it completely

The project I'm working on is still very much a work in progress so I'm in no rush. Just figured I'd give it a shot to see what issues I run in to.

I'm going to deprecate the "marker" property and merge its functionality with "markers", and if everything goes well I will put it online on the mainpage.

I expect you agree with the changes, there's a lot of code changed, but I'm already thinking about what can we accomplish with this, like easily adding leaflet plugins.

Yeah! I think this is a very positive upgrade overall. Sure there will be some bumps in the transition, there always will be, but we'll be much better off for it all afterwards.

[tombatossals]

I have finally merged the newdev branch into master.

There are things to implement and document yet, but I think it's good to continue development into the master branch. I'm open to suggestions and changes, but I think that this change (big one) was fundamental to continue evolving the project.

The "gh-pages" is updated with the new library and everything works as expected with minimal code changes. Basically, the changes are:

• Deprecated "marker" attributes, merged with "markers".
• Deprecated "events" attribute in favor of "event-broadcast" attribute.
• leafletData service which allows to access and inspect the leaflet map object, and others objects like layers, overlays, markers, tiles, etc.
• The "custom-controls" attribute has been removed becase I didn't know which was its function. We have to think about an easy way to add custom controls to the leaflet map defined by new <leaflet> attributes.

So that's all! I will continue with this development on the master branch.

[tombatossals]

I have tagged the last commit before the "newdev" merge as v0.6.1, in case you need to access the last old-cycle-development code.

Need to update the Changelog with the changes done.