Proposed change to widgetLoader

[tmcgee]

@DavidSpriggs, @gobsmack and others,

recent conversation here has introduced variations on the loading of widgets and new potential widgets. Techniques for widget loading could go a number of different directions but here's a relatively simple one that adds some flexibility to add custom widgets/override standard widgets while leaving the controller unmodified (ideally):

1) Add two new functions to the controller:

registerWidgetLoaders: function (loaders) {
    if (loaders) {
        for (var key in loaders) {
            if (loaders.hasOwnProperty(key)) {
                this.registerWidgetLoader(key, loaders[key]);
            }
        }
    }
},

registerWidgetLoader: function (key, loader) {
    this.widgetLoaders[key] = lang.hitch(this, loader);
}

2) change the current widgetLoaders variable to be a property of the controller and register them accordingly:

widgetLoaders: {},

controller.registerWidgetLoaders({
    scalebar: function(widgetConfig) {
        ...
}

3) allow custom widgetLoaders to be registered from the config in the initWidgets function:

initWidgets: function(evt) {
    this.registerWidgetLoaders(config.widgetLoaders);

    this.identify = new Identify({
       ...

4) add any widgetLoaders to the config:

widgetLoaders: {
    // custom widget
    find: function (widgetConfig, position) {
       ...
    },

    // override existing standard widget
    print: function (widgetConfig, position) {
       ...
    }
}

Hopefully the above snippets gets the gist across clear enough.

Thoughts or ideas? Close enough to submit a PR?

[DavidSpriggs]

This idea has potential. Basically moving the widget loader defs from the controller to the config.js. My concern is that the the config.js is getting rather large and complicated. We need to keep the non-developers in mind. They need to be able to open the config and not be intimidated by functions. To solve this, what if we simply take the widget loader out of the controller into its own class, or put the widget loader config out into its own separate config file? Thoughts? I'm thinking a class.

[tmcgee]

The concern for the unwieldy config is definitely valid. Perhaps a separate optional widgetconfig.js?

At least in my case, the config approach gives more flexibility for supporting non-developers. I am using this ConfigurableViewerJSAPI for several clients who want to configure not build. These clients are comfortable modifying a config file but far from coders. So my thoughts and their input are leading more into the single config or multiple configs for a web app. They have operated successfully this way with their apps (built by me and others) for many years. Obviously not the only way to do this but where I started from because of their comfort level.

The widget loader could be separated from the controller as a class though I am not proposing moving all the widget loaders to the config - only custom or override. I think there is still a valid reason for separation of "standard" widgets versus custom. Ideally, the core app - your code here with standard widgets - is stored in one place for all the client's apps with the config(s) and additional widgets isolated to a separate folder.

FYI: any and all widgets I build for clients on their dime will ideally be contributed back here and some become standard widgets. My clients have agreed with this. There may someday be something so custom that nobody but the client or I can make sense of it. I know, I know - if that's the case, I built it wrong! ;)

Thoughts?

[tmcgee]

Related to this, I think it would be useful to allow the option to include/exclude the baseMap, identify, geocoder and growler widgets individually from the map like you can do with the other mapWidgets and sidebarWidgets.

[DavidSpriggs]

So here is my current thought. I will totally revamp the widget loading so that all you need is the config.js and the current definition of a widget. In other words, we need a better standard around widgets, the types and definitions, then we can generalize the loader to handle just those types, rather than a separate loader definition for each widget. Proposed definition:

{
    widgetType: 'sidebar', // floating, sidebar, map
    widgetClass: 'gis/dijit/Print', // class file path
    include: true,
    title: 'Print',
    open: false,
    position: 5,
    domNodeId: 'homeButton', // if a map widget, the dom node id to place the widget, or null
    options: { //will be passed into the widget class
        map: true, //if you need a map refrence true, if not false
        serviceURL: 'http://sampleserver6.arcgisonline.com/...',
        copyrightText: 'Copyright 2014',
        authorText: 'Me',
        defaultTitle: 'Viewer Map',
        defaultFormat: 'PDF',
        defaultLayout: 'Letter ANSI A Landscape'
    }
}

This will allow the widget loader to have one function to create all widgets. Thoughts?

[tmcgee]

I see where you are headed with this and think it can work quite nicely. I took a quick look through the current widget loaders and did not immediately spot an instance where this would not work.

[gobsmack]

I agree with @tmcgee, inasmuch as we need a way to "add custom widgets/override standard widgets while leaving the controller unmodified (ideally)". By moving the widgetConfig out of the controller, it becomes something that's less disruptive to modify. But, I'm having trouble understanding how those code changes fit together.

How to pass configuration to widgets? I also agree with @DavidSpriggs that the config.js is getting too big, and less accessible to new users and nondevelopers. I'm party to blame for suggesting we put the basemap configuration there #67. If we add Identify, with it's popupConfigs, and then Finder configuration, it will be a total rat's nest.

Here's a different idea for "stacking" the configuration:

1. The widget should have it's own default setup in the widget file itself.
2. The widget will accept widgetOptions from config.js like it does now
3. The widget will mixin it's own options, the widgetOptions, and the customOptons. Configuration "stacking" is used in the ESRI boilerplate:

this.templateConfig = lang.mixin(defaultTemplateConfig, templateConfig);

It then becomes the job of the widget loader to pass the user's custom options to the widget. If I really wanted to specify some extensive options, I could:

1. Create a widgetOptions folder, and add basemap.js, identify.js, etc
2. Conditionally load the custom options when we load each widget
3. Pass the custom options into the widget when it's constructed Here's a brief example:

directions: function(widgetConfig, position) {
   var directionsTP = this._createTitlePaneWidget(widgetConfig.title, position, widgetConfig.open);
   require(['gis/dijit/Directions', 'customWidgetOptions/directions.js'], lang.hitch(this, function(Directions, customDirectionsOptions) {
      this.directionsWidget = new Directions({
         map: this.map,
         options: lang.mixin(widgetConfig.options, customDirectionsConfig),
         titlePane: directionsTP
      }, domConstruct.create('div')).placeAt(directionsTP.containerNode);
      this.directionsWidget.startup();
   }));
},

How to specify which widgets are in the application? I guess I'll be filling the contrarian role, today. I actually like the widget loader the way it is. I like the clarity and simplicity. It's easy for new developers to understand. It's a good example of using a conditional require. It's simple to comment out a widget that I'll never use. It's easy to add or override sidebar widgets from a "custom" widget folder. It allows me to put two widgets in one title pane. It allows me to hookup additional events, like we do with the measure widget. And, I can do something like add additional options, as described above.

So, loading widgets the way @DavidSpriggs is suggesting, with a widgetClass variable, wouldn't be my first choice (with respect). Outside of the AMD require, I don't like the idea of creating objects by a string class name. It reminds me of Activator.createInstance() from .NET, and that's like a poor-man's dependency injection. Also, could instantiating objects with widgetClass break the ArcGIS Web Optimizer??? I still can't get it to work, but I think the JSO scans for require statements, looking for modules to bundle.

[tmcgee]

@gobsmack, regarding your last paragraph: If I am envisioning it correctly, @DavidSpriggs is not intending to use the widgetClass string var to create a class. I was envisioning it as using the widgetClass for the requires in a generic widget loader. Something along these lines:

function widgetLoader(widgetConfig) {
    if (widgetConfig.widgetType = 'titlepane') { 
        var tp = this._createTitlePaneWidget(widgetConfig.title, position, widgetConfig.open);
        require([widgetConfig.widgetClass], lang.hitch(this, function(WidgetClass) {
            var widget = new WidgetClass(widgetConfig.options)
            }, domConstruct.create('div')).placeAt(tp.containerNode);
            widget.startup();
            this.widgets.push(widgets);
       }));
    }
});

That's off the top of my head and obviously incomplete. Hopefully it gets my idea across. David can confirm if that was his general direction.

Having separate config files for widgets is definitely better to one big config file. Putting all widget config files in one place and in the same "custom" or "config" folder as config.js would be my vote.

The current Identify widget handles it's own config file in a similar way to what you are suggesting. I just don't care much for the location of that config file. In the new approach, I would add a short config object for the Identify widget within config.js - just to handle the basics of letting the controller know there is an identify widget and that it should be included in this app. The actual definitions of identifies would be in the separate identify.js file. This same method can be used for basemaps, bookmarks, find queries.

There's more spaghetti on the wall. What's it look like now? ;)

[gobsmack]

I can get over my objection to pulling in dependencies based on strings. You can think of it as Inversion of Control, and the AMD loader is our IOC container. But, I still don't think that the JSO, or any other build tool, can crawl over that dependency for bundling. So, even if we bundle the app, each widget will be loaded with an async request. Oh well.

So, this widgetLoader looks good. What about the case of the measure widget, that has some events hooked up?

[rogers259]

Like the way this is going, need to see if we can get grouping multiple widgets together in one titlepane going on this. Its one thing I'm fighting in the current version and its just to messy.

[DavidSpriggs]

@tmcgee You are correct in your understanding of the proposed widget loader (ps - we will add widgetId in i just forgot that one).

@gobsmack If you think about it we are doing nothing different than what is already happening in standard AMD to load dependencies. Classes are loaded by strings: require(['esri/dijit/HomeButton']...) we are simply moving the storage of the string to a different place. Also as for doing a build, this viewer was not designed, and as you point out will not work with a build, because of its dynamic nature. If you want to do a full build on the app, then you need to refactor things load the classes in a hard coded manor that will support a build. At that point its no longer a configurable view, its a focused app. However, it would be interesting to create a grunt task that would take a config file, refactor the controller to a buildable app for the optimizer. Ahhh, if I only had more time in the day ;).

I will work on refactoring the widget loader and post it soon.

[DavidSpriggs]

Implimented in 3fa6e945bae6fae08ddecbe60e341bc5ba72f6a2