Server handlers

[brettz9]

Note that for the onerror test with a bad version (too low), Firefox does not recover. Even if we wrap the open() in a try-catch block, Firefox throws an uncatchable error for such VersionError errors.

I think therefore, the only way we could work around this would be to always open the current version, reflect on what that version is, close the connection, and then either open the connection if the user-supplied version is good, or immediately reject if it is bad. I don't think it is worth doing this though, just to workaround a bug in Firefox. (I haven't reported the issue yet, however.)

[aaronpowell]

So what's the overall goal of this PR?

[brettz9]

For onabort and onerror, it allows adding a higher-level listener (as with DOM element bubbling), without needing to add catch conditions for every promise that is made:

server.onabort(function (e) {
    displayGenericAbortedMessageToUser();
}).onerror(function (err) {
    displayGenericErrorMessageToUser();
});

server.add(...).then(function () {
    server.query(...).then(function () {};
});

The onversionchange event handler, on the other hand, can be used to auto-close the database whenever an upgrade is being attempted in say another tab (and which therefore needs for such old connections as in this tab to first be unblocked). It might also notify the user that the page will be refreshed or otherwise updated in order to take advantage of the new schema.

[brettz9]

Btw, as you've merged the previous PR already, and I've merged your changes, the diffs here should be much clearer/shorter now...

[aaronpowell]

Build failed, and this time it's an actual test failure :grinning:

It might be a PhantomJS problem, but still, it's a failure that's real for once!

[aaronpowell]

I wonder if the events shouldn't operate in the same manner as DOM events, using addEventListener/removeEventListener, rather than just exposing a single function. This would allow multiple handlers and also cleaner removal.

[brettz9]

That's a great idea re: addEventListener.

As far as errors, Chrome is ok, but Firefox is still failing for me on the version checks, and there is a new problem I reported in Firefox in the OP of this PR.

[aaronpowell]

So thinking about this particular failing test - it's most likely a bug in the IDB implementation inside of PhantomJS (well, the QtWebKit it uses) not firing the success event after the versionchange event completes.

This isn't something which we can patch in db.js, it's too low level. Instead for this test what I'd do is move the done() call inside the onversionchange handler as the test is done when that fires, not when the new version is opened. The promise continuation should still close the db if it's called, but it being called is not important to the test passing.

[brettz9]

Btw, regarding addEventListener/removeEventListener, I am thinking that event handlers (similarly to dbCache) should be stored separately according to the particular server and schema version name, but not to a table name (so I think the event listener methods will need to be avoided on the store object as with close).

A different database should have its own separate events, and if the schema has undergone a version change, changes may be serious enough to warrant new events, but I don't see any particular benefit to tying events to a particular store (it might make a little sense for onerror and onabort but definitely not for versionchange).

What do you think?

[aaronpowell]

Why not just delegate down to the addEventListener on the underlying server/objectStore? Isn't that what you're doing with the events already?

[brettz9]

Ok, the addEventListener/removeEventListener work is done. I've avoided added it to the level of the store, since there is not much advantage to it, and the built-in API doesn't do this anyways. I've changed the API to actually use addEventListener as well as using it internally (I had just been using on* such as onerror, etc.).

As far as the PhantomJS error, I'll take a look at making the fix(es) when Travis is back online.

[brettz9]

Hmm..Travis is working again, but apparently I need to get the babel-polyfill added somehow to the grunt file...

[brettz9]

Your approach to workaround the versionchange issue worked--it is now passing again!

One other thing...

Since I changed to addEventListener not only internally, but also for the API, what do you think of the following API to also utilize addEventListener but avoid the bulkier syntax and allow chaining:

server.abort(function (ev) {}).error(function (err) {}).versionchange(function (e) {});

[brettz9]

I've added the shorter event syntax--see what you think...

Getting a new error now in the latest builds though and going a bit nuts trying to figure out the problem...

[brettz9]

Problem solved. I assume you'll want a new PR to squash all of the commits (and not sure Github allows squashing to the same branch).

[brettz9]

Ok, I've rebased my many recent commits down into one and it is passing Travis. Please review so I can start my next PRs. Thanks!

[aaronpowell]

How cool is it that the travis-ci stuff works again :grinning:

[brettz9]

Yes, it is cool... The console logging option I added was also very helpful toward my finally debugging it, and the consequent mess I made of many commits forced me to learn how to rebase.

[brettz9]

And I removed the redundant es6 promises from package.json and Karma.

I will see about whether I can use afterEach to delete the databases to avoid memory issues and start fresh on the dbName creation in beforeEach

[brettz9]

I switched to afterEach for deleting... Still curious why there are problems adding those handlers sometimes for PhantomJS, but I've removed them and added a comment in the files where they were causing problems.

Anyways, the commit is all rebased and ready for review.

[brettz9]

Awesome, thanks!