Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Lawnchair for multiple storage backends #23

Closed
wants to merge 19 commits into from
Closed

Lawnchair for multiple storage backends #23

wants to merge 19 commits into from

Conversation

nilbus
Copy link
Owner

@nilbus nilbus commented Dec 5, 2012

Lucian,

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.

This would require me first to write a suite of unit tests to make sure there aren't regressions. Since lawnchair calls are asynchronous, it'll require some refactoring of the internals, but the external API will be able to stay the same. I'm of course willing to implement all of this, since you've retired from this project. I just wanted to make sure that this kind of change is in line with your vision for the project before I do this in a fork of Backbone.dualStorage rather than adding dual-layer support to Backbone.lawnchair. Does this sound like something you would be interested in merging in eventually?

@lucian1900
Copy link
Collaborator

Funnily enough, the project for which I eventually wrote dualStorage, I'd used lawnchair for initially. I stopped using it because it was very buggy (at least at the time) and it exposed an async API when the only backend I cared about (localStorage) has a synchronous, blocking API.

Since Backbone.sync is an asynchronous API anyway, it should be possible to use lawnchair. Feel free to port it, while keeping in mind that you will most likely end up as its maintainer :)

@nilbus
Copy link
Owner Author

nilbus commented Sep 18, 2012

That sounds good to me. I'll give it a shot and see how things go.

Do you remember what sort of bugs you ran into during your earlier experiences with lawnchair? Knowing that would be helpful to know what to look out for.

@lucian1900
Copy link
Collaborator

The released version at the time returned garbled data for everything I tried to store. I remember that cloning the repo and making my own release worked, though.

npm is weird and doesn't link executables for dependencies.
It also doesn't install devDependencies with npm install --global.
See https://github.com/isaacs/npm/issues/1167#issuecomment-9057455
Skip the intermediate variable of helper -> window
The localStorage implementation of Store happens to overwrite existing models
when create is called with the same id as an existing model in the Store.
Underlying implementations may rely on this behavior, so I'm adding it to the spec.
This largely tests that localsync delegates its operations to the underlying store.
Also consistently use destructuring assignment to declare variables.
The underlying store may eventually change.
Vim's .swp files were messing it up.
We need to add in callback testing first.
localStorage is a synchronous API, but other storage APIs are not.
We must not assume that the work we request is done until a callback fires.
This will be important for future integration with other storage backends.
By creating the spy in the context, it's available to the code in the context.
See issue this works around: http://stackoverflow.com/questions/13141316/after-modifying-attributes-on-a-node-js-vm-context-sandbox-the-modifications/13145366#13145366
@nilbus
Copy link
Owner Author

nilbus commented Dec 5, 2012

Hmm, I should attach these tests to a different pull request. I'm going to close this and open a new one.

@maxfi
Copy link

maxfi commented Dec 3, 2013

Hi @nilbus. Did you ever end up working on implementing WebSQL as the datastore? I have the same issue as you described in the first post.

@nilbus
Copy link
Owner Author

nilbus commented Dec 3, 2013

No, I ended up leaving the project that had that requirement. I'm still interested in making this happen eventually, but most of our other issues are higher priority. Let me know if you want to work on implementing that, and I would be happy to assist.

The biggest thing to overcome to make this happen is switching from the synchronous API of localStorage to the asynchronous API of Lawnchair/IndexedDB/WebSQL. Using a promises may come in handy here.

@maxfi
Copy link

maxfi commented Dec 4, 2013

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...

@lucian1900
Copy link
Collaborator

Popularity should not prevent choice of the correct abstraction, I think :-)

Backbone's API is asynchronous, so it should be possible to use an async backend. Its API is also callback based, which could be an argument for not using promises, since an extra dependency might not be appreciated.

@nilbus
Copy link
Owner Author

nilbus commented Dec 4, 2013

Let's continue this discussion in #37, since this pull request is already closed.

@maxfi
Copy link

maxfi commented Dec 4, 2013

You accidentally referenced #37 instead of #73.

@nilbus
Copy link
Owner Author

nilbus commented Dec 4, 2013

Whoops, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants