keyPath support for indexed-db adapter

[demaniak]

Hi - would be very useful if indexed-db adapter offered support for making use of the keyPath param that IndexedDb is supposed to support.

[MarkMYoung]

@brianleroux, would you be in favor of this being added to all adapters? Currently, the unique identifier is assumed to be 'key' so making default value of 'keyPath' to be 'key' should work perfectly. I can handle updating the SQLite and probably IndexedDB adapters, but I don't have the time to do localStorage (and sessionStorage) and DOM storage too.

Although I see the benefit of adding 'keyPath' support and the benefit of adding it across all adapters, I have to say that IndexedDB is the biggest waste of everything: typical code size is 40+% bigger, it attempts to "replace" technology which can do almost everything it can do, it lacks the decades of implementation, documentation, and understanding of SQL, etc. I fully believe IndexedDB was developed for the sole purpose of restarting the browser DB race.

[brianleroux]

absolutely

[MarkMYoung]

Okay, so the keyPath pull request is ready for discussion and testing (@demaniak). I am still learning Git so I may have produced an unnecessary branch in the process.

[demaniak]

Coolio @MarkMYoung - gonna have a look now!

[demaniak]

Ok, nothing is breaking. AND it seems that keyPath is behaving as expected (meaning if I save a object with keyPath value "123", and an object already exists, then it seems that object gets updated - perfect!).

One comment then (perhaps should be logged as different issue) - the "fall through to first valid adapter" does not seem to work as expected?

I had the adapters linked in this order (yes, I'm greedy, I want to have it all, don't judge me!):

Lawnchair.js
indexed-db.js
webkit-sqlite.js
gears-sqlite.js
blackberry-persistent-storage.js
dom.js
chrome-storage.js
touchdb-couchdb.js

I ended up with some adapter that was NOT indexedDb.

Anyway, after commenting out all other adapters, indexedDb kicked in, and things seem...coherent.

So, @MarkMYoung - with limited testing (which I continue with, since I'm pulling this into a real live project): all seems well!

[MarkMYoung]

Glad to hear it. I think I should let @brianleroux add analogous changes to all the other adapters before merging this branch. Actually, I only knew which lines of code to change because of comments I added to local changes I made over a year ago to serialize non-string keys.

You know, now that you mention it, I include IndexedDB as an alternate storage implementation, but it is listed first (alphabetically) so it should've been used all this time. I guess you should open that as another issue and list any differences in results when you list adapters in the constructor options.

[demaniak]

@MarkMYoung - cool, I will chase the adapter selection weirdness again when I have some time.

In the meanwhile, what is the protocol here? Do I close this issue (since I got what I wanted :p ), does the project owner close the issue? Or the change author?

I could have sworn @brianleroux had some comments on the change still - should those first be addressed, if they were not retracted (since I can't find them now...)?

[MarkMYoung]

Well, @brianleroux made a couple comments (directly to me I guess since I don't see them here). I addressed his code formatting issue (with a commit this morning). His other concern was the number of lines required to achieve this feature. So, I might be making it into a plug-in (maybe called custom-keyPath). Since I wasn't 100% sure how I should do that since it includes methods not used by any other plug-in/adapter, I suggested a methodology for doing that and I'm awaiting his response.

In the meantime, you can use this branch to get what you want. Either the owner or the assignee typically close them. Your testing was greatly appreciated!

[brianleroux]

why not bake all this into just the indexeddb adapter to isolate the code to the thing it works with?

[MarkMYoung]

It works in the SQLite adapter too (in this branch) and can work in any adapter. People have been asking to be able to customize the primary key for a while (some want 'id', some want it not added to the object, etc.). Although I would've just made it a customizable string to a top-level property, IndexedDB's specification is the greatest common factor.

I don't think 145 lines of code isn't too bad for what you get. Not only does it allow for a custom key, it allows for 1) multiple properties to be keys 2) at multiple levels and 3) formalizes key value comparison. I'll commit an update with it as a plugin and you let me know what you think. All that the other adapters need to do is hook into the 'key*' methods

[brianleroux]

👍👌👏🎉🍺

On Tue, Mar 10, 2015, 8:02 AM Mark M. Young notifications@github.com wrote:

It works in the SQLite adapter too (in this branch) and can work in any adapter. People have been asking to be able to customize the primary key for a while (some want 'id', some want it not added to the object, etc.). Although I would've just made it a customizable string to a top-level property, IndexedDB's specification is the greatest common factor.

I don't think 145 lines of code isn't too bad for what you get. Not only does it allow for a custom key, it allows for 1) multiple properties to be keys 2) at multiple levels and 3) formalizes key value comparison. I'll commit an update with it as a plugin and you let me know what you think. All that the other adapters need to do is hook into the 'key*' methods

— Reply to this email directly or view it on GitHub https://github.com/brianleroux/lawnchair/issues/211#issuecomment-78058599 .