Esri / application-boilerplate-3x-js

Starter application that simplifies the process of building templates for the ArcGIS.com template gallery.
https://developers.arcgis.com/javascript/
Apache License 2.0
90 stars 68 forks source link

4x enh: strong decoupling between boilerplate and app that uses it #85

Closed MikeTschudi closed 7 years ago

MikeTschudi commented 7 years ago

The boilerplate main program does necessary app setup (setting direction, creating the webmap/webscene/group gallery), but is subject to change as the JSAPI evolves. If the boilerplate's main file includes the app's customizations, it can be difficult to re-graft an app onto the updated boilerplate--a careful line-by-line merge is required.

The proposed enhancement separates the main file into two main files:

The boilerplate main would be coupled as little as possible with the app's main; so far, a single entrypoint has been sufficient for a 3D 4.x app: a run function that's called after the dom, map (WebMap or WebScene), and view (MapView or SceneView) are ready and resolved; this function is given the boilerplate's config object, the map, and the view. (I've not explored what the run function should be like for group apps.)

Two other entrypoints might be useful. For both, if the app's main provides the function, it's called; otherwise, the boilerplate runs as it does now.

It does not appear to be necessary to add the i18n resources to the config object or the run() signature: the app's main file can simply include the i18n invocation without incurring another network fetch because the boilerplate's main program has already loaded the i18n resources.

With this setup, if the boilerplate very rarely changes the contract between the two main files, it becomes possible to re-graft an app onto an updated boilerplate in the time it takes to copy files.

driskull commented 7 years ago

hey @MikeTschudi that makes sense.

I think we just need to figure out how it works with group apps too. We still want the creating of webmaps and webscenes to be able to be modified so users can tweak things and still modify config values before they are created but at the same time not have to worry about major merge conflicts in the future.

driskull commented 7 years ago

@MikeTschudi do you want to try and propose a PR to handle this and we can discuss code in it?

alaframboise commented 7 years ago

Hey guys, I have a suggestion. The way init.js is right now means that we have to wait until the view is ready before I can do the rest of the UI stuff in main.js or I just have to customize init.js directly (which I would rather not). My template apps all need to handle the creation of the map/scene, view, components... so instead of creating the view in the init.js, what about just passing the portItem and view settings to the main.js and let me set the view, map and app there?

This way I won't need to wait for the view and I don't need to customize init.js. e.g.

init.js

...
      if (webmap) {
        var viewProperties = {
          map: webmap,
          container: this.settings.webmap.containerId
        };

        // Override item with config setings
        if (!this.config.title && webmap.portalItem && webmap.portalItem.title) {
           this.config.title = webmap.portalItem.title;
        }

       lang.mixin(viewProperties, this.urlParamHelper.getViewProperties(this.config));

       // Launch the app specifics (immediately)
       this.main.start({
          settings: this.settings,
          config: this.config,
          viewProperties: viewProperties,
          webMap: webmap  // contains portItem
       });
...

main.js

...
   start: function(options) {
         // Setup UI (HTML/CSS) - no waiting for view...
         this._setAppUI(options.config);

        // Create view and components
        this._setView(options.viewProperties);

        // Do other stuff not that view is created
        this.view.then(function (response) {
          domClass.remove(document.body, CSS.loading);
       }
...

If you agree I'll put in a PR.

alaframboise commented 7 years ago

So here's what my main.js looks like now. All UI and view creation happens here and there's no delay (for the UI) - minus the portalItem fetch of course.

    //--------------------------------------------------------------------------
    //
    //  Variables
    //
    //--------------------------------------------------------------------------

    _settings: null,
    _config: null,
    _viewProperties: null,
    _webmap: null,
    _webscene: null,

    _activeView: null,
    _searchWidget: null,

    //--------------------------------------------------------------------------
    //
    //  Public Methods
    //
    //--------------------------------------------------------------------------

    start: function (options) {

      // App
      this._initialize(options);

      // Elements
      this._setDocumentTitle();

      // UI
      this._setAppUI();

      // Views
      this._setView();
      this._setViewEvents();

      // Panels
      this._setPanelEvents();
      this._setBasemapEvents();

      // Widgets
      this._setLegendWidget();
      this._setSearchWidget();
      this._setPopupEvents();
    },

    //--------------------------------------------------------------------------
    //
    //  Private Methods
    //
    //--------------------------------------------------------------------------

    _initialize: function(options) {
      this._settings = options.settings;
      this._config = options.config;
      this._viewProperties = options.viewProperties;
      this._webmap = options.webMap;
      this._webscene = options.webScene;
    },

    // Document

    _setDocumentTitle: function() {
      document.title = this.config.title;
    },

    _setAppUI: function(portalItem) {
      this._setMenuUI();
      this._setTitleUI();
      this._setSubTitleUI();
      this._setAboutUI();
    },
...
driskull commented 7 years ago

I'm still not sold on the init.js vs main.js. It would be best if we could just put what we think will be common to all apps into the Boilerplate.js. If that means a public method to create a webmap or webscene then we can do that. But currently the point of init.js isnt clear.

alaframboise commented 7 years ago

@driskull I agree. I also don't understand is why index.html creates an init (called application) and init.js creates a main... And they have back-references...

To me, the boilerplate.js should return a portalItem (with settings.json applied) and the app main.js or app.js should set the view with the user settings overrides (config.json).

<script type="text/javascript">

    var app;

    require([
      "boilerplate",
      "dojo/text!boilerplate/settings.json",
      "dojo/text!boilerplate/config.json",
      "application"
    ], function (
      Boilerplate,
      boilerplateSettings,
      configSettings,
      applicationMain
    ) {

      // create my main application. Start placing your logic in the main.js file.
      ///var mainApp = new applicationMain();

      // create the template. This will take care of all the logic required for template applications. If unsuccessful, we're calling error on main. Otherwise, we call startup on main.
      //new Boilerplate(JSON.parse(boilerplateSettings)).then(mainApp.init.bind(mainApp), mainApp.reportError.bind(mainApp));

     // Get portal item
     var portalItem = new Boilerplate(JSON.parse(boilerplateSettings)));

     // Build the view with updated user settings
     app = new app(portalItem, configSettings);

    });

  </script>
driskull commented 7 years ago

@MikeTschudi @alaframboise I removed init.js and brought back just main.js. If there is something common throughout all apps we can move it into the boilerplate. Otherwise, anything in main is fair game to change and should not be counted on not changing. If we want to move some functions into another helper class like viewHelper or something for the creation of a view from a webmap or webscene we can do that.

MikeTschudi commented 7 years ago

The problem that this request was attempting to solve is that the content in init.js (now main.js) tends to change over time, and to change in massive ways, making it risky and challenging to re-graft an app onto an updated boilerplate. If the current boilerplate main.js really, really, really will not change for the next year or more, then I agree that this is a better way to go. But history shows that it needs to change as the API updates what's involved in creating webmaps and webscenes.

@alaframboise, note that the earlier main.js had an optional hook that was called before the map or scene was created, so it wasn't necessary to modify init.js

@driskull @kellyhutchins

driskull commented 7 years ago

Yeah, but the problem is there's nothing common about main.js. Apps may want to totally customize the reporting page, the creation of webmaps, webscenes and views. They essentially need to be exposed to a user for customization.

driskull commented 7 years ago

We could have helper utilities to create these and a user could choose to not use them if that helps

MikeTschudi commented 7 years ago

While an app developer may want to modify these things, we've found that the boilerplate has been impressively close to what we need, and we've been happy to just add even complex apps on top of it.

Helper utilities sound great, especially if in a separate file. Again, the goal is to minimize merge pain. When the main program is rearranged and reformatted as part of its changes, safe merging can be challenging even with a good diff editor. So any form of stability in the boilerplate makes a big difference.

driskull commented 7 years ago

Closing. Let me know if further work is required.