Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

LocalStorage Adapter #44

Closed
daleharvey opened this Issue · 38 comments
@daleharvey
Owner

We could do with lots of adapters, but localstorage is probably the most important

@gabrielmancini

about adapters how http works?

@daleharvey
Owner

Sorry what is the question

@gabrielmancini

i don't understand how the httpAdapter works.
where is stored the data? and how pouchdb behaviour with couchdb in replication subject?

@daleharvey
Owner

the http adapter is storing the data in an online couchdb via http (its a couchdb client)

Each of these adapters needs to implement the same api for replication to work

@jchris
Owner

I propose we rename this one to Works on iPad :)

@daleharvey
Owner

lol, did you try out the websql one? to be honest between idb and websql I think we will have enough bases covered, I dont really see anyone taking the time to work on this if we already have the major bases covered, especially considering how terrible localStorage is

@jchris
Owner

I have an in-brain filter to /dev/null anything about WebSQL, so I didn't try it. I guess I should :)

@daleharvey
Owner

Hah, I think its best to close this as I dont think anyone is going to work on it, and with websql + indexeddb we have a very good browser coverage

@daleharvey daleharvey closed this
@daleharvey
Owner

Reopening for hood.ie (cc @janl if you were wanting anything to keep track)

@daleharvey daleharvey reopened this
@daleharvey
Owner

Was mentioned in another thread that it may be a good idea to implement this by bundling lawnchair http://brian.io/lawnchair/ so we can also have support for weirder browsers

@jo
Owner
jo commented

also of interest: Local Forage

@nickcolley
Owner

Rather than bundling lawnchair etc would it be easier to just have a localstorage adapter specific for Pouch? Going by localForage's info that's the only one Pouch needs to work everywhere.

@calvinmetcalf
@jo
Owner
jo commented

anyway

@nolanlawson
Owner

#1174 was where we discussed Lawnchair. It seems a better choice than Local Forage, because it supports IE userdata and a BlackBerry-specific storage where localStorage is unavailable, effectively extending our support all the way back to IE6. and some other exotic browsers

+1 to @calvinmetcalf 's point that this should start life a separate plugin.

@nick-thompson

I just want to chime in quickly to point out that if we invested in #1250, these questions wouldn't be an issue. Do we bundle it into core? It doesn't matter; anybody who's against having it bundled in core can re-build with only the adapters they need painfully easily:

// entry.js
var PouchDB = require('PouchDB');

function factory(location, options) {
  opts = extend(opts, {
    db: whateverYouWant
  });
  return new PouchDB(location, opts);
}

module.exports = factory;

... and then just browserify it.

Do we write the adapter with localStorage directly? Lawnchair? LocalForage? Who cares! Embracing the levelup ecosystem means that we can write whatever adapters we deem most important, and anybody who wants weird browser support and isn't getting it out of our adapter can fork it and rewrite it with Lawnchair or whatever he wants to because it would be so easy.

The only reason we're debating these questions is because the current architecture of the project is too tightly coupled for anyone besides serious maintainers to get involved.

I wish I had more time to just get #1250 rolling, and I'm sorry that I don't; I'll do what I can to clear some time in the coming days/weeks for this, but in the mean time, as I see discussions like this, I feel like I need to reiterate that #1250 could make so many of these "issues" particularly irrelevant.

@daleharvey
Owner

We can already trivially include and exclude adapters in builds

Using lawnchair vs localstorage matters because it would give us platform support without writing more code, its the same question of using leveldown backends

The levelup stuff is great but at a guess its going to take a while to get working stable, it doesnt really have much bearing on this issue

@nick-thompson

Using lawnchair vs localstorage matters because it would give us platform support without writing more code

The point I'm more concerned about is that, right now, the decision we make here is important. Either we choose Lawnchair or we choose localStorage, for example, and in making that decision we have to closely evaluate the tradeoffs. How important are the extra bytes in our production build compared to the increased platform support? Evaluating this tradeoff matters because once we decide and implement it, that will be a lot of work, a lot of code, and nobody will revisit it any time soon - why? Because our codebase is tightly coupled and the only people who can make any headway are people very familiar with the code.

The reason it doesn't matter with levelup is that a brand new pouchdb user can evaluate the tradeoff himself, and if there is not already a levelup adapter built for the result of his personal evaluation, implementing it has MUCH less overhead than trying to implement the analogous in PouchDB's current implementation.

@nick-thompson

Additionally...

We can already trivially include and exclude adapters in builds

Right - we can. This doesn't have to be solely our decision, and our users don't have to be tied to our decision. Reorganizing PouchDB can put that flexibility in our users hands.

Granted, a user could set up the pouchdb env and read into the code to learn how we choose the adapters that go into the build, tweak it, and build his own file, but I would again argue there's much more overhead there than the example I suggested above.

@daleharvey
Owner

So for those looking to work on this, alternative backends to our level adapter should now work, so creating a build using https://www.npmjs.org/package/localstorage-down shouldnt be a huge amount of work, I would suggest starting with this as its going to be the quickest way to get to a working solution, an custom adapter may very well be needed, but first make it work, then make it fast

following #1478 and #1241 should give you more of an idea how how to use alternative backends,

You will likely want to remove websql indexeddb, and the process.browser check from https://github.com/daleharvey/pouchdb/blob/master/lib/index.js#L21

@daleharvey
Owner

So for an update, calvin made a start on the work I mentioned about getting the adapters setup into a testable state https://github.com/daleharvey/pouchdb/tree/indexedlevel, the branch isnt working yet, but it should give a headstart

@daleharvey
Owner

adam mentioned level.js doesnt implement .delete, max said he would take a patch for it, and currently uses https://github.com/maxogden/level.js/blob/master/testCommon.js#L11-L26 as a workaround, will need to fix that first as we arent gonna test much without delete

@qs44

We added the following to ./node_modules/level-js/index.js:

Level.destroy = function (dbname, callback) {
  var request = indexedDB.deleteDatabase(dbname);
  request.onsuccess = function(event) { 
    console.log('success', event); 
    callback(); 
  }
  request.onerror = function(event) { 
    console.log('error', event); 
    callback(); 
  }
}

This allowed additional tests to pass, but it appears that not everything is being destroyed properly, causing other tests to fail. Are we doing something wrong here?

@calvinmetcalf
@adamshih
Owner

@qs44 forgot to mention that onsuccess appears to be called, but yeah including the error to the callback is probably a good idea

@calvinmetcalf
@daleharvey
Owner

I think this is gonna be a pouch issue more than anything level.js can fix

@qs44 / @adamshih So I think whats best is, 1. your code is good, I would submit a pull request to level.js so you dont need to maintain a patched version, now limit your tests to only GREP=test.basics.js

Then its just straight up debugging, feel free to post failures as you hit them and we can take a look at whats wrong

@maxogden

i'd merge a PR for this in level.js

@qs44 qs44 referenced this issue in maxogden/level.js
Merged

Added .delete function #29

@qs44

Screenshot of failing basics tests with .delete function mentioned above

For the first test, the error does not show up the first time a different id is used. For instance, changing 'yet_another' to 'new_id' will succeed on one run and then fail on the next if 'new_id' remains. The first ones do not appear to be cleaned up.

For the following mismatch errors, the doc_count and total_rows are indeterminate and seem to increase after each test run.

@calvinmetcalf
@qs44

https://github.com/qs44/pouchdb/tree/44-localstorage-adapter

@adamshih and I got test basics working using what was suggested on Monday. There is definitely a lot of room for neater code, but we wanted to ask how best to approach it with what we have.

The first major addition was the level.js bundle that we included in tests/deps as per @calvinmetcalf and @daleharvey 's suggestions. However, to get all the tests to pass, we had to include the opts object (containing adapter and db) in every PouchDB.destroy() call. What would be the best way to clean this up? It seems like the test files need to be restructured to support passed-in opts, or we have to somehow adjust the way {adapter: 'leveldb', db: somebackend} are handled.

@calvinmetcalf
@daleharvey
Owner

Yeh as @calvinmetcalf said, we dont want to change all the tests

I would say the first thing would be to create an alternative index.js file, index-level.js or something, use that to create a distribution of pouchdb that packages uses level.js by default, level.js should get included via browserify, and a few patches (like qs44@28dbfe4#diff-7cad5e3b1c1c9c07e33704f332ec9359R715) can be merged to core

you can have it do INDEX=index-level.js npm run build / INDEX=index-level.js npm run dev just uses the level distribution

I think the hard part of this might be the changes needed to package.json, I believe they should all be possible to replace them with options to the browserify command, so its likely you will need a build.js / build.sh file to be called on nom run build that handles the differences between default

As you have the basic suite passing, thats an awesome step, I think now we want to have it so you can work on it (+test localdown etc) without too much branched, so its figuring out how to get your studd merged while not breaking the default package

@calvinmetcalf

ok so this change 9c6d2d0 gets all but 10 non-migration, non-attachment tests to path, the main thing it does (besides some stuff that tricks it to use leveldb over idb) is to have browserify replace leveldown with level-js

@adamshih
Owner

Finally getting back to the localstorage adapter conversation :)

Trying to swap in localstorage-down via the new levelalt adapter: adamshih@b87db54

But the callback doesn't seem to be hitting dbDeleted, instead redirects browser to http://localhost:8000/tests/_pouch_test_basics immediately. Probably forgetting something silly, but not sure atm, ideas?

EDIT: patched localstorage-down for the above

@nolanlawson nolanlawson referenced this issue in hoodiehq/hoodie.js
Open

Migrate To PouchDB #8

@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt 19c5789
@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt 84ccfd7
@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt 92c7af5
@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt d0d148e
@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt da2da7d
@adamshih adamshih referenced this issue from a commit in adamshih/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt 0e51cf3
@adamshih
Owner

Update: localstorage-down is now a LEVEL_BACKEND parameter for levelalt.

Trying to get attachment tests to pass, but dealing with a problem when attachments are being saved using the sublevel and levelup put method -> using binary valueEncoding with levelup in browser errors rvagg/node-levelup#241

@sygi sygi referenced this issue from a commit in sygi/pouchdb
@qs44 qs44 (#44) - Add localstorage-down to Travis tests f1f3fb6
@sygi sygi referenced this issue from a commit in sygi/pouchdb
@qs44 qs44 (#44) - Make backend usage a bit more flexible 5b8fccc
@sygi sygi referenced this issue from a commit in sygi/pouchdb
@adamshih adamshih (#44) - Add localstorage-down option to LevelAlt 7a43d3d
@qs44 qs44 referenced this issue from a commit in qs44/pouchdb
@qs44 qs44 (#44) - Fix worker tests for alt backends a5eb381
@qs44 qs44 referenced this issue from a commit in qs44/pouchdb
@qs44 qs44 (#44) - Fix worker tests for alt backends fe25773
@daleharvey
Owner

Now passing all tests! 613363e

@daleharvey daleharvey closed this
@nickcolley
Owner

Sweeeeeet :dancer:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.