cmv / cmv-app

CMV - The Configurable Map Viewer - A community supported open source mapping framework built with the Esri JavaScript API and the Dojo Toolkit
https://demo.cmv.io/
MIT License
325 stars 278 forks source link

Straw-man proposal for revised widget config management #86

Closed tmcgee closed 10 years ago

tmcgee commented 10 years ago

There have been several discussions about where to maintain the configuration options for widgets while making the config.js file easier to manage. For history refer to issues #65, #67 and #69 (and possibly other issues).

Here's a relatively simple solution: Add a new optional parameter to the config object called "configPath". Here is an example to demonstrate.

The config for the new Find widget I recently committed looks like this:

find: {
    include: true,
    id: 'find',
    type: 'titlePane',
    path: 'gis/dijit/Find',
    title: 'Find',
    open: false,
    position: 3,
    options: {
        map: true,
        queries: [
            {
                description: 'Find A Public Safety Location By Name',
                url: 'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/PublicSafety/PublicSafetyOperationalLayers/MapServer',
                layerIds: [1,2,3,4,5,6,7],
                searchFields: ['FDNAME, PDNAME', 'NAME', 'RESNAME'],
                minChars: 2
            },
            {
                description: 'Find Incident By Code/Description',
                url: 'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/PublicSafety/PublicSafetyOperationalLayers/MapServer',
                layerIds: [15,17,18],
                searchFields: ['FCODE','DESCRIPTION'],
                minChars: 4
            }
        ]
    }
},

To simplify, I can add a configPath so the config object now looks like this:

find: {
    include: true,
    id: 'find',
    type: 'titlePane',
    path: 'gis/dijit/Find',
    title: 'Find',
    open: false,
    position: 3,
    configPath: 'viewer/findconfig',
    options: {
        map: true
    }
},

Much easier to read/follow, especially when you have many widgets each with their own config. Then the new viewer/findconfig.js file looks like this:

define({
    queries: [
        {
            description: 'Find A Public Safety Location By Name',
            url: 'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/PublicSafety/PublicSafetyOperationalLayers/MapServer',
            layerIds: [1,2,3,4,5,6,7],
            searchFields: ['FDNAME, PDNAME', 'NAME', 'RESNAME'],
            minChars: 2
        },
        {
            description: 'Find Incident By Code/Description',
            url: 'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/PublicSafety/PublicSafetyOperationalLayers/MapServer',
            layerIds: [15,17,18],
            searchFields: ['FCODE','DESCRIPTION'],
            minChars: 4
        }
    ]
});

The existing config.js for the identify widget looks much like this one.

The widgetLoader function in Controller.js would then mixin the 2 config objects from config.js and the separate config so the options would appear and behave like my original example. As a proof of concept, I have a revised version of the widgetLoader function working locally (simplified it as well). I won't include it all here but can if necessary to continue the discussion

To adopt this approach, I believe that only 3 widgets would need to be updated:

With those widgets modified, all widget configuration would be handled in consistent and an easily documented fashion. @gobsmack has already done the bulk of the work on the basemap widget config in PR #67.

As I mentioned early on, the "configPath" parameter is optional. All of the options could be maintained in config.js as in my first example above If a developer/user chose to do so.

One additional advantage of having the configPath parameter is it is easier to find where the config options for a various widget might be located. Early on in my exploration of the ConfigurableViewer, I had to search through the code for various widgets (like identify) to see where they were configured.

OK. The straw-man has been propped up. Kick him around a bit and let's hear your thoughts.

tmcgee commented 10 years ago

FWIW: In my project, I have modified the 3 widgets mentioned to use this approach. Very minor changes and the app behaves as it always did.

Also, I wrote:

configPath: 'viewer/findconfig',

that was for simplicity. The path can obviously be changed to something like:

configPath: 'config/find',

or

configPath: 'widgets/find',

I think that any of these would be fairly self-explanatory for new developers/users.

The dojoConfig var in index.html would need to reflect the additional path(s)

DavidSpriggs commented 10 years ago

@tmcgee Thanks for starting this discussion. I think the cleanest approach is going to be to have the config.js for each widget in that widgets folder and then simply load the config in the widget. Identify has this approach and I think it works well. This prevents you from needing a configPath option and the module path in index.html, and it organizes the config into the folder of the widget that it belongs to, which makes sense. This can all be documented in the wiki. Is there a use case that I'm missing with the above approach?

tmcgee commented 10 years ago

What I am wanting to do is separate the repository of widgets from any configuration of widgets used by an application. With the current approach for Identify (and by BaseMap and Bookmarks though done differently), I cannot have a directory of widgets re-used by multiple applications. The entire widget directory must be duplicated for each app. This is for the benefit of my clients using this configurable viewer. The goal is to make it easier for them to manage/configure with a methodology that they have been using successfully for a long time with my apps and those created by others.

tmcgee commented 10 years ago

One quick additional thought - the configPath approach can support the config.js in the widget's folder as it is today - just point to it in the configPath. That can even be the default. And if my users want it somewhere else, they can do that. Additional benefit is the location of that config file is documented right within the main config.js.

msereda commented 10 years ago

On first glance, I don't like splitting out the configs like that at all. I understand and agree with the general idea of not wanting to have one gigantic config that contains everything, but on the other hand, I look at something like URLs. I have different URLs in Dev, Test & Production as I'm hitting different ArcGIS Servers in our environment. Therefore, if all of these URLs are split out into different files, I need to have different DEV/TST/PRD files for every single config.

If you had something like 'baseUrls' in the main config that then concatenated with the other config URLs, perhaps that'd get around that issue? I'd have to think about that one some more - I'm looking at something like:

baseAGSUrl: 'http://my-dev-server' ... url: '/ArcGIS/rest/services/PublicSafety/PublicSafetyOperationalLayers/MapServer' ... Not sure if I'm making sense in my example or not. I'm sure I'm not the only one who has to deal with multiple environments though.
tmcgee commented 10 years ago

@msereda Thanks for your thoughts. I think you and I are on a similar wavelength. The way I intend this and how I have my test widgetLoader written, "configPath" is optional. Basically, if you use configPath, it will mixin the options from the separate config file into the widget options already in config.js.

If you want to have everything in a single config as you suggest, you can (and that's how I would do it in my dev environment).

If others (like my clients) prefer to have them split up into multiple configs, they can do that as well.

Hope that makes sense.

msereda commented 10 years ago

@tmcgee Sorry, didn't get to read it all at first - it was just 'first glance', ha ha. Now that I'm home and able to read the rest, yes, looks good to me.

friendde commented 10 years ago

I'm on the fence on this one. I like the original file structure David set up for the Identify widget. As a non-programmer hacking & chopping up this code, it was easy to find and modify. If all widgets follow that structure it keeps all the config files in their respective widget folder.

But I also like the configPath option as it does allow for flexibility to organize your own version of the project. For my own readability of the code I rename each config.js to include their respective widget name, such as configViewer.js, configIdentify.js. It helps me read the code and if I did move them into one directory they will stack in alpha order when I go look at them in Windows Explorer.

It was mentioned that the configPath option would allow app admins to re-use the widgets in other projects because he/she can place them in a shared directory and call them by code. That sounds very handy indeed. But wouldn't the Admin need to also copy the Viewer.js and Controller.js into that other project as well? It seems to me that's the most important part of David's project. So if indeed the Admin has to copy those two files into another project, why not just copy the whole structure as David originally intended anyway?

Pardon my lack of knowledge & skills, I know I am in the company of talented programmers. I'm still trying to get my head around all of the brackets and parentheses and function calls.

tmcgee commented 10 years ago

@friendde regarding you question "But wouldn't the Admin need to also copy the Viewer.js and Controller.js into that other project as well?"

Ease of configuration is the most important part of David's project and my intent is to strengthen that with this proposed change. The config/controller JS files you mention and any additional separate widget configs (if used) would be the ONLY JS files needed within an application's folder hierarchy. All the other JS files (dbootstrap and gis) could be located in a common directory and referenced by all applications. The only change to an application would be to the referenced dojoConfig packages in the index.html. Something like this:

var dojoConfig = {
    async: true,
    packages: [{
        name: 'viewer',
            location: location.pathname.replace(/[^\/]+$/, '') + 'js/viewer'
        },{
        name: 'gis',
            location: '/common/js/gis'
        },{
        name: 'dbootstrap',
            location: '/common/js/dbootstrap'
    }]
};

P.S. controller.js is becoming more generic with less and less customization required. Soon it might conceivably be truly generic and shared by all applications as well.

I hope these further details help explain what I am proposing.

DavidSpriggs commented 10 years ago

Good discussion here. I think we can achieve both outcomes but need to propose/come up with something simpler. I totally get Tim's point but also need to keep in mind the non-dev people that may be confused.

Proposal: Perhaps all configs are stored in one location like so:

var dojoConfig = {
    async: true,
    packages: [{
        name: 'viewer',
            location: location.pathname.replace(/[^\/]+$/, '') + 'js/viewer'
        },{
        name: 'gis',
            location: '/common/js/gis'
        },{
        name: 'dbootstrap',
            location: '/common/js/dbootstrap'
        },{
        name: 'configs',
        location: 'js/configs'
    }]
};

The above location would be the default for the app and most users would use this location, making complete copies of the app when deploying. The configs would be named like: confg-viewer.js, config-basemaps.js, config-find.js, etc. For advanced users you could simply point this location to any path of your choosing and the location would contain all the configs you needed for a particular app or deployment.

Thoughts?

tmcgee commented 10 years ago

That structure certainly works for me. A couple of comments/questions:

  1. If all the configs are in a single js/configs folder, then each file doesn't need "config-' in front of it. So the naming convention could be simplified as viewer.js 'basemaps.js', find.js, etc. matching the widget id.
  2. Are you thinking that each widget would:
    • find its config relative to its location like the identify widget does now. I believe this forces the widget config to always be in a separate file and must exist for dojo's require to find it.

OR

This second option is what I was proposing.

friendde commented 10 years ago

@tmcgee thanks for clarifying. I appreciate that all contributors respect the original intent that David wants it to be flexible for advanced programmers and simple enough for anyone to deploy. I look forward to the generic Controller. I forsee a customer facing app with limited features and an internal app for Engineers, Crews, Dispatchers, and Managers.

Big Thanks to you and David and all contributors for sharing your knowledge and skills with everyone. Its exciting to watch this evolve everyday. This is the most useful, feature packed, extensible app I have seen related to Esri products. I started back in ArcINFO AML days, so I've seen a lot of atttempts.

DavidSpriggs commented 10 years ago

@friendde Thanks for the kind words. I too am enjoying the community participation and the contributions have been fantastic.

@tmcgee Thanks for clarifying. I think we can make this work as follows:

New Proposal:

  1. In the widgets section of viewer/config.js the options key can either be an object or a string (string needs to be the location of the config file).
  2. If options is an object we simply use it to create the widget, as things currently function.
  3. If options is a string the widget loader will then load this config when it loads the widget class and use the loaded config to create the widget.

I think this is the best approach and will work well for both use cases. Thoughts?

tmcgee commented 10 years ago

@DavidSpriggs Yes, I like this. I had been contemplating this morning how to make this simpler/clearer and was about to propose the same thing. I have 99% of this working already as a test so I can create the PR when we're all done (and think we almost are).

@gobsmack You had weighed in earlier with some good thoughts on a couple of issues related to widget configuration. Based on your use of bower and your other thoughts, my sense is you may have a slightly different perspective than David, myself and others participating here. I am pretty sure that where we are ending up will support your needs as well but if there's anything you want to add to the discussion, please do.

gobsmack commented 10 years ago

@tmcgee Sorry, I've been out of the loop for most of this. I got the beta for the WebApp Builder, and that's consumed my time. Unfortunately, the WebApp Builder won't suit my needs, and probably won't suit the needs of some others. That's a different discussion we should have, though. Understanding the WAB strengths and limitations would benefit developers on this project.

One limitation of the WAB is directly related to the discussion of widget configs. The WAB allows you to pass to the constructor a config object, or it will look in the widget folder for a widget config. This would be great, except passing the config object is a "hack" for developers, and not a supported configuration. The WAB GUI will actually move your config object back out to it's own file. For people like us who want to have multiple configurations per deployment, storing the configurations in the widget folder is less suitable than passing an object into the constructor.

So, I am strongly advocating for @DavidSpriggs solution, which gives us that flexibility. It's gives the non-developer the flexibility to have a clean config. But, it enables the developers to have a massive configuration file that configures all of the widgets.

tmcgee commented 10 years ago

@gobsmack thanks for the input on direction and absolutely no need for apologies!

On the WebApp Builder beta, your takeaway is similar to mine. I presented this Configurable Viewer and the WAB to a couple of clients. They reviewed the beta too and this project was chosen for us to move forward. I'll continue to stay in tune with the WAB as it evolves and advise clients accordingly. I fully agree that our takeaways from the WAB will benefit this project.

Back to widget configuration... I mentioned earlier that I have a version running locally with the approach we brought together here using an object or a string for the widget's options. To test, I converted the 3 widgets that used stand-alone options (basemap, bookmarks, identify) and use a config directory which can be pointed anywhere in the dojoConfig. My config directory currently looks like this:

I did this to mimic a client's use case (non-developer). I ran through it with that client and he likes the separation. I'll submit a PR after @DavidSpriggs is back from vacation so he's bombarded just a little bit less when he returns.

Thanks again to all who weighed in here. Hopefully I've captured most if not all of that contribution in the version I'll submit.

DavidSpriggs commented 10 years ago

@gobsmack Thanks for your input. @tmcgee Lets see that PR!

tmcgee commented 10 years ago

Will do.

tmcgee commented 10 years ago

PR #104 submitted.