Remove Cross Component References

[ranchodeluxe]

I would appreciate some feedback about the ideas I mention below and some of the solutions the commits tied to this ticket propose.

The App/Controller Idea

Originally the MapView class was called something like DncApp and it's general purpose was to glue and setup things:

• setup and bind components being used in DNC, like the map
• setup component event listeners such as the FileReader handlers
• it was a place for slim applciation logic, but not component specific stuff
• it was also the hub where components could reference and talk with each other

When we decided that MapView felt like it was it's own component, things got shuffled around and some glue stuff didn't make sense to have in that class anymore.

I've noticed that some of the glue logic was moved out into L.DNC where it currently lives. My guess is that we can expect that glue to grow in the future. L.DNC really feels like a simple namespace object for things to hang off, but not it's own class. Maybe that's just me.

More importantly, another side effect of the refactor was the propagation of some patterns that I feel like we should avoid:

• FileReader now reaches outside of it's scope globally to access other components here.
• The TurfOperation also reaches outside of it's scope to add layers here.
• Even more confusing, the FileReader component fires an event that it itself listens to here.

Proposal

It feels like we need a thing that does the glueing and setup again. This thing, lets call it a controller, would also hold component references so we don't force components that shouldn't know about each other to tightly reference each other.

What's Changed?

• I removed all cross references except one, which I flagged as a possible removal later.
• For lack of a better term ( i'm open to anything ), I called the controller AppController. See the class here.

[mapsam]

Sorry for the delay on this one @thebigspoon, was busy this past weekend! I'm in favor of this layout - though I was in favor of the last one as well so I think my opinion shouldn't hold too much weight here.

Just to keep my head straight, let me present a hypothetical scenario. If I want to add a "fileparsed" publication to a notification system that would subscribe to such events, would I add another line to the _addEventHandlers function with the following:

_addEventHandlers: function(){
    this.dropzone.fileReader.on( "fileparsed", this._createNotification.bind( this ) );
    // ... more handlers ...
} ,

and then add a new private handler function to deal with the notification information/type

// new notification handler that passes options into a create() function
_createNotification: function( e ) {
    this.notifications.create( stuff, anotherThing, etc ); 
} ,

??

[ranchodeluxe]

@svmatthews -- yep, that's exactly right

[mapsam]

Excellent. I think this is exactly what I was thinking about in my head but wasn't sure how to phrase it all. When we were talking about the pub/sub piece it seemed like there was something missing that ALL publications were sent to and all subscriptions listened to. This seems to properly handle that disconnect.

👍 from me

[alukach]

While we don't have to solve it all right now, can you give some hints in regards to what you're thinking about a refactor between the MenuBar, Menu, and Operation?

[ranchodeluxe]

I haven't looked at the relationship in depth enough to guess what needs to change now. But in particular it feels like the L.DNC.MenuBar and it's inlaws own the selection relationship more than L.DNC.LayerList does.

So a quick win in terms of this reference might be to have L.DNC.LayerList fire events for add and remove here and L.DNC.MenuBar and inlaws listen for those and hold the selection internally.

That's a quick guess shooting from the hip, so not sure if that would work.

[alukach]

More importantly, another side effect of the refactor was the propagation of some patterns that I feel like we should avoid:

• FileReader now reaches outside of it's scope globally to access other components here.
• The TurfOperation also reaches outside of it's scope to add layers here.
• Even more confusing, the FileReader component fires an event that it itself listens to here.

Yeah, most/all of these were my bad. The interlinking of dependencies felt wrong, I was pretty much shooting from the hip. 👍 regarding these changes.

To further understand, in the scenario described by @svmatthews, the .on() tooling comes from L.Mixin.Events, correct?

[ranchodeluxe]

@alukach: yep, .on() and .off() and all the aliases for them are from L.Mixin.Events.

@alukach and @svmatthews: when you guys come to a decision about this PR can you merge it in? I'm going to be on a beach for the next week.

[mapsam]

Let's merge this! I think my head is tied a little tighter with this PR and will be able to cruise on some functionality after it is merged.

[mapsam]

@thebigspoon is this going to be you next week? You ocean man, you.

[alukach]

Pulling the trigger on this one! 💥 🔫