Improvements to the promises implementation

[cgwrench]

Firstly, thank you for a great library. We've used this library successfully on a recent project at @EnableSoftware.

In the course of using this library, we've found a couple of issues which if addressed we feel will make the library even nicer to work with:

1. It seems that throughout the code, a string is thrown, rather than an Error (see, for example, A String is not an Error;
2. Code both rejects and throws;
3. In a number of places in the code, error handlers appear to be attached too late.

An example of these issues is the get() method on the Server object:

this.get = function ( table , id ) {
    if ( closed ) {
        throw 'Database has been closed';
    }
    var transaction = db.transaction( table ),
        store = transaction.objectStore( table );

    var req = store.get( id );
    return new Promise(function(resolve, reject){
      req.onsuccess = function ( e ) {
          resolve( e.target.result );
      };
      transaction.onerror = function ( e ) {
          reject( e );
      };
    });
};

Here we see that a string, rather than an Error is thrown. I believe that the first block in this method should be replaced by

if ( closed ) {
    throw new Error('Database has been closed');
}

or should return a Promise.reject() (see below).

Secondly, we see in the get() method that this method either throws, or returns a Promise. This means that calling code has to handle both cases. This means a try/catch and a .then()/.catch().

I realise that there is a school of thought that suggests throwing rather than rejecting, but this leads to, in my opinion, overly verbose calling code (the linked article acknowledges that this practice is controversial, and that some people recommend "Reject[ing] whenever possible, [as] it's more performant because throw breaks the stack.").

With our current codebase, we have had to litter our code which consumes db.js with lots of code like the following, just to handle both the errors thrown and the promises returned:

return new Promise(function (resolve, reject) {
    try {
        db.open({
            server: "name",
            version: 1
        }).then(function (s) {
            resolve(s);
        }, function (e) {
            reject(e);
        });
    } catch (e) {
        reject(e);
    }
});

It would be nicer if consumers of db.js only needed to handle the returned promise.

Finally, it appears that in some cases error handlers are attached too late. In the get() method, above, we can see that a error handler is added to the transaction after the call to store.get(id). It appears from our testing that this means that if an error is thrown during the get operation, it is not handled by the error handler attached to the transaction. It appears that this is a pattern that is employed throughout the code, not just in this get() method.

Given the concerns above, a re-factored get() method could look something like:

this.get = function ( table , id ) {
    if ( closed ) {
        return Promise.reject(new Error('Database has been closed'));
    }

    return new Promise(function (resolve, reject) {
        var transaction = db.transaction(table);

        transaction.onerror = function ( e ) {
            reject( e );
        };

        var store = transaction.objectStore(table),
            req = store.get( id );

        req.onsuccess = function ( e ) {
            resolve( e.target.result );
        };
    });
};

I believe that this addresses the concerns outlined above. Please let me know if any of this requires further information or examples.

[aaronpowell]

Hey @cgwrench,

First off - that's for the really detailed feedback, it's much appreciated :smile:.

It seems that throughout the code, a string is thrown, rather than an Error

I'd say this is because you're using an outdated version of the library. I recently moved the code to ES6 and added ESLint to it, which then raised all of those as errors and in turn I fixed them. I haven't pushed a new package to npm yet, so if that's your source then my apologies that it's not up to date (had a few PR's that I've been merging in before cutting the next release).

Code both rejects and throws;

This is a really good point and I'd never really thought about it, but you're totally right, it shouldn't throw an error, it should reject the promise. More than happy to get this refactored in.

In a number of places in the code, error handlers appear to be attached too late.

You mentioned that your testing has indicated that the errors don't always get picked up and result in a rejected promise, do you have any repo's which I can see how that condition can happen, I've not come across that myself.

That said, the approach to using promises is not optimised for ES6 promises, it's still somewhat based around the previous implementation (which was my own Promise library, based on jQuery).

With your final code example I'd actually refactor it to look like this:

    this.get = function ( table , id ) {
        return new Promise(function (resolve, reject) {
            if ( closed ) {
                return reject(new Error('Database has been closed'));
            }

            var transaction = db.transaction(table);

            transaction.onerror = function ( e ) {
                reject( e );
            };

            var store = transaction.objectStore(table),
                req = store.get( id );

            req.onsuccess = function ( e ) {
                resolve( e.target.result );
            };
        });
    };

This way your using the same Promise instance for all checking, which is more inline with the recommendations.

If you're able to do a PR with the changes I'm more than happy to review them as it might be a bit until I am able to get it done myself.

[brettz9]

I have some commits down the pipeline which deal with error handling, so I should be able to add these kind of changes at the same time.

[cgwrench]

Thanks for the quick response @aaronpowell.

It seems that throughout the code, a string is thrown, rather than an Error

Apologies for raising this string vs. Error issue — I was looking at the source of the latest NuGet package, whereas I should've been checking the latest source in the repository.

Code both rejects and throws;

This is a really good point and I'd never really thought about it, but you're totally right, it shouldn't throw an error, it should reject the promise. More than happy to get this refactored in.

Great to hear this — looking forward to seeing this in a future release.

In a number of places in the code, error handlers appear to be attached too late.

You mentioned that your testing has indicated that the errors don't always get picked up and result in a rejected promise, do you have any repo's which I can see how that condition can happen, I've not come across that myself.

The simplest test case I can come up with which demonstrates this issue is the following:

db.open({
    server: 'my-app',
    version: 1
}).then(function (s) {
    try {
        s.get("table-that-doesn't-exist", 1)
            .catch(function (error) {
                console.log("OnRejection triggered");
                console.error(error);
            });
    } catch (error) {
        console.log("Catch triggered");
        console.error(error);
    }
});

In this case an error is logged, but so is the string Catch triggered. I would expect that the string OnRejection triggered is logged instead. The transaction.onerror handler within Server.get() is not hit in this case. With the modified code given in my original post I see OnRejection triggered logged, as expected.

Note that the same issue appears to affect numerous methods, not just Server.get().

[brettz9]

I believe this must be due to the fact that errors in IndexedDB continue to propagate (and abort). An easier workaround I think should be to add error.preventDefault() to your catch() block. This is currently the approach taken in add() and perhaps ought to be applied in each onerror within db.js so as to avoid the need for any workaround.

[cgwrench]

Thanks for your response @brettz9.

However I'm not sure that I agree with your analysis. If I understand correctly, if the problem was that errors continued to propage after they were handled by a Promise.catch() or onRejection() callback then I would expect both OnRejection triggered and Catch triggered to be logged in the example I provided in my previous post. This is not the case and only Catch triggered is logged.

I beleive that the problem is where the onerror handler is added in the db.js code, not with the calling of the db.js code: the point I am trying to raise with this issue is that I, as a consumer of db.js, should be able to just handle a rejected promise. I shouldn't need a try/catch block everywhere I use db.js, let alone need to add an error.preventDefault() to this catch block.

I did try adding a e.preventDefault() to the transaction.onerror handler in the Server.get() method, as you have suggested, and this did not resolve the issue for me: the promise is still not rejected and a try/catch is still required.

The code given in @aaronpowell's last post does fix this issue and so I believe is the correct change to implement.

I hope this clarifies this a little. Please let me know if you need any further information.

[brettz9]

Sorry, I hadn't meant to word things to definitively as it was not something I had confirmed; I was more speculating, and I do see why there could be such a problem with the errors, especially without a handler ready on the transaction.

[brettz9]

If PR #168 is accepted (and with already-merged PRs #151 and #162 ), this issue should be ready to be closed.

[brettz9]

I think this can be closed now.

[aaronpowell]

Yep, meant to get to that