Skip to content
This repository has been archived by the owner on Dec 1, 2020. It is now read-only.

Support Lazy Loading of ESRI Javascript API #60

Merged
merged 4 commits into from
Aug 16, 2015

Conversation

ScottONeal
Copy link
Contributor

Mentioned in issue #58.

First off, thanks for work that has being done on this library!

Essentially, this PR can be used when you want to defer the inclusion of the Esri ArcGIS API until necessary, which can be particularly helpful when developing SPAs that may not need the resources until a specific time (or not at all).

The bulk of the changes were made in src/services/esriLoader.js and an example can be found under test/deferred-map.html.

Generally syntax would look as followed, when doing a lazy load:

angular.module('esri-map-example', ['esri.map'])
    .controller('MapController', function ($scope, esriLoader) {

        // Default loaded to be false
        $scope.loaded = false;

        // Make a call to load Esri API resources, a promise is provided for when the resources has finished loading.
        esriLoader.bootstrap()
          .then(function() {

            // Set Loaded to be true
            $scope.loaded = true;

            // Do things
          });
    });

Also, this PR changes how requiring new Dojo/Esri modules work under the esriLoader provider. Now, esriLoader.require('name/of/modules') will need to be called, instead of esriLoader('name/of/modules').

Thanks!

@ScottONeal
Copy link
Contributor Author

@tomwayson, is this project being actively maintained or is it used for just static examples? Feedback into how to proceed with development efforts would be appreciated. Thanks!

@tomwayson
Copy link
Member

Thanks for your patience and follow up @ScottONeal. I'm trying to onboard others to help triage the PRs.

I'm very interested in this functionality. What do you think about having esriLoader.require() take an array of strings and be able to load multiple modules as in #54?

…red and will reject promise if Esri API is not loaded yet.
@ScottONeal
Copy link
Contributor Author

Okay, it would appear that some of the additions in #54 were a bit superfluous, essentially, the callback convention used in #54 would work the same if you just passed in the arguments as an array to promise.resolve(arrayOfModules).

callback convention in #54 would look something like:

esriLoader.require(['module1', 'module2'], function(module1, module2) {
 // do something
});

But, the promise that require is already uses will work as well:

esriLoader.require(['module1', 'module2')
  // An array of the modules are returned in resolve
  .then(function(modules) {
    //do something
  })

Essentially, if we don't have to embrace both Promises and Callbacks as conventions to get this done, I think that would be preferable.

Pinging @willisd2 for credit and input.

@ScottONeal
Copy link
Contributor Author

@tomwayson, Sorry to keep pinging. Any thoughts?

@willisd2
Copy link
Contributor

willisd2 commented Jul 2, 2015

@ScottONeal & @tomwayson: I believe I took the callback route (which is actually setup to support both a callback and using the promise method like you described) was because in situations where you are loading a ton of modules, you'd also have to write code like this:

esriLoader.require(['esri/toolbars/draw', 'esri/graphic', 'esri/tasks/IdentifyParameters', 
'esri/tasks/query', ... , 'esri/layers/FeatureLayer']).then(function(modules) {
    var Draw = modules[0];
    var Graphic = modules[1];
    var IdentifyParameters = modules[2];
    var Query = modules[3];
    ...
    var FeatureLayer = modules[15];
}

As opposed to having all the modules already defined as parameters in your function that does the work with the loaded modules, which you can do with the callback method. It's also more inline with how the ArcGIS for JavaScript library currently functions. However, I am more than happy to comply with whatever is decided, as I do see how it can cause confusion.

@jwasilgeo
Copy link
Contributor

Hey all, hopefully I can help out with keeping this PR conversation going--see above #60 (comment).

  1. We do need a remedy for being able to properly require in an array of strings when you want multiple AMD modules. I like both ideas presented here by @ScottONeal and @willisd2 in both Support Lazy Loading of ESRI Javascript API #60 and Fixes to esriLoader to enable passing an array of strings to it #54, respectively.

    @willisd2, you make a good point that when loading multiple modules, the resolved $q promise makes you have to write just as many lines of var declarations, which is less desirable.

    And yes, @ScottONeal, I do agree that it would be best to not have both a promise and a callback paradigm available in esriLoader, but to stick to one style or the other.

    My own quick fix of being able to require multiple modules at once while using the promise paradigm also suffers from the same result, leaving the developer with the task of setting up vars for each module required.

  2. Regarding the primary purpose of this PR for lazy loading the Esri JSAPI, @ScottONeal my hope is to test out your changes in this PR within the coming week.

What do you all think?

Thanks for your patience and interest in keeping this repo alive and well!

@tomwayson
Copy link
Member

Thanks for chiming in @jwasil!

At first I was thinking the promise-only approach was cleaner, but now that you point out that it would require declaring a bunch of local variables, I think it's worth while to also support a callback that would return all the modules as arguments (which would be familiar to the dojo/require syntax.

I'm just not sure the best way to bring it in. Merging #54 also brings in support for setting the visible attribute on a feature layer - which is also much needed, so I'm not opposed to that.

If I merge #54, then would you (@ScottONeal) be willing to merge those changes into this PR?

FYI - Now that we are post-UC and I am actually working on an Angular project again, I can promise a quick turn around on this.

@ScottONeal
Copy link
Contributor Author

Sorry for the late response (was at OSCON). I can absolutely pull in #54, once you have merged them in.

@tomwayson
Copy link
Member

@ScottONeal #54 has been merged.

@ScottONeal
Copy link
Contributor Author

@tomwayson PR has been updated with all changes. esriLoader.require now takes a callback, you can see an example in test/deferred-map.html.

I made a minor change in the callback check operation from #54.

Previously it was:

if (callback !== null && callback !== undefined) 

changed to:

if ( callback && angular.isFunction(callback) )

Essentially, if callback was truthy and not a function, then it would fall through the previous check and cause an exception. This minor change just ensures that it exists and is a function, before trying to call it.

@ScottONeal
Copy link
Contributor Author

@tomwayson What are your thoughts?

tomwayson added a commit that referenced this pull request Aug 16, 2015
Support Lazy Loading of ESRI Javascript API
@tomwayson tomwayson merged commit 1502db5 into Esri:master Aug 16, 2015
@tomwayson
Copy link
Member

Thank you @ScottONeal for your patience

Just a little patience

@jwasilgeo
Copy link
Contributor

👍

@ScottONeal
Copy link
Contributor Author

Haha, cheers! 🍻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants