Factor Out MapLoader

[gobsmack]

I've created a fork, done some work, and before submitting a PR, I'd like to gauge the willingness of the project to go in this direction. Rather than having the logic to load the map in the Controller, I'd like to delegate that to a separate class -- the DefaultMapLoader.

Here's the repo, minimal changes:
https://github.com/gobsmack/cmv-app

Why? Because I've got some requirements that are not really compatible with the CMV ethos -- (custom json, webmap json, and AGOL webmap). Having the separate MapLoader allows for a safer forked deployment. Rather than changing code in the Controller, I can just drop in my CustomMapLoader. This does NOT pollute CMV with AGOL, but DOES allow CMV be used with other map config specifications.

[friendde]

Interesting concept. Looking forward to reading comments from @DavidSpriggs @tmcgee and other contributors. I'll try your fork next week.

[gobsmack]

In frameworks like ASP MVC or Java Struts, it's called "dependency injection". The Controller just accepts an interface to a dependency, rather than the dependency itself. That allows you to 1) swap the dependency without changing code, and 2) test each component in isolation from the other. You usually see this in databases or loggers. In the tests, you use "mocks" of the dependency. In our case, we could test Controller with just a mock of IMapLoader.

Extending the idea further, it would be interesting to see an IConfigLoader to allow getting the config from a database. Or, IWidgetLoader, to allow different widget loaders like the one in @DavidSpriggs dojo-sidebar project. Or, an IErrorHandler to replace the HandleError function. Different implementations of IErrorHandler would be log to Growler, log to console, log to AJAX, or log to SignalR. We'd always want to provide a simple "Default" for the dependency so nondevelopers can get CMV deployed with no configuration.

Lots of possibilities, but, for now I only have a concrete need for the IMapLoader, and I don't want to overreach.

[btfou]

@gobsmack Ideas and PRs are always welcome. FYI...agol web map loading is on it's way. https://github.com/cmv/cmv-hoser

[jebu75]

I think this is a great idea!

I've also experimented with different options for breaking the Controller class down into separate classes with clear responsibilities. I think besides being able to swap out the default for your own implementation this approach also benefits to code maintenance and readability, and testing.

One concern that has been brought up is that with separate classes, latency becomes more of an issue.

Any thoughts on that?

[gobsmack]

@btfou I had seen that repo the other day!!! But, since it was empty, I didn't see the direction. I had assumed that it was a one-time conversion, some kind of a batch file. If you're saying that it will plug-in to CMV to load the webmap, then that's a bid deal. I'd like to contribute to that, since I've been parsing webmap json and id into our custom format for a long time. The two "curve balls" in there are that webmap has a "basemap" element, and that the CMV MapLoader derives four widget configs while reading the config. Easy to workaround though.

[gobsmack]

@jebu75 You are absolutely right for two reasons.

First, each dependency that we load will be an additional AMD call to download a javascript file. One or two more files wouldn't be the end of the world, but it's a slippery slope. ESRI Web App Builder downloads hundreds of files, and it's part of the reason we're getting off THAT train. But, if CMV uses esri-slurp to build into a single file, it would solve that. See here: http://gis.utah.gov/grunt-esri-slurp/. I've never used it myself, but it's high on my list of things to try.

Second, loading the map would have to become an async task. Currently, the config is loaded via AMD and parsed. But, getting the json any other way (id or database) requires an ajax call. We have to await that completion before calling initWidgets. I handed that in my repo. Let's figure out how to ensure that the DefaultMapLoader would not be any less snappy that it is now.

[DavidSpriggs]

Good discussion. I like where this is heading. We have been talking about doing this for awhile now, I appreciate you voicing the need for it.

As for slurp, yes, I use it on another project and it works well. Bringing it to CMV raises the bar quite high for the average user. It would have to be an optional feature.

[tmcgee]

Mixins including one for the map (modeled along the sampe lines as @gobsmack 's fork) was added in PR #475