Support for other storage backends for larger data storage

[nilbus]

Opening this ticket as a continuation of #23.

@nilbus wrote:

How do you feel about Backbone.dualStorage using Lawnchair instead of directly using localStorage? Lawnchair will persist using localStorage, but if other adapters are available and localStorage isn't supported by the currently executing javascript runtime, lawnchair will attempt each successive adapter until it finds one that works.
Why? It allows the caching layer to work when localStorage is not supported, and it supports indexed-db and websql, which allow storing data > 2.5MB, which localStorage does not (in most browsers). I'm in the latter camp, where 2.5MB just isn't cutting it.

@maxfl wrote:

Yes, I'll work on implementing it. :-)
Re changing to an asynchronous API wouldn't it be better to use callbacks and let people manage them with async.js and the like? I thought > promises/deferreds are becoming less popular these days?
We might be able to use some code from Backbone.websqloffline for the WebSQL store? Otherwise something like Lawnchair or Sticky gives us options for > different storage backends. What do you think?
Also, do the current tests provide full coverage?
PS. I don't use coffeescript...

[nilbus]

@maxfl

As the implementer, you should decide what's best for the async handling. I do have reservations about adding a dependency on a library as large as async.js when (I think) we really only need a small portion of its functionality. That said, I can be convinced if you think that's the best option.

I would not like to use any storage engine directly. I like the idea of using Sticky or Lawnchair, as it lets people choose. You can evaluate which better suits the need here, but it seems like Sticky has an easier API to work with. I was not aware of Sticky when I first looked into this. That said, Backbone.websqloffline could be a useful reference implementation to look at.

The current tests cover most everything, but not 100%. If the test suite is green, you should be able to feel confident that you didn't break anything important. Of course the parts touching Store and maybe localSync will have to be replaced or change. Please feel free to fill in gaps as you find edge cases that aren't covered that should be.

Have fun learning coffeescript! It's pretty easy to pick up. When learning the syntax, I found it useful to use the http://coffeescript.org Try Coffeescript tab to type something in and see if the javascript that came out was what I expected.

[maxfi]

Sorry, let me clarify re async.js. I didn't mean that we should add a dependency for async.js. I meant that if we make the API callback-based rather than promise-based it would be easier to use, by end-users, with popular libraries like async.js. Also, with a callback-based API, having a callback parameter in a method's signature quickly shows that the function is (likely) asynchronous.

At the moment I'm liking the look of Sticky...

[nilbus]

@dghopkins FYI there is work being done on this now

[honi]

I'm currently working on the same thing (the project I'm working on depends on storing > 5MB).

What's the status regarding this issue? Has anybody made any progress?

[nilbus]

@maxfi How's it coming along?

I haven't heard anything outside the comments in this ticket.

[maxfi]

Hey all, I've been on holidays so haven't had time to implement anything yet but have thought about how to get it done. I'll be starting the implementation in the next few days. Cheers.

[nilbus]

Feel free to collaborate with each other if you both want to work on it.
There's plenty of work that needs to go into making this happen.

[honi]

I've made an initial refactor which abstracts the use of LocalStorage to a StorageAdapter, which is async (calls to setItem, getItem and removeItem returns a jQuery Promise).

Then, I've implemented a LocalStorageAdapter which uses LocalStorage (but has an async API as required by StorageAdapter).

Up to this point, everything works and all tests are green. BUT I didn't remove the use of LocalStorage in the tests. Some tests (like the ones for Store) use LocalStorage directly after doing some operation to see if it worked correctly. Since I've setup LocalStorageAdapter as the default, the tests work.

Finally, I've made 2 initial implementations for a StickyStorageAdapter using Sticky and LawnchairAdapter using Lawnchair.

Sadly, these adapters are not working yet, I'm getting some mixed errors. I'm currently working on this to see if it's a problem in the async StorageAdapter refactoring, or something within the StickyStorageAdapter or LawnchairAdapter adapters themselves.

In general, these things need to be worked on:

• Tests shouldn't rely on LocalStorage
• Add tests for StorageAdapter
• Backbone.localSync uses Store (one for each model / collection) which uses Backbone.storageAdapter (one for all). Is this ok, or should each store also have it's own StorageAdapter?

Finally, as a side note, by adding the async StorageAdapter now we have a callback for syncDirtyAndDestroyed.

I've pushed my work in progress in the async branch in my fork. And here are the adapters.

Any thoughts or ideas are welcomed! And @maxfi, let me know if you push something, we can work together.

[nilbus]

I'll take a look next week when I free up. Thanks for the update!

[honi]

Follow up: I've managed to fix the error I was getting, and happy to say everything is working fine with Indexed DB!

I'm using Sticky, and because of this line, empty collections were not persisted locally (because collections are stored as records.join(',') and if records.length == 0, the final result was an empty string that evaluates as a falsy value Sticky didn't want to save).

After fixing that line in Sticky, everything is working as expected. I'll continue to test everything and post back if any progress.

[honi]

I've made some more progress on the matter. Decided to stick with Sticky for now. Added some tests and other stuff. Everything is pushed to async branch in my fork.

I'm already using this on a current project and it works great!

[reubano]

As long as you don't remove promise support I'm all for it. In fact, I have a use case where I want to persist storage to json files so this could definitely come in handy.

[nilbus]

@honi Great work! I turned this issue into a pull request with your async branch. I'll be reviewing it and leaving comments so that we can get this merged in.

[nilbus]

The ; is a coffeescript syntax warning.

[nilbus]

All the places that use $ (deferred creation) should use Backbone.$ for compatibility with apps that change the value of $ for their app. To keep it DRY, I recommend defining $ = Backbone.$ at the top of any file that uses $.Deferred(). Since Dualstorage files are inside a closure wrapper, the redefinition will be limited in scope to this library and will not affect the app.

[honi]

Hi @nilbus

Thanks for the review! To be honest, I gave up on this a while back. In general, my implementation had many flaws. I started working on a rewrite on some of the core stuff to better handle the async nature of the storage backend. But never completed it.

Though If there is some interest in this, I'll give it another try. I'll post back with some improvements so that we can further discuss the async implementation / testing.

[nilbus]

Yes, there is still interest, and this seems like a good start.

I haven't gotten deep into reviewing everything yet. Do you remember still what the issues were?

[honi]

Hi @nilbus, great work!
Sorry I couldn't reply earlier, I'm low on free time right now.

Regarding your list of goals, I had similar ideas back then and decided to try a complete rewrite of the library. I've pushed whatever I had to my fork in the async branch.

I'm not suggesting to continue work on the rewrite, but since I was using Mocha, maybe we can take some ideas from that code.

I was trying to break up the library in smaller pieces, you can find code in src/ and test/ directories. This way each part was easier to test and debug. Though it is all WIP, so don't expect anything to actually work!

[nilbus]

@honi Thanks, I'll take a look at it. Still though, I'm unclear on what problems you ran into with your original implementation that deserved a rewrite. I can understand wanting to improve the code, but it seems like there must have been something more important. Do you still remember?

[honi]

I believe the problems were related to how I was integrating dualStorage with the project I was working on, rather than the library itself.

Anyways, I think one issue was related with the async initialization and initial reset of the offline collections on application startup. I was trying to load several collection from the cache at once.

Maybe this could be one of the new test cases, since I'm sure it's normal stuff to use dualStorage for several collections. So it should work to sync several collections at once from the cache, as well as saving.

[nilbus]

This concerns me a little bit. If a user of this library needs to wait for initialize to resolve (they would), then they would need to call initialize again to gain access to a promise to wait on. This means that the database adapters would have to initialize twice.

This can be solved by either not calling initialize in the library, or altering the StickyStorageAdapter to memoize and return the same promise on a subsequent call, ensuring that initialize is only called once. One benefit to the latter approach is that library users can still use the library if they forget to call initialize. That could also be a downside, because if they don't initialize, then the bugs will only appear when doing things immediately on page load. This however can be worked around by calling initialize internally.

class StickyStorageAdapter
  constructor: (name) ->
    @name = name || 'Backbone.dualStorage'
    @initialize = _.memoize @initialize # only initialize once, and memoize the returned promise

  initialize: ->
    deferred = $.Deferred()
    @store = new StickyStore
      name: @name
      adapters: ['indexedDB', 'webSQL', 'localStorage']
      ready: -> deferred.resolve()
    return deferred.promise()

  setItem: (key, value) ->
    @initialize().then => # ensure initialize is resolved before executing
      deferred = $.Deferred()
      @store.set key, value, (storedValue) ->
        deferred.resolve storedValue
      return deferred.promise()

  ...

[dschien]

Very exciting to see this happening. May I ask what the current status of the async branch is? Any feel for when this might arrive in the master branch?

[nilbus]

I believe async is correct and complete. I have reviewed the code thoroughly and found nothing wrong.

I am currently finishing up a complete rewrite of the tests, which are now all written using Backbone models and collections, rather than testing low level units. These will also serve as better documentation of how to use the library. I'm building the tests on the released version of the library and will then test the async branch using these same tests. The test rewrite lives on the mocha branch.

When the tests are complete and passing on the mocha branch, I will merge both branches into master for people to try out generally, which will begin the 2.0 milestone. I will continue to work on the bugs in the 2.0 milestone until I feel 2.0 is ready to be released. Everything in that milestone that is not backward incompatible will also be released as 1.x.x before 2.0.

You should be able to try out the async branch now. There are non-critical changes on master that are not in async that you could merge into async, but there are conflicts. These are the changes in master since async:

• Fix Unable to use UUID's as permanent ids #117: Don't generate temporary IDs that conflict with UUIDs (David Almilli)
• Fix Destroy model with temp id #115: Don't attempt to delete models from the server that were only local, and call the callback in this case (Micha Reiser)
• Allow Backbone.DualStorage.offlineStatusCodes to include 200 OK as an offline status
• syncDirty, syncDestroyed, and syncDirtyAndDestroyed accept options (and therefore callbacks) which are passed to save and destroy

[dschien]

Many thanks for the very detailed response. I'll try the asynch branch but will not attempt to merge the mentioned non-critical changes at this time. I hope to be using this on cordova with indexeddb on Android and WebSQL on iOS.

[nilbus]

Status update:

• I have converted the test suite from Jasmine to Mocha, which has cleaner support for testing async libraries.
• At the same time, all tests have been modified to use only the public interface, so each should serve as an example of how this library can actually be used.
• Mocha run in browser has a bug mochajs/mocha#165 that manifests only in Chrome and makes the errors meaningless (Script Error). Run the tests in Firefox, Safari, or something else.
• The async branch in this repository is based on this PR, with master merged and several fixes (see git log origin/async ^origin/mocha ^2148a58 --no-merges).
• The tests in the mocha branch all pass against master, but not against the async branch. There's some subtle bug that I wasn't able to determine quickly.

@elad This is where I left off last. Would you mind looking into this last point? If you're not able to make any progress there and would still like to contribute toward the 2.0 release, there are several things there that will be easier to address that are included under the 2.0 milestone issues. Thank you for your help!

[nilbus]

@elad For now I would like to review everything coming into master, so I will make it a priority to do so, at longest within a couple days.

[nilbus]

@SonoIo I haven't reviewed your BackboneIDBDualStorage fork yet and will not likely have time to before February like I mentioned in the other thread. Based on what you've seen though, do you think it would be worthwhile to merge the two projects, given the goals of our 2.0 release? As summarized above, the async branch in this repo uses the StickyStorage adapter to provide IndexedDB support.

Please see my most recent 2 comments above regarding the current status.

[SonoIo]

@nilbus I completely rewrote DualStorage on my own and I've created IDB to talk with IndexedDB. So it isn't a fork, it's a completely new project.

1. I love your adapter concept and the way you wrote it but with large dataset, collections separated by comma are not enough. As an example, Chrome inspector crashes with 1000+ entries. This is the only possible way with localStorage, I know, but not with IndexedDB or WebSQL.
2. Why not split adapters into different projects and distribute only the local one with DualStorage? I could fork the "async" branch and adapt my DirtyStore for bigger dataset to DualStorage. I'm using it with 2,000 rows in a mobile device.
3. Add the common.js compatibility, I can't live without.
4. The last thing that I would like to do is the capability to change the order in which the application calls the adapters. I would like to choose if it goes online -> local or local ->online or, why not, local -> online -> something else?

[nilbus]

@SonoIo

(Numbers edited into your previous comment)

1. In a similar way to how we allow swappable database adapters, we can also allow a swappable Store class, which is what currently keeps the comma-separated index of all records. You could write an alternative IDBStore that uses a more efficient indexing method, with IDB as the backend. If you find that Chrome inspector crashes with 1000+ entries even with StickyStoreAdapter, then I think this would be a good idea.
2. Splitting adapters/stores into different projects is an idea worth considering if they stand well on their own. If they are only generally useful as a part of DualStorage, then better to keep it in the same project. If size is a concern, the build script can take options. If I do end up keeping just one adapter/store and convert the others to plugins/projects, then it will likely be the existing Store/StickyStoreAdapter, since it supports both localStorage and IndexedDB.
3. I know common.js and require.js are similar but different. We already have a require.js (AMD) build. Is it possible for this build to support both, or would common.js support require a 3rd separate build?
4. We are hoping to improve this. See dualsync remote/local options confusing #12 (comment). Suggestions are welcome.

[elad]

First, for CommonJS support check out umdjs/umd.

I'm very interested in IndexedDB as a backend. I agree localStorage doesn't cut it. Unfortunately, BackboneIDBDualStorage lacks documentation. :) @SonoIo, do you mind adding some, or at least note whether the usage and API is identical to DualStorage? (as in a drop-in replacement)

Also, could you elaborate on whether there's support for alternative backends, and so on? it's difficult (without looking at the code, of course) to tell how BackboneIDBDualStorage relates to DualStorage.

[SonoIo]

@nilbus

1 and 2. My store class has a similar interface of yours, so they could be exchanged easily, I think. Just add some promises to my code. The current version fits 80% of user's cases. I could extract IDBStore and make usable for who needs more performance or bigger dataset.
3. You can support common.js, AMD and normal include with one single file. I can't see a problem here.
4. I've read. Let me go deeper with my application that absolutely needs it and I'll share my experience.

@elad

I've created Backcbone.IDBDualStorage because I need it at work quickly. This is why I didn't add documentation. Anyway there is a good set of tests you can read that show you how to use the library. Like DualStorage, IDBDualStorage is Backbone, plus a little initial setup.

[nilbus]

@SonoIo It sounds like your ideas are compatible with this project. Feel free to work on merging if you have the time and desire.

[elad]

@SonoIo I also need this code for a real-world application, which is why I'm interested in the IndexedDB backend in particular. Did you create a separate project so that you could keep it up-to-date?

(If so, then this is also a primary concern for me, too, and I also use a forked version.)