Prototype

[brettz9]

Change to prototype-based methods internally (to reduce functions added into memory); refactor Query to have succincter syntax (via createReturnObject).

Note that I have not committed dist as it could cause problems upon merges. You can just build dist when doing tests or what not and commit it yourself when you finish a merge or merges. I think this tends to be the most common practice anyways.

[brettz9]

I'm holding off on new pull requests now given that this PR in particular is very involved and would be a pain to resolve conflicts with other work. As such, I humbly request if you could review this PR (and ideally the other ones, particularly #140 with the API docs) before any other incoming PRs...

[aaronpowell]

I actually disagree with this change, the reason it was designed the way that it was is I don't want the db, name or closed made public which has to be done to achieve the change that you've made.

[brettz9]

I just added c05672abcef9061385391c404000d2dba72c680b which uses WeakMap to give real privacy for those three variables at a minimum of cost.

[aaronpowell]

Sure there are other ways, such as you've done, but I don't really get the why of this change, sure you're leveraging the memory reduction by using the prototype change but is it really saving that much? Are you in an application where the memory duplication by not using a prototype chain is causing an issue?

[brettz9]

In my case, my work with db.js has been aimed at ensuring it was fully expressive enough to meet my eventual needs and flexible enough as a wrapper to allow full coverage of the IndexedDB API. Although it is not yet using IndexedDB, I do have an application in mind, and it is very general purpose and large in intent. Especially in the case of not only each store, but each query building functions into memory, I think it is safer to err on the side of caution, even if the code for it cannot be quite as simply expressed.

I'd really like to continue benefiting from your experience and insights and contribute back collaboratively so I am hesitating to contemplate any kind of fork, but I'd really rather not take chances with memory or performance, especially since I am also seeking to contribute to the development of the likes of this Node.js IndexedDB-on-SQLite implementation and hope we can ensure maximum performance in your library for the sake of usage in server environments as well, where there will of course be many concurrent demands placed on such applications. (Given IndexedDB becoming the only option in the client, it makes sense to me as a reusable API on the server as well.)

Besides better memory usage, prototypal implementations are significantly faster, at least in some browsers with Chrome currently being about 20 times faster (see my comment at the end of the discussion at http://stackoverflow.com/a/3493725/271577 ). (Not sure whether Firefox's more recent optimizations in this regard is universal or just for simple cases like that test.)

[brettz9]

So have I persuaded you with my last comment, or do we just have different ideas/purposes with the project?

[aaronpowell]

Simply put, no, I don't believe this PR provides and real-world benefit. Like I said before I understand the point of using prototypal inheritance but I don't agree with using it if you're going to either expose internal state or track internal state in another object (which is really just resulting in a compound memory problem).

Unless you're expecting to do tens of thousands of calls concurrently (and for extended periods) the difference of a few ms is really not going to have that big an impact, and with IDB you're more likely to hit other problems such as it being I/O bound.

I did some simple testing myself using jsPerf (http://jsperf.com/prototype-vs-not) and really, it's negligible the difference for what I believe to be real-world scenarios.

Because of that I will be declining this PR for the time being. If you can produce a scenario where the lack of prototypal inheritance is a performance bottleneck then I'm more than happy to review the usage, review the implementation and look at a solution that meets the goals of keeping internals internal and not a maintenance overhead.

[brettz9]

The compound memory is not much of a problem since the map is only duplicating in the sense of adding references to the current object. If i converted to the new ES6 class syntax, I think it would be even less ugly if that was part of your maintenance overhead concern. Anyways, it's of course your prerogative.

[aaronpowell]

The reason I chose not to convert it to class syntax was because classes don't have the notion of private members.

[brettz9]

Sure, I just meant with the use of WeakMap + class syntax.

[brettz9]

As one alternative to this--why not have one single public property which is unlikely to have a conflict and store db, etc. on there? Besides reflection on name, version, and the closed status (the first two of which are available by reflection on IndexedDB's own API), it could also house the open success event. As you might tell, I get a little uncomfortable walling ourselves off from the full API in case it might be needed at some point...

[brettz9]

Also in this vein, I'd like to add onabort/onerror/onversionchange (and onclose if there is already some browser support) on Server (and so as to be able to listen for these events at a higher level than the transaction where also available on the transaction). Please let me know if you'd be open to such a PR (or for the PR for a public object property)).