Feat/promise api

[tomwayson]

resolves #8
resolves #28

TODO:

• verify new API functions work in wrapper libs, or at least ember-esri-loader
• can new promise-based methods can get rid of reliance on data-esri-loader on the script tag?
• remove error handlers when script/modules successfully load

[tomwayson]

@davetimmins, @nicksenger, I'd love your feedback on this.

Check out the updated README for examples of how to use the new API.

TLDR: loadModules() (like dojoRequire(), but returns a promise) is a one stop shop that will lazy-load the script if it hasn't been already by calling through to loadScript() (like bootstrap(), but returns a promise), which should sound familiar to @nicksenger, and I'm pretty sure is something that @davetimmins asked for at one point.

[nicksenger]

Looks good overall, may want to address the things I mentioned if they're a concern.

Nice test suite too! 👍

[nicksenger]

Will chime in here because this is something I literally just found in esriPromise() yesterday. As is, I think this will add a new handler for the dojo require's 'error' event every time requireModules is called. In esri-promise I just switched this section to the following:

    return new Promise((resolve, reject) => {
        // If something goes wrong loading the esri/dojo scripts, reject with the error.
        const handle = window['require'].on('error', (err: Error) => {
            handle.remove();
            reject(err);
        });
        window['require'](modules, (...args: any[]) => {
            handle.remove();
            // Resolve with the parameters from dojo require as an array.
            resolve(args);
        });
    });

This way the handler gets removed when the request either succeeds or fails and there should only be one handler on require's "error" at any given time -- at least so long as users allow any previous requireModules calls to resolve/reject before calling it again.

[tomwayson]

Great suggestion. I've implemented in af6528b.

[nicksenger]

Again something I just debugged recently in esri-promise, if someone has already included another loader or another package that claims window.require then this will return true even though the require is most likely not dojo's. Now in esri-promise I'm just making sure that the on method from dojo's require is present, otherwise assume window.require is not dojo's require and that the JS API still needs to be loaded..

It's not great, since forcing users to have dojo's require instead of whatever one they've already loaded may create problems in their app. On the other hand though, allowing them to use a non-dojo require will break things in the JS API, so it's a bit of a pickle.

I'm currently looking into ways of keeping dojo and the JS API out of the global namespace completely without having to sacrifice any functionality

[tomwayson]

Thanks for the suggestion! I've added this in
4ebb65d

I'm currently looking into ways of keeping dojo and the JS API out of the global namespace completely without having to sacrifice any functionality

💯 ☝️ that would be amazing and solve most of my web dev problems.

[tomwayson]

I've npm linked this branch in ember-esri-loader and it works like a charm.

One issue is, as I suspected, I get 'Promise' is undefined errors in IE:

That would be a breaking change for that library, so I'll have to figure out how to allow apps to provide their own Promise implementations sooner than later, but I don't know if that will happen in this PR, or even in the next release.

[davetimmins]

I like it! One thing I would like is the ability to access the results as named properties, at the moment the array just has items named f, so changing to something like

function requireModules(modules) {

    const modulesNames = modules;

    return new Promise(function (resolve, reject) {
        // If something goes wrong loading the esri/dojo scripts, reject with the error.
        window['require'].on('error', reject);
        window['require'](modules, function () {
            var args = {};
            for (var _i = 0; _i < arguments.length; _i++) {
                args[modulesNames[_i].split('/').pop()] = arguments[_i];
            }
            // Resolve with the parameters from dojo require as an array.
            resolve(args);
        });
    });
}

would allow you to work with the response as .then({MapView, Map}) => { which supports destructuring as an added bonus. This is for the use case when you want to pre load the API and modules but then resolve the promise later, though I think it makes overall debugging nicer too.

[tomwayson]

Thanks @davetimmins

I've been down that route before (Esri/angular-esri-map#72) and we ran into namespace collision issues and ultimately decided that it wasn't worth the hassle.

would allow you to work with the response as .then({MapView, Map}) => { which supports destructuring as an added bonus.

Unless I misunderstand, you can also destructure the array that is currently returned from loadModules() into named variables like:

esriLoader.loadModules(['esri/views/MapView', 'esri/WebMap'])
.then(([MapView, WebMap]) => {
  const webmap = new WebMap({
    portalItem: { // autocasts as new PortalItem()
      id: this.itemId
    }
  });
  // Set the WebMap instance to the map property in a MapView.
  this._view = new MapView({
    map: webmap,
    container: this.elementId
  });
});

I find that very familiar to the signature of Dojo's own require()/define() signatures and makes it just as easy to work w/ the returned classes.

[davetimmins]

I agree the current approach is more similar to the dojo signature, whether that is good or bad is personal preference I guess. Not sure namespace issue would be any different since the parameters are locally scoped so they should take precendence (I was expecting them as just the same class name rather than the fully qualified dot notation name). Also, for the array I don't think you can change the order that you define the parameters whereas with an object it won't matter as long as the names match e.g. { Map, MapView } is equivalent to { MapView, Map }

[tomwayson]

Those are some good points, esp re: personal preference.

I guess the issue I linked to did not describe the naming collision issues we encountered, but let's say you wanted load 'dojo/_base/Color' and 'esri/Color', what should the object property names be? The difference w/ an Array is that we push that decision on the caller and don't have to solve it in the API.

Thanks again for your thoughtful feedback!