esri-loader icon indicating copy to clipboard operation
esri-loader copied to clipboard

loadModules examples in the README - async/await?

Open gavinr opened this issue 4 years ago • 9 comments

To me, the way @jwasilgeo is using loadModules with async/await here is much more readable. Do others agree? Now that async await is pretty mainstream (sorry IE11), I'm wondering if we should switch all our examples in the README to use this style (we could still include one Promises style example just to cover all bases)

gavinr avatar Jan 10 '20 20:01 gavinr

I completely agree! We use the same pattern here: https://github.com/Esri/angular-cli-esri-map/blob/master/src/app/esri-map/esri-map.component.ts#L86

andygup avatar Jan 10 '20 21:01 andygup

I agree that async/await is more readable.

Up until now I have not been convinced that it is the right choice for the docs. I believe that people who are comfortable w/ async/await see promises and think "I know how do that w/ async/await," but I'm not sure it's always true going the other way. That might not be true anymore. There's probably an entire "generation" of JS devs that have never seen promises.

If I'm out voted, I certainly wouldn't reject a PR updating the docs to async/await.

tomwayson avatar Jan 17 '20 15:01 tomwayson

One note that I've realized since I posted the original issue:

sometimes part of the "simplicity" of async/await examples is actually just because the promise-based includes error handling where the async/await example [incorrectly] leaves it out.

If we take an example from our actual README:

// component.js
import { loadModules } from 'esri-loader';

// this will lazy load the ArcGIS API
// and then use Dojo's loader to require the map class
loadModules(['esri/map'])
.then(([Map]) => {
  // create map with the given options at a DOM node w/ id 'mapNode'
  let map = new Map('mapNode', {
    center: [-118, 34.5],
    zoom: 8,
    basemap: 'dark-gray'
  });
})
.catch(err => {
  // handle any script or module loading errors
  console.error(err);
});

It would be tempting and satisfying to change it to async/await without error handling:

// component.js
import { loadModules } from 'esri-loader';

// this will lazy load the ArcGIS API
// and then use Dojo's loader to require the map class
const [Map] = await loadModules(['esri/Map']);

// create map with the given options at a DOM node w/ id 'mapNode'
let map = new Map('mapNode', {
  center: [-118, 34.5],
  zoom: 8,
  basemap: 'dark-gray'
});

... but that's not equivalent to the original promise-based example! Making it equivalent (including error catching):

// component.js
import { loadModules } from 'esri-loader';

// this will lazy load the ArcGIS API
// and then use Dojo's loader to require the map class
try {
  const [Map] = await loadModules(['esri/Map']);

  // create map with the given options at a DOM node w/ id 'mapNode'
  let map = new Map('mapNode', {
    center: [-118, 34.5],
    zoom: 8,
    basemap: 'dark-gray'
  });

} catch (err) {
  // handle any script or module loading errors
  console.error(err);
}

... which makes it closer in complexity to the promise-based example. I would argue the async/await-example-with-try-catch example is still slightly better readability, but only marginally.

gavinr avatar Jan 17 '20 16:01 gavinr

Those are all really good points. What if we were to start with and evaluate having a side-by-side (or rather, one right after the other) comparison of both styles, for those devs that have a preference or more familiarity with one style over the other? And we leave the remaining code snippets alone to see how it goes?

jwasilgeo avatar Jan 17 '20 16:01 jwasilgeo

Not sure how you'd do that in a GitHub README. I wouldn't be opposed to such a PR, but I'd rather see the effort go to v3.

tomwayson avatar Jan 17 '20 16:01 tomwayson

Sorry for the confusion, I shouldn't have used the phrase "side-by-side" in terms of .md. I just meant adding another code snippet that is after/before an existing promise-based one to show the alternative async/await syntax as a way to evaluate this idea. But, I see now that we already do this in a few lines of 2nd half README anyways.

jwasilgeo avatar Jan 17 '20 17:01 jwasilgeo

Indeed all good points. I'm on board with @jwasilgeo suggestion of adding another code snippet as an interim step.

FWIW, in the Angular repo and associated tech presentation repos, we've had the async/await + try/catch pattern for quite a while. While not a definitive or final survey of pluses/minuses of this approach, I haven't received any negative comments or complaints, and I know it's being used in quite a few implementations.

andygup avatar Jan 17 '20 20:01 andygup

I think the Angular repo's a bit of a different story, you pretty much know the whole audience is using TypeScript.

I wouldn't add a duplicate snippet just to show async/await.

Maybe start w/ just all the examples under Advanced Usage?

Except this one of course: https://github.com/Esri/esri-loader#using-the-esriloader-global

tomwayson avatar Jan 18 '20 16:01 tomwayson

I think we should do this. It's time.

tomwayson avatar Dec 31 '20 16:12 tomwayson

Closing since this package is scheduled for deprecation at 4.29.

andygup avatar Feb 05 '24 18:02 andygup