Promise-based functions shouldn't throw exceptions
Giners opened this issue · 1 comments
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. 😄
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.