remotestorage / remotestorage.io

[DEPRECATED] Old RS website
50 stars 30 forks source link

remoteStorage.js: deprecation of localStorage and synchronous BaseClient APIs #17

Closed nilclass closed 11 years ago

nilclass commented 12 years ago

Due to problems with localStorage (primary quota), the local cache, that is currently implemented through localStorage will be changed to use another DB option.

As most DBs in browsers use asynchronous interfaces (IndexedDB has synchronous interfaces as well, but those are about to be deprecated), this also means that the synchronous versions of the data access methods (getObject, storeObject, getFile, storeFile, remove) will no longer be supported. Instead those methods will probably return promises in the future, if they are called without a callback.

Databases other than localStorage are not fully supported by all targeted browsers. Currently the blocking requirement of remoteStorage.js is CORS, which is supported by the following browser versions:

As WebSQL is deprecated, the only other databasese we can switch to are IndexedDB and Filesystem.

The Filesystem API only works fully on Chrome >= 23 and Blackberry Browser >= 10.0, so it is not an option. Also using IndexedDB would give us indexes for values, so we don't have to implement them on top of the remotestorage data tree. However, IndexedDB is also not supported in all target browsers, especially in the mobile section. This is a list of target browsers that don't support IndexedDB:

For those browsers the localStorage implementation could remain as a fallback. That means additional features of IndexedDB cannot be used until all target browsers are supported. However all those browsers that don't support IndexedDB support WebSQL, so we could also implement a WebSQL storage backend as a fallback. That way SQL indexes could be used in place of IndexedDB indexes and those benefits remain.

So, I'm interested in what you all think about this. Especially regarding the previous paragraph.

raucao commented 12 years ago

Why is this not in https://github.com/RemoteStorage/remoteStorage.js ? This is the website repo.

nilclass commented 12 years ago

No, this is the mailinglist or something like it. See #2.

raucao commented 12 years ago

From my comment there:

The ideal situation is that we don't need the mailing list anyway, because all discussions are held in GitHub issues, either as suggestions or with/on actual code

Whoever is interested in remoteStorage.js development will watch that repo instead of this one anyway. And you're just clogging up the issues of the website repo. I want to start developing the website this week, and it will be quite annoying with this situation.

raucao commented 12 years ago

Also: GitHub issues don't become a mailinglist, just because someone calls it that. Ever.

raucao commented 12 years ago

Ok, I had a look at some of the topics, and it may make sense to leave them here for now.

But this issue in particular is definitely a remoteStorage.js issue, like some others on the list are as well. I agree that event announcements, app testing requests etc. might best be added here, but not issues that solely concern remoteStorage.js.

silverbucket commented 12 years ago

On Mon, Nov 5, 2012 at 3:38 PM, Niklas Cathor notifications@github.comwrote:

Due to problems with localStorage (primary quotahttps://github.com/RemoteStorage/remoteStorage.js/issues/144), the local cache, that is currently implemented through localStorage will be changed to use another DB option.

As most DBs in browsers use asynchronous interfaces (IndexedDB has synchronous interfaces as well, but those are about to be deprecated), this also means that the synchronous versions of the data access methods (getObject, storeObject, getFile, storeFile, remove) will no longer be supported. Instead those methods will probably return promiseshttp://wiki.commonjs.org/wiki/Promises/Ain the future, if they are called without a callback.

Databases other than localStorage are not fully supported by all targeted browsers. Currently the blocking requirement of remoteStorage.js is CORS, which is supported by the following browser versions:

  • IE >= 10
  • FF >= 15
  • Chrome >= 22
  • Safari >= 5.1
  • Opera >= 12
  • iOS Safari >= 3.2
  • Android Browser >= 2.1
  • Blackberry Browser >= 7.0

As WebSQL http://caniuse.com/#feat=sql-storage is deprecated, the only other databasese we can switch to are IndexedDBhttp://caniuse.com/#feat=indexeddband Filesystem http://caniuse.com/#feat=filesystem.

Although WebSQL is deprecated, it's important to make note that there are still companies using this technology (even for new development projects) for various reasons. So, although we likely wont see new adoption of this in browsers, it will likely still be around for a while.

(Ah, that Filesystem library seems cool. I was wondering if something like this existed. I wonder if we'll see more browsers supporting this in the future. It would definitely increase the possibilities of application development using the browser)

The Filesystem API only works fully on Chrome >= 23 and Blackberry Browser >= 10.0, so it is not an option. Also using IndexedDB would give us indexes for values, so we don't have to implement them on top of the remotestorage data tree. However, IndexedDB is also not supported in all target browsers, especially in the mobile section. This is a list of target browsers that don't support IndexedDB:

  • Safari
  • Opera
  • iOS Safari
  • Android Browser
  • Blackberry Browser < 10.0

For those browsers the localStorage implementation could remain as a fallback. That means additional features of IndexedDB cannot be used until all target browsers are supported. However all those browsers that don't support IndexedDB support WebSQL, so we could also implement a WebSQL storage backend as a fallback. That way SQL indexes could be used in place of IndexedDB indexes and those benefits remain.

So, I'm interested in what you all think about this. Especially regarding the previous paragraph.

As I mentioned in the original issue, I think that having the remoteStorage.js library as an abstracted and uniform API to any of these browser storage APIs is a huge win for developers, and is likely to increase adoption when people can write one app that supports any of these options and degrades gracefully.

I don't like the fact that we'll have to completely give up the synchronous methods - but I think it's a small price to pay and not something that really matters in the long run.

raucao commented 12 years ago

We need a WebSQL fallback at the moment anyway, if we want to support iOS and Android. And not supporting these platforms would be suicide.

jancborchardt commented 12 years ago

Yeah, I guess it’s pretty clear we need to both move forward (using IndexedDB) but also provide fallbacks (WebSQL & localStorage).

On Mon, Nov 5, 2012 at 4:31 PM, Sebastian Kippe notifications@github.comwrote:

We need a WebSQL fallback at the moment anyway, if we want to support iOS and Android. And not supporting these platforms would be suicide.

— Reply to this email directly or view it on GitHubhttps://github.com/RemoteStorage/remotestorage.io/issues/17#issuecomment-10074734.

raucao commented 12 years ago

We don't need much of a localStorage fallback, at least regarding the pressing size-related issues. All remoteStorage-compatible browsers without indexedDB support WebSQL, while localStorage would still be broken for that.

silverbucket commented 12 years ago

I don't see why we should drop support for something we already have. We can't predict that we will never need it. What if there is an app running in a node.js environment? I know there's a localStorage node app that I use to write remoteStorage.js tests that run from the console. There are many edge cases where localStorage could be the easiest thing for someone to fake if they are using RS in a new, unexpected, environment. I say we keep it as a fallback, as long as it's not a significant hindrance on resources.

On Mon, Nov 5, 2012 at 9:48 PM, Sebastian Kippe notifications@github.comwrote:

We don't need much of a localStorage fallback, at least regarding the pressing size-related issues. All remoteStorage-compatible browsers without indexedDB support WebSQL, while localStorage would still be broken for that.

— Reply to this email directly or view it on GitHubhttps://github.com/RemoteStorage/remotestorage.io/issues/17#issuecomment-10087184.

nilclass commented 12 years ago

We can keep localStorage support, it's very simple. I've started refactoring the code to use adapters (branch: store-backends, see localStorageAdapter), so if you want to use nodejs, you can just write an adapter for the data store of your choice and no longer have to mimic the localStorage interface.

If localStorage turns out not to be needed in any environment, we can just take it out of the regular build, but for now it's a good fallback.

We also need localStorage for migration of current data to the new data store. I think the default DB choice will be indexedDB > websql > localStorage, so data migration would go in the other direction.

raucao commented 12 years ago

:+1:

nilclass commented 12 years ago

I've changed the interface for storage adapters once more, this is what it looks like now and how it will probably stay.

silverbucket commented 12 years ago

I search my email and found a thread from Sept 24th, which was initially about deployd, but went on to discuss localStorage limitations, and IndexedDB. Ian Bicking posted a link to this https://github.com/axemclion/IndexedDBShim

Could be useful to cut back on the work needed to support WebSQL through that instead? Just thought I'd post the link in case it was of any use.

On Tue, Nov 6, 2012 at 10:08 PM, Niklas Cathor notifications@github.comwrote:

I've changed the interface for storage adapters once more, thishttps://github.com/RemoteStorage/remoteStorage.js/blob/e806c9f729cfc84e3c3452938f36b2ff2927ef6b/src/lib/store/memory.jsis what it looks like now and how it will probably stay.

— Reply to this email directly or view it on GitHubhttps://github.com/RemoteStorage/remotestorage.io/issues/17#issuecomment-10129862.

raucao commented 12 years ago

It's not a good idea to use a deprecated standard as the default for a new project.

nilclass commented 12 years ago

@skddc what are you referring to?

raucao commented 12 years ago

Oh sry, I read it wrong. I thought the suggestion was to use WebSQL with a shim, not the other way around.

michielbdejong commented 12 years ago

combining the three complaints of speed, quota size, and unpredictability of quota, it sounds like a necessary performance improvement to stop relying on localStorage.

but i do have one remark to make about the asynchronous callback/promise though; whether syntactically a call is synchronous or with a callback or with a promise, doesn't change the fact that we want it back fast (which probably means <10ms). That's why we cache in the first place: An instant local response, followed by asynchronous synchronization in the background, with the spinning cube.

imho the app should never ever sit and wait for the sync, unless 1) you just connected and it's still loading, or 2) you just unpredictably jumped to a part of the app that could not reasonably have been preloaded. All other actions should complete instantly, without a round trip to remote. And i would even say that part of the app design is to not present too many wild navigation options, so that preloading becomes a bit more feasible. But that's the scalability-nerd in me speaking, i guess ;)

You could even go as far as putting a memory cache in front, then flush from memory to local disk once a second, and to remote once a minute or whatever. Then the disk can be as slow as it wants.

adding a memory layer could even be an easier quick win and could buy us some time maybe? just a suggestion.

raucao commented 12 years ago

There are a lot of use cases where you want both instant upload, and give the user the url of what they just uploaded. Every app that includes a sharing feature needs that. For Sharedy, it's even the core feature. So "never ever sit and wait for the sync" is not an option for all cases.

michielbdejong commented 12 years ago

right, i forgot about that one, that's 3) when sharing via remote you also need to wait for a round trip. you could already say "the URL will be: ..." but it would not work until syncing is complete.

michielbdejong commented 12 years ago

also, Sharedy is a good example of why it needs to be easier to push out a big document but not cache it. that doesn't change even if we start using Indexeddb in some desktop browsers.

what would be the right way to do that right now? with baseClient.release(path)? is there a way to make that easier?

raucao commented 12 years ago

Yes, it doesn't have to be cached. But if the library can't decide it itself, I'd rather have an option parameter on storeFile than call it manually in apps.

nilclass commented 12 years ago

but i do have one remark to make about the asynchronous callback/promise though; whether syntactically a call is synchronous or with a callback or with a promise, doesn't change the fact that we want it back fast (which probably means <10ms). That's why we cache in the first place: An instant local response, followed by asynchronous synchronization in the background, with the spinning cube.

Yes, there is no need to wait for the synchronization to finish. But we have to wait until the data is in cache. Also for GETting data that has not been cached, we need to wait for the request.

You could even go as far as putting a memory cache in front, then flush from memory to local disk once a second, and to remote once a minute or whatever. Then the disk can be as slow as it wants.

Sure, that would be simple to implement. I wouldn't flush periodically though, but just write to memory cache, then queue the write task, then return control.

Yes, it doesn't have to be cached. But if the library can't decide it itself, I'd rather have an option parameter on storeFile than call it manually in apps.

Currently all data that is stored by an app is cached. Admittedly that's confusing and not such a good idea. When writing we should also adhere what paths have been use()d an release()d. So for an image app that shows all images and allows to upload them, that would mean it could do baseClient.use(path, true), so the directory listings get synced, but no data. Then all reads and writes on data nodes would skip cache. If it turns out that this is not enough, we can also add an additional parameter to storeFile, but I find it cleaner this way. (the extra parameter would only be needed when you write to a directory that you want to have cached, but don't want that particular file you're storing being cached for some reason - I don't really see a usecase for that right now).

raucao commented 12 years ago

baseClient.use is not exactly expressive naming. Can we improve this to reflect better what it does?

nilclass commented 12 years ago

I'm open for suggestions.

raucao commented 12 years ago

I don't know what it's supposed to be or do and more importantly why it's named the way it is now, so I can't make a suggestion.

michielbdejong commented 12 years ago

use() can be called preload(), and yes it seems reasonable to only cache stuff that the app explicitly preloads. that would definitely solve my current problem in a nice way

silverbucket commented 12 years ago

Yeah, I have no idea what it's for or does as well :) I just know I need to write it in.

On Thu, Nov 8, 2012 at 2:15 PM, Sebastian Kippe notifications@github.comwrote:

I don't know what it's supposed to be or do and more importantly why it's named the way it is now, so I can't make a suggestion.

— Reply to this email directly or view it on GitHubhttps://github.com/RemoteStorage/remotestorage.io/issues/17#issuecomment-10189182.

michielbdejong commented 12 years ago

so would cache() or preload() be a good name? sync() maybe?

raucao commented 12 years ago

What is the function's purpose and why is it named use? Without knowing what it should do, those are all equally meaningless names. As far as I see it, it only sets a preference, changing the behaviour of other functions. If that's true, it shouldn't use a verb as name at all (other than maybe set or configure e.g.).

xMartin commented 12 years ago

I agree with @skddc .

raucao commented 12 years ago

Haha, didn't see that one. :)

elf-pavlik commented 12 years ago

not sure if useful but justed noticed this presentation on MediaGoblin list, which mention HTML5 Filesystem API: http://www.htmlfivecan.com/#26

it comes from Google I/O presentation...

michielbdejong commented 11 years ago

superseded by the ticket @xMartin mentioned above.