bramski / angular-indexedDB

An angularjs serviceprovider to utilize indexedDB with angular
165 stars 49 forks source link

Throw all IndexedDB errors as promises #5

Closed JamesMessinger closed 10 years ago

JamesMessinger commented 10 years ago

Any code that interacts directly with the IndexedDB API needs to be wrapped in a try...catch block so that any errors can be passed to promise.reject() rather than being thrown up the stack. Currently, I have to have two types of error handling logic in my code, because some errors are thrown synchronously, and others are "thrown" asynchronously via promise.reject(). Here's an example:

var user = { id: 1, username: 'jdoe', password: 'password' };

$indexedDB.openStore('users', function(userStore) {
    try {
        userStore.upsert(user).then(
            function(result) {
                // Yay!  The IDBRequest.onsuccess event fired!
            },
            function(err) {
                // Onoes!  The IDBRequest.onerror event fired!
            }
        );
    }
    catch (e) {
        // Onoes!  IDBObjectStore.put threw an error!
    }
});

Ideally, I could get rid of the try...catch block entirely and just write the following code instead:

var user = { id: 1, username: 'jdoe', password: 'password' };

$indexedDB.openStore('users', function(userStore) {
    userStore.upsert(user).then(
        function(result) {
            // Yay!  The IDBRequest.onsuccess event fired!
        },
        function(err) {
            // Onoes!  An error occurred!  Doesn't matter whether it was IDBRequest.onerror or IDBObjectStore.put
        }
    );
});
bramski commented 10 years ago

The angular exception handler does this pretty well. Is there a reason that you don't want to use that for generally thrown exceptions?

Otherwise this would make for a very confusing API. If there is a specific type of error that should be handled, caught and rejected then sure. Otherwise a general "catch-all" ends up being confusing for all parties involved. You will end up catching programming errors in your controller code. What are you expected to do if the exception is some kind of unknown property error?

On Saturday, October 25, 2014, James Messinger notifications@github.com wrote:

Any code that interacts directly with the IndexedDB API needs to be wrapped in a try...catch block so that any errors can be passed to promise.reject() rather than being thrown up the stack. Currently, I have to two types of error handling logic in my code, because some errors are thrown directly, and others are passed to promise.reject(). Here's an example:

var user = { id: 1, username: 'jdoe', password: 'password' }; $indexedDB.openStore('users', function(userStore) { try { userStore.upsert(user).then( function(result) { // Yay! The IDBRequest.onsuccess event fired! }, function(err) { // Onoes! The IDBRequest.onerror event fired! } ); } catch (e) { // Onoes! IDBObjectStore.put threw an error! }});

— Reply to this email directly or view it on GitHub https://github.com/bramski/angular-indexedDB/issues/5.

JamesMessinger commented 10 years ago

The IndexedDB APIs can throw some errors immediately (rather than through the IDBRequest.onerror event). If you prefer to avoid a "catch-all" error handler, then you can catch the specific error types (they're documented on MDN), but that seems overly complex and fragile, since different browser implementations might throw different errors. Instead, perhaps a better approach would be to only wrap the actual IndexedDB API call in a try...catch block, that way you know that any error that is thrown is an IndexedDB error, as opposed to a general programming error.

General-programming errors aside, the end result is a much cleaner API that wraps the entire IndexedDB API in promises, which is the main reason for using Angular-IndexedDB. It doesn't seem very consistent to wrap some parts of IndexedDB in promises, but not other parts.

JamesMessinger commented 10 years ago

Specifically, a common use-case for Angular-IndexedDB is to make it easy to write Angular services that perform async DB operations and return promises. Something like this:

var user = { id: 1, username: "jdoe", password: "password" };
$myCustomService.saveUser(user).then(showSuccessMessage, showErrorMessage);

But if Angular-IndexedDB sometimes throws synchronous errors and sometimes throws asynchronous errors, then the above code becomes much less clean:

var user = { id: 1, username: "jdoe", password: "password" };
try {
    $myCustomService.saveUser(user).then(showSuccessMessage, showErrorMessage);
}
catch (err) {
    showErrorMessage(err);
}

Alternatively, I could move the try...catch logic inside myCustomService instead, but this is also pretty ugly:

angular.modlue('myApp').factory('$myCustomService', function($indexedDB, $q) {
  return {
    saveUser: function(userObj) {
      var deferred = $q.defer();

      $indexedDB.openStore('users', function(userStore) {
        try {
          userStore.upsert(userObj).then(
            function(result) {
              deferred.resolve(result);
            },
            function(err) {
              deferred.reject(err);
            }
          );
        }
        catch (err) {
          deferred.reject(err);
        }
      });

      return deferred.promise;
    }
  };
});

Instead, if Angular-IndexedDB always threw errors asynchronously via deferred.reject(), then my service code would be much more clean:

angular.modlue('myApp').factory('$myCustomService', function($indexedDB) {
  return {
    saveUser: function(userObj) {
        return $indexedDB.openStore('users', function(userStore) {
          userStore.upsert(userObj);
        });
    }
  };
});
bramski commented 10 years ago

So I'm closing this because Angular already does this for us.... All work within indexedDB is already wrapped in a promise. Namely the promise openDatabase(). I made an experiment and had the library itself throw a random exception, and low and behold the top level "catch" receives this exception! You may see the more recent test code I am submitting now which is successful in having vision of unhandled exceptions within the code and making them visible to Jasmine. So you can see there is no need to do this. As I suspected ... Angular promises already do this. Here is a screenshot of the code so you may understand what they do:

screen shot 2014-10-31 at 10 59 12 am

So... should some promise handler throw an exception, the promise is rejected with the exception AND the exceptionHandler is notified of this.

smks commented 8 years ago

Hey Bram, could you help with this? http://stackoverflow.com/questions/36869596/angularjs-return-object-of-methods-after-indexeddb-call