Fix dojo build issues #547

green3g · 2016-04-21T18:19:18Z

I wanted to start discussion on the possibility of increasing compatibility with the dojo build system.

This branch includes the necessary changes to perform a successful dojo build on the cmv widgets, core, and config files. Some of the changes are beneficial in general as well and don't necessarily apply directly to a dojo build.

Changes include:

Build system file

Add a package.js. This is necessary in the cmv package to tell the dojo builder that these files are 'amd' style files.

Directions widget

Removed the usage of a reserved word float

MapInfo widget and app config

Changed proj4 path to be configured by the app.js config file. This is necessary because the dojo builder cannot access files remotely to bundle. In addition, this is a useful addition because it allows for easier upgrade of proj4 in the future (if more than one widget uses proj4) and lets people load proj4 locally without modifying any widgets.

Streetview widget

Modified the proj4 path as in the MapInfo widget
Modified the way that google-maps is loaded by switching to the bower/npm version of the google-maps loader.

green3g · 2016-04-21T18:28:46Z

Is there any docs on the requirements for the tabs/spaces etc for the spaces/tabs indentation?

tmcgee · 2016-04-21T18:39:23Z

@roemhildtg

This discussion of a build system is very timely. I will comment more when back at the office.

As to the code rules:

The rules are in the .eslintrc file

travis uses that file to impose our perhaps overly strict rules. ;)

You can run grunt eslint locally to catch the errors in advance or grunt watch to check for errors as the file is saved. I use an eslint package within my editor (Sublime Text) to catch them as I type. All these methods use the same source .eslintrc file to impose consistency.

tmcgee · 2016-04-21T18:44:37Z

Here's an example of possibly an overly strict rule: https://travis-ci.org/cmv/cmv-app/builds/124825733#L1352

Alternative is to use dojo's domStyle to set the float value. This should pass the eslint checks AND avoid the float is a reserved word.

DavidSpriggs · 2016-04-24T07:40:59Z

@roemhildtg This is GREAT! Thanks for putting in the time to make this work. It has been on our list for awhile. Please get the Travis checks passing, as @tmcgee mentioned, and we will take steps to include this PR.

DavidSpriggs · 2016-04-26T04:10:57Z

@roemhildtg Please update with base branch.

DavidSpriggs · 2016-04-27T18:26:29Z

@roemhildtg Let us know when it's working.

green3g · 2016-04-27T21:08:37Z

Oh sorry, I've tried it out and everything seems to work as expected.

tmcgee · 2016-04-30T16:51:28Z

@roemhildtg I am curious if loading the Google maps api as a plugin addresses the issue noted at the bottom of the README for the new Google Nearby contributed widget that also uses that api?

green3g · 2016-04-30T17:03:29Z

I think that issue should be addressed by the google-maps plugin. It looks like if Google is defined, it simply returns the result.

https://github.com/Carrooi/Js-GoogleMapsLoader/blob/develop/lib/Google.js#L62-L78

green3g · 2016-04-30T17:06:06Z

We should wait on merging this, I've just started discovering a new set of issues with the dojo builder and cmv. I'd like to get those cleared up I think before.

tmcgee · 2016-04-30T17:06:51Z

@roemhildtg @DavidSpriggs I understand the need/desire for moving proj4js to the dojoConfig packages array for build purposes.

In PR #553 I added xstyle (and put-selector) to the packages array - a necessity to address a potential issue with Google Chrome identified in #552. That got me thinking about a topic that has been tossed around before. Should we be more in control of our own destiny and include some or all of these third-party libraries in a vendor or similar folder?

tmcgee · 2016-04-30T17:10:50Z

Ok. Will not merge this now.

On the build issues, I know that @btfou was working on the dojo build (and a number of other concepts) in the cmv-labs repo. I don't know if it has any relevance. Just throwing it out there.

tmcgee · 2016-04-30T17:26:58Z

Another comment related to proj4js.

I am wanting to move all resources and map services over HTTPS whenever possible. Even when the originating app is served over HTTP, accessing other resources securely using HTTPS is best practice.

The new Google plugin loads the google api using HTTPS - a move in the right direction.

I think http://spatialreference.org is the only blocker in the core cmv project. The references to spatialreference.org needs to change regardless since it will not work when the app is served over HTTPS. A move to epsg.io would address this.

green3g · 2016-04-30T17:29:32Z

Its definitely relevant, its great to look into the other options available. I've found the dojo builder really difficult to configure and work with and very finicky. So I've also been looking into loading dojo with systemjs with esri systemjs.

green3g · 2016-04-30T17:30:59Z

SystemJS has been used to create StealJS which comes with a very easy build tool. I've been using Steal on a different project and its build process scans for dependencies by telling it an entry point. Really neat and almost no config necessary.

green3g · 2016-04-30T17:39:52Z

+1 on epsg.io

green3g · 2016-05-02T13:21:28Z

@tmcgee One of the issues I'm currently looking into is the js/config/app.js. Dojo Builder can't find where window is declared, and throws this error ReferenceError: window is not defined. Any suggestions on how we handle this?

My only thought is to move the window.dojoConfig back into the index.html.

tmcgee · 2016-05-02T15:12:31Z

@roemhildtg does the build succeed if you use dojoConfig instead of window.dojoConfig?

green3g · 2016-05-02T15:26:21Z

It doesn't. dojoConfig is now undefined. Same for location on line 2.

Edit: I was wrong. the dojoConfig without window is okay. Its just location that is causing the problem now.

green3g · 2016-05-02T15:52:13Z

This is how I'm able to get the build to work:

(function () {
    //var path = location.pathname.replace(/[^\/]+$/, '');
    dojoConfig = {
        async: true,
        packages: [
            {
                name: 'viewer',
                location: 'js/viewer'
            }, {
                name: 'gis',
                location: 'js/gis'
            }, {
                name: 'config',
                location: 'js/config'
            }, {
                name: 'proj4js',
                location: '//cdnjs.cloudflare.com/ajax/libs/proj4js/2.3.12'
            }
        ]
    };

    require(dojoConfig, [
        'dojo/_base/declare',
//........

tmcgee · 2016-05-02T16:05:43Z

Can you wrap it in a define statement like this:

define([], function () {    // or perhaps define(null, function () { ?
    var path = location.pathname.replace(/[^\/]+$/, '');
    window.dojoConfig = {
        async: true,
        packages: [
            {
                name: 'viewer',
                location: path + 'js/viewer'
            }, {
                name: 'gis',
                location: path + 'js/gis'
            }, {
                name: 'config',
                location: path + 'js/config'
            }
        ]
    };

    require(window.dojoConfig, [
        'dojo/_base/declare',

    ...

});

which seems a little more "dojo-ish" than the original

(function () {
    ...
})();

The function wrapped in the define should not be executed by the AMD loaded during the build.

btfou · 2016-05-02T17:00:14Z

@roemhildtg Some thoughts... Simply pass dojoConfig object as the first require param. Don't use CDN packages; pull all dependency packages in with bower and include in the build. You should also set baseUrl for builds.

(function () {
  require({
    async: true,
    baseUrl: '/',
    packages: [{
      name: 'app',
      location: './app'
    }, {
      name: 'dojo',
      location: './dojo'
    }, {
      name: 'dijit',
      location: './dijit'
    }, {
      name: 'dojox',
      location: './dojox'
    }, {
      name: 'dstore',
      location: './dstore'
    }, {
      name: 'dgrid',
      location: './dgrid'
    }, {
      name: 'esri',
      location: './esri'
    }, {
      name: 'dgrid-esri',
      location: './dgrid-esri'
    }, {
      name: 'put-selector',
      location: './put-selector'
    }, {
      name: 'xstyle',
      location: './xstyle'
    }, {
      name: 'ArcGISServerStore',
      location: './ArcGISServerStore',
      main: 'ArcGISServerStore'
    }, {
      name: 'TerraArcGIS',
      location: './TerraArcGIS',
      main: 'TerraArcGIS'
    }, {
      name: 'proj4',
      location: './proj4',
      main: 'proj4'
    }],
    map: {
      esri: {
        dgrid: 'dgrid-esri'
      }
    }
  }, ['app/App', '/config.js', 'dojo/domReady!'], function (App, config) {
    App.run(config);
  });
})();

The above is app/main.js. The nice thing about this approach is your dojo build can be loaded with a single script tag:

<script data-dojo-config="deps:['app/main']" src='dojo/dojo.js'></script>

green3g · 2016-05-02T17:38:10Z

@tmcgee This is working:

define(['dojo/_base/window'], function(window){

    var path = window.global.location.pathname.replace(/[^\/]+$/, '');
    var dojoConfig = window.global.dojoConfig = {
        async: true,
        packages: [
            {
                name: 'viewer',
                location: path + 'js/viewer'
            }, {
                name: 'gis',
                location: path + 'js/gis'
            }, {
                name: 'config',
                location: path + 'js/config'
            }, {
                name: 'proj4js',
                location: '//cdnjs.cloudflare.com/ajax/libs/proj4js/2.3.12'
            }
        ]
    };

    require(dojoConfig, [
        'dojo/_base/declare',

        // minimal Base Controller
        'viewer/_ControllerBase',

        // *** Controller Mixins
        // Use the core mixins, add custom mixins
        // or replace core mixins with your own
        'viewer/_ConfigMixin', // manage the Configuration
        'viewer/_LayoutMixin', // build and manage the Page Layout and User Interface
        'viewer/_MapMixin', // build and manage the Map
        'viewer/_WidgetsMixin' // build and manage the Widgets

        //'config/_customMixin'

    ], function (
        declare,

        _ControllerBase,
        _ConfigMixin,
        _LayoutMixin,
        _MapMixin,
        _WidgetsMixin

        //_MyCustomMixin

    ) {
        var controller = new (declare([
            _ControllerBase,
            _ConfigMixin,
            _LayoutMixin,
            _MapMixin,
            _WidgetsMixin
        ]))();
        controller.startup();
    });
    //we have to return something
    return dojoConfig;
});

@btfou That approach looks good and clean.

tmcgee · 2016-05-02T17:44:36Z

@roemhildtg This seems to be getting more complicated only to please the build system...

tmcgee · 2016-05-05T08:40:39Z

Additional Comments:

GoogleMapsLoader.VERSION

The** version of the Google Maps API should be set to 3 to load the current "Release" version (3.23 at this time) or some version higher than 3.18. Any version number below 3.22 will load the "Frozen" version of the api (3.22 at this time).
Reference: https://developers.google.com/maps/documentation/javascript/versions

GoogleMapsLoader.KEY

We need to provide a way for a Google Maps Key be passed to the API when it is loaded. Using this new plugin provides the opportunity. If you do not include a KEY, there is a console message of Google Maps API warning: NoApiKeys. At present this is harmless. However since that message is a relatively new addition, I would not be surprised at all if Google some time in the future requires a KEY with all usage.
Reference: https://developers.google.com/maps/documentation/javascript/error-messages#no-api-keys

With some modification to the plugin, the version, key and other Google API parameters could be configured within viewer.js or elsewhere using something like this:

GoogleMapsLoader = {
    VERSION: 3,
    KEY: 'MyKey'
}

green3g · 2016-05-05T12:57:21Z

@tmcgee I think that the plugin can be configured without modification like this:

require(['gis/plugins/Google'], function(GoogleMapsLoader){
    GoogleMapsLoader.URL = 'https://maps.googleapis.com/maps/api/js';
    GoogleMapsLoader.KEY = null;
    GoogleMapsLoader.LIBRARIES = [];
    GoogleMapsLoader.CLIENT = null;
    GoogleMapsLoader.CHANNEL = null;
    GoogleMapsLoader.LANGUAGE = null;
    GoogleMapsLoader.REGION = null;
    GoogleMapsLoader.VERSION = googleVersion;
    GoogleMapsLoader.WINDOW_CALLBACK_NAME = '__google_maps_api_provider_initializator__';
});

Documentation here: https://www.npmjs.com/package/google-maps

…on of proj4.

green3g · 2016-05-12T20:20:46Z

Ready for review. Sorry for all the commits, I could try and merge them into one, but I'm not sure exactly how to do so, and I would rather not mess anything up.

Rather than changing the whole app.js, I just use the package.js to ignore it. It passes the dojo build so there's no problem there.

green3g · 2016-05-12T23:17:28Z

Hi guys, just added a demo: http://roemhildtg.github.io/cmv-app-dojo-builder/dist/

Enjoy!

tmcgee · 2016-05-13T15:08:53Z

@roemhildtg Thanks for this. I should have some time this weekend to check it out.

tmcgee · 2016-06-17T23:32:01Z

@roemhildtg Can you merge the revised develop branch into your branch?

@DavidSpriggs Do you have any input on this PR? I haven't tested the dojo build process but the other changes make sense to me. I have pending changes to the StreetView and MapInfo widgets to migrate from spatialreference.org to epsg.io since the former site does not work with HTTPS.

green3g · 2016-06-17T23:33:36Z

Github is so awesome. That was too easy.

green3g · 2016-06-17T23:38:53Z

FYI if anyone wants to test out the dojo build, I have a repo set up. It would be awesome to hear feedback from someone on their experiences with the process and usage.

https://github.com/roemhildtg/cmv-app-dojo-builder

green3g · 2016-07-10T20:43:55Z

@tmcgee @DavidSpriggs I updated the branch again. Is this good to be merged?

tmcgee · 2016-07-12T19:30:41Z

@roemhildtg I do like this and think it should be merged. I believe @DavidSpriggs wants to discuss it further once he returns from his extended holiday.

tmcgee · 2016-07-13T20:53:02Z

viewer/js/config/app.js

@@ -20,6 +20,9 @@
                name: 'xstyle',
                main: 'css',
                location: 'https://cdn.rawgit.com/kriszyp/xstyle/v0.3.2'
+            }, {
+                name: 'proj4js',
+                location: '//cdnjs.cloudflare.com/ajax/libs/proj4js/2.3.12'


@roemhildtg A heads up. From my latest PR #571, I am recommending we use HTTPS wherever possible as it has now become the preferred "standard" practice versus the old // url pattern. Assuming that recommendation passes review from @DavidSpriggs, this proj4js reference would change to HTTPS.

DavidSpriggs · 2016-07-14T04:33:51Z

Looking good. Thanks for the work on this!

DavidSpriggs · 2016-07-14T04:34:58Z

Merge the base branch back in and we will do a final merge.

green3g force-pushed the fix/fix-dojo-build-issues branch from 0117e09 to a53d766 Compare April 26, 2016 14:49

green3g added 3 commits May 12, 2016 11:12

Move proj4 to the app config to allow developers to use offline versi…

e8ef183

…on of proj4.

Remove proj4 cdn path from streetview

c5eeb40

Add a package.js build descriptor file

35e8d8d

green3g added 6 commits May 12, 2016 11:12

Tell dojo to use the package.js build file

0e1ba87

Exclude node_modules and other non-amd files from being tagged as amd

1cf7750

Remove troublesome error from Google Loader

059b3ac

Add more rules to the package.js

a62df6a

Makes config/app dojo-build friendly

95ed51c

Fix proj4js path merge

1e916e6

green3g force-pushed the fix/fix-dojo-build-issues branch from e3c7b4e to 1e916e6 Compare May 12, 2016 16:35

green3g added 2 commits May 12, 2016 11:57

Fix eslint errors (again)

3f89efc

Revert app.js back to cmv default and ignore app.js via the profile.

0e5224f

tmcgee added the feature label Jun 16, 2016

tmcgee added this to the v1.4.0 milestone Jun 16, 2016

Merge branch 'develop' into fix/fix-dojo-build-issues

088b9c9

Merge branch 'develop' into fix/fix-dojo-build-issues

9914473

tmcgee reviewed Jul 13, 2016
View reviewed changes

tmcgee mentioned this pull request Jul 13, 2016

files not accessible within network; cloudflare & @font-face #570

Closed

Merge branch 'develop' into fix/fix-dojo-build-issues

90ee802

DavidSpriggs merged commit f229b6e into cmv:develop Jul 14, 2016

green3g deleted the fix/fix-dojo-build-issues branch July 14, 2016 19:46

green3g restored the fix/fix-dojo-build-issues branch July 14, 2016 19:46

Fix dojo build issues #547

Fix dojo build issues #547

Conversation

green3g commented Apr 21, 2016 • edited Loading

Build system file

Directions widget

MapInfo widget and app config

Streetview widget

green3g commented Apr 21, 2016

tmcgee commented Apr 21, 2016

tmcgee commented Apr 21, 2016

DavidSpriggs commented Apr 24, 2016

DavidSpriggs commented Apr 26, 2016

DavidSpriggs commented Apr 27, 2016

green3g commented Apr 27, 2016

tmcgee commented Apr 30, 2016

green3g commented Apr 30, 2016

green3g commented Apr 30, 2016

tmcgee commented Apr 30, 2016

tmcgee commented Apr 30, 2016

tmcgee commented Apr 30, 2016

green3g commented Apr 30, 2016 • edited Loading

green3g commented Apr 30, 2016 • edited Loading

green3g commented Apr 30, 2016

green3g commented May 2, 2016 • edited Loading

tmcgee commented May 2, 2016

green3g commented May 2, 2016 • edited Loading

green3g commented May 2, 2016

tmcgee commented May 2, 2016 • edited Loading

btfou commented May 2, 2016

green3g commented May 2, 2016

tmcgee commented May 2, 2016

tmcgee commented May 5, 2016

GoogleMapsLoader.VERSION

GoogleMapsLoader.KEY

green3g commented May 5, 2016

green3g commented May 12, 2016 • edited Loading

green3g commented May 12, 2016

tmcgee commented May 13, 2016

tmcgee commented Jun 17, 2016

green3g commented Jun 17, 2016

green3g commented Jun 17, 2016 • edited Loading

green3g commented Jul 10, 2016

tmcgee commented Jul 12, 2016

tmcgee Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

DavidSpriggs commented Jul 14, 2016

DavidSpriggs commented Jul 14, 2016

green3g commented Apr 21, 2016 •

edited

Loading

green3g commented Apr 30, 2016 •

edited

Loading

green3g commented Apr 30, 2016 •

edited

Loading

green3g commented May 2, 2016 •

edited

Loading

green3g commented May 2, 2016 •

edited

Loading

tmcgee commented May 2, 2016 •

edited

Loading

green3g commented May 12, 2016 •

edited

Loading

green3g commented Jun 17, 2016 •

edited

Loading

tmcgee Jul 13, 2016 •

edited

Loading