Promise-based functions shouldn't throw exceptions

[Giners]

Functions that you have documented as returning a promise (e.g. queryRadius()) also throw exceptions as well. This is an anti-pattern in terms of JavaScript promise-based functions. This is especially problematic when working with the dynamodb-geo.js APIs outside of the context of an async function. Here is an example/repo:

// The signature of 'myCoolFunction' doesn't use the async function
// expression and thus doesn't have an async function context
function myCoolFunction() {
  myGeoTableManager.queryRadius({
    RadiusInMeter: 100000,
    CenterPoint: 'lol bananas - this string isnt a valid CenterPoint',
  })
  .then(data => {
    // Some more business logic...
  })
  .catch(err => {
    // Oh noes! Something went wrong but I will recover from the error here...
  })
}

This will cause the exception Error: [DecimalError] Invalid argument: undefined to be thrown which is reasonable since I provided bad input for CenterPoint. Yet the thrown exception circumvents all of my error handling logic (i.e. function passed to the catch() method) that I expect to run when the promise is rejected. The problem further compounds itself if I was expecting to delegate to another construct/module to perform additional async work as a result of a rejected promise (e.g. performing an async setState() call for a React component).

The problem can be resolved by wrapping APIs you have documented as returning a promise with a try/catch. For example:

/**
 * Query a circular area constructed by a center point and its radius.
 * 
 * I promise (har har) to never throw an exception and to always return a promise
 * that will be resolved with the results of the query or rejected with any errors that
 * occurred while trying to perform the query.
 */
public queryRadius(queryRadiusInput: QueryRadiusInput): Promise<DynamoDB.ItemList> {
  try {
    const latLngRect: S2LatLngRect =
      S2Util.getBoundingLatLngRectFromQueryRadiusInput(queryRadiusInput);

    const covering = new Covering(new this.config.S2RegionCoverer().getCoveringCells(latLngRect));

    return this.dispatchQueries(covering, queryRadiusInput)
      .then(results => this.filterByRadius(results, queryRadiusInput));
  } catch(e) {
    return Promise.reject(e);
  }
}

Other notes:

• The exception thrown by the example/repo I gave isn't something you throw directly but rather from other modules/APIs that you consume
• I haven't checked whether other APIs that dynamodb-geo.js exposes for consumption also have this problem/anti-pattern
• I'm consuming dynamodb-geo @ version 0.3.4. If this was fixed in version 0.4.0 then sorry for the noise.

Just wanted to finish off by saying great work on dynamodb-geo.js @rh389! It has proved very valuable. :smile:

[robhogan]

You're quite right - I hadn't thought about those throws from dependencies. The neatest approach to this these days is probably just to mark our public, promise-returning APIs as async, that way they're guaranteed to return a Promise and any throw inside the function will be converted to a rejection.

Unfortunately this is a (very minor) breaking change, since anyone actually expecting a synchronous exception will now see different behaviour. It'll be in the next release though.