Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw all IndexedDB errors as promises #5

Closed
JamesMessinger opened this issue Oct 25, 2014 · 5 comments
Closed

Throw all IndexedDB errors as promises #5

JamesMessinger opened this issue Oct 25, 2014 · 5 comments

Comments

@JamesMessinger
Copy link

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
Copy link
Owner

bramski commented Oct 25, 2014

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
#5.

@JamesMessinger
Copy link
Author

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
Copy link
Author

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);
        });
    }
  };
});

@JamesMessinger JamesMessinger changed the title Throw *all* errors as promises Throw all IndexedDB errors as promises Oct 26, 2014
@bramski bramski closed this as completed Oct 31, 2014
@bramski
Copy link
Owner

bramski commented Oct 31, 2014

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
Copy link

smks commented Apr 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants