Should esriLoader.require()'s promise resolve an object instead of an array

[tomwayson]

I know we've already had this discussion but I wonder if the promise was resolved with an object that contained the required modules as properties if it would be easier to work with instead of an array. The only trick would be making sure there are no name conflicts in the prorperties, but we could just replace the / characters with . characters.

To me, this:

esriLoader.require(['esri/Map', 'esri/layers/FeatureLayer']).then(function(modules) {
  var map = new modules.esri.Map(...);
  var layer = new modules.esri.layers.FeatureLayer(...);
});

Is better than this:

esriLoader.require(['esri/Map', 'esri/layers/FeatureLayer']).then(function(modules) {
  var Map = modules[0];
  var FeatureLayer = modules[1];
  var map = new Map(...);
  var layer = new FeatureLayer(...);
});

Just want to get input on this before #69.

[tomwayson]

Just to clarify - this would only change the behavior in the case where a user requires more than one module (i.e. passes in an array of module names instead of a string) and used the returned promise (i.e. the callback would still receive an array of modules).

[jwasilgeo]

@tomwayson I think it is a good idea how you propose to do dot notation properties. I can live with the current style of modules[index] array lookup, but I also feel like this can be further refined.

I looked back at the history you linked to above, and I am wondering (or totally missed something!), is there a possibility to use the style of the optional callback to try and emulate the dojo require style, putting the onus on the developer using the esriLoader to provide the names of modules they'll use, e.g.: ...then(function(Map, FeatureLayer) {});? EDIT: whoops, basic promise resolving fail

Just a thought. As I mentioned, I like your proposal for property lookup as well in the resolved promise.

[tomwayson]

...then(function(Map, FeatureLayer) {});

is not possible. Promises can only resolve to a single value. If you called the above statement, every argument after Map would be undefined. So we have to resolve either and array of modules (like we do currently), or a composite object of modules (as I'm proposing).

If no one else is bothered by the current implementation (array of modules) I'll probably leave it as is... so many other fish to fry.

[jwasilgeo]

Aha, yeah...that thought made sense early this morning, but I completely see why that doesn't fly. I really need to stop trying to make intelligent-sounding comments on here pre-caffeine. Thanks for clarifying, @tomwayson. I still vote for your composite object of modules, if we get a chance to revisit this discussion.

[tomwayson]

I think this proposal is a better API, and now is the time to change it if we're going to. The only thing holding me back is that I have zero energy to do effectively do a lightweight implementation of _.set. Also, no one else has chimed in on this, so doesn't seem like anyone else sees value in it.

Thoughts @jwasil?

[jwasilgeo]

Agreed, the proposal is 👍. However, I have been using the 2nd arg callback style lately anyways, just as in the new custom-directive test page, so I haven't really had to face this. I'm asking because I am genuinely curious and want to understand if I'm not taking into account other use-cases: is it worth offering this in the first place? EDIT: I mean as a public-facing API is it worth making the change? For example, internally use modules[0] lookup, but recommend to devs providing a 2nd arg function, like one might normally do with dojo's require.

If either of us spend time in esriLoader, I think it could benefit from some additional modifications, such as:

• how about injecting $document and $window instead?
• the return of methods at the end is an agreeable style, but doesn't adhere to how our other factories are formatted

[tomwayson]

Would the reason to inject $document and $window to make the module more testable?

[jwasilgeo]

Hmm yeah I suppose that's the only solid reason I can come up with to be honest. Can reasses when actually writing unit tests. May be an unnecessary item.

[jwasilgeo]

Gonna try it out anyways

[tomwayson]

FYI - #140 is may cause many a merge conflict. May want to review/merge that before starting on anything (hope I'm not too late).

[jwasilgeo]

Let's cancel this, as discussed, due to jsapi naming conventions. Won't be so bad to implement again if we want to in the future.