Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add storage crate #59

Closed
wants to merge 1 commit into from
Closed

Add storage crate #59

wants to merge 1 commit into from

Conversation

c410-f3r
Copy link

@c410-f3r c410-f3r commented Mar 30, 2019

This draft is an attempt to unify both Storage and IndexedDB into a single API.

use gloo_storage::*;

let mut s = JsonStorage::with_local_storage("some-storage");
// let mut s = JsonStorage::with_session_storage("some-storage");
// let mut s = IndexedDB::new("some-storage");
s.add_version(1, |v| {
    v.add_and_update_table("first-table", |t| t.add_row_with_index("id", Index::Unique))
});
s.add_version(2, |v| {
    v.add_table("second-table").update_table("first-table", |t| t.remove_old_row("id"))
});
s.transaction( ... );

The design was heavily inspired by Dexie.js and other libs like nanoidb and idb.

Please note that I didn't made a full implementation/test-suite, decoupled logic into layers or wrote a more complete documentation because I would like to know upfront if this is the correct approach to handle persistent data storage in gloo.

@Aloso
Copy link

Aloso commented Mar 30, 2019

Dexie.js only uses Storage, because it has better browser support than IndexedDB. However, since WASM is younger than IndexedDB, you can assume that IndexedDB is supported.

That's why I think concentrating on good IndexedDB bindings would be much more valuable than this. localStorage has a very different purpose, so I don't see much use in merging these APIs into one.

@derekdreery
Copy link
Contributor

Indexeddb is very hard imo

@Pauan
Copy link
Contributor

Pauan commented Mar 30, 2019

@derekdreery It sure is, which is why we want to put the work into making gloo-indexeddb good, so that way other people don't need to.

@Pauan
Copy link
Contributor

Pauan commented Mar 30, 2019

@c410-f3r Thanks for getting started on this!

With regard to localStorage, my personal position is that we shouldn't have it.

Instead, I think we should provide a simple IndexedDB wrapper API which always has the name "" and version 1:

var request = indexedDB.open("", 1);

It would have an extremely simple schema:

request.onupgradeneeded = function (event) { 
    var db = event.target.result;
    db.createObjectStore("", { keyPath: "key" });
};

Then it would just store struct Record { key: &str, value: JsValue } things inside of it.

This gives us the same functionality as localStorage (plus some extra stuff), but underneath it would use the asynchronous IndexedDB API (for performance).

If you want, I can try sketching up what the API would look like.

@Aloso
Copy link

Aloso commented Mar 30, 2019

@Pauan

With regard to localStorage, my personal position is that we shouldn't have it.

localStorage can be useful, too.

For example, you can keep a HashMap in memory and persist it to localStorage only when the user leaves the page or closes the tab (this is impossible with IndexedDB, since pending transactions are aborted when the website is closed).

Also, localStorage supports storage events to get notified when it is modified in another tab. There's no equivalent for IndexedDB yet.

I think it would be best to have two crates: gloo-indexed-db and gloo-storage.

@Pauan
Copy link
Contributor

Pauan commented Mar 30, 2019

@Aloso localStorage only works for very small amounts of data (probably less than a few kilobytes). Anything above that needs IndexedDB.

And even then localStorage has pretty significant performance costs, since it completely blocks rendering.

I've previously explained my position on this issue, so I won't repeat that here.

Experts have said for several years that localStorage is terrible, and browsers are starting to push for an alternative async API which is (surprise) built on top of IndexedDB.

It wouldn't surprise me if within a couple years browsers start to deprecate and remove localStorage, similar to how they deprecated and removed synchronous XMLHttpRequest (which was also done for the exact same performance reasons). But that's just speculation on my part.

For example, you can keep a HashMap in memory and persist it to localStorage only when the user leaves the page or closes the tab

That's a pretty bad idea regardless: there's all sorts of things that can mess that up, including browser crashes, power outages, etc.

The correct thing is to periodically flush to storage (say, every minute or so), so that way it will work regardless of the circumstances.

I've implemented debouncing systems like that for state storage in Chrome extensions, they work well.

I think it would be best to have two crates: gloo-indexed-db and gloo-storage.

I'm not going to stop somebody from implementing gloo-storage, but I would highly recommend we put our effort behind gloo-indexeddb instead, and leave gloo-storage for later (if ever).

It's not hard to use the raw web-sys bindings for localStorage, if somebody needs to do that.

@Aloso
Copy link

Aloso commented Mar 30, 2019

@Pauan localStorage allows up to 10 MB per origin in modern browsers.

And even then localStorage has pretty significant performance costs

I believe that most websites only need to persist small amounts of data, where localStorage's performance is sufficient.

I would highly recommend we put our effort behind gloo-indexeddb instead

I can live with that.

@Pauan
Copy link
Contributor

Pauan commented Mar 30, 2019

localStorage allows up to 10 MB per origin in modern browsers.

I am indeed aware of that. But 10 MB is unacceptable for performance, that's why I said less than a few kilobytes. And even those few kilobytes block rendering.

@samcday
Copy link
Contributor

samcday commented Mar 31, 2019

Just throwing my 2c in here.

I think Gloo should have some kind of support for localStorage since, whether it's terrible for performance or not, it is a part of the web platform that is not (yet) deprecated. As such, it's reasonable to expect that people will want to use it. However I think whatever Gloo provides for localStorage should be an absolute minimum, and only exist to support use cases where for example there's already data in localStorage that one needs to grab out in Rust land (or put in there so some other downstream component in JS can grab it later). Anything higher level should be based on top of Indexed DB since it is obviously better and more flexible than localStorage in just about every conceivable way.

The interesting thing about saying localStorage is bad for performance is that's not strictly true either. If you're running in a web worker, there's absolutely nothing wrong with using localStorage. Ironically, Indexed DB is touted as the better solution since it's asynchronous, but there is (was?) some activity in providing a synchronous API for it to improve the ergonomics of using it in web workers where synchronous blocking APIs aren't an issue.

@c410-f3r
Copy link
Author

With a local/session storage API and an IndexedDB API, the user has the freedom to choose whatever he likes, even if his option is to shoot himself in the foot with a given situation/API. However, as @Pauan said, local/session storage is so simple that it doesn't have a very strong appeal, one has to simply do the following:

let store = window().unwrap_throw().local_storage().unwrap_throw().unwrap_throw(); 

@Aloso

I don't see much use in merging these APIs into one

@Pauan

I would highly recommend we put our effort behind gloo-indexeddb

Looks like the unified approach didn't have much success.


For any decision that has to be made, I am personally cool in doing a more simplistic gloo-indexed-db (like nanoidb and idb) and/or gloo-storage (like store.js) but I still do believe in the unified API and might publish it in a separated crate.

@c410-f3r
Copy link
Author

I think we should provide a simple IndexedDB wrapper API which always has the name "" and version 1

@Pauan You mean that the version feature of IndexedDB shouldn't be exposed to the user? If so, it is possible to have 2 APIs, one that has the version feature, like IndexedDbFull, and other that builds on top of it and hides the version feature, like IndexedDbBare or IndexedDb.

@Pauan
Copy link
Contributor

Pauan commented Mar 31, 2019

@samcday I think Gloo should have some kind of support for localStorage since, whether it's terrible for performance or not, it is a part of the web platform that is not (yet) deprecated.

That's an argument for it being in web-sys, not Gloo.

Gloo isn't intended to handle literally every API, that's web-sys's job.

Though I agree localStorage is a bit of an unusual borderline case, since it is rather commonly used (despite all the pushback against it). So I can't definitively say that it shouldn't be in Gloo.

If you're running in a web worker, there's absolutely nothing wrong with using localStorage.

Originally I thought you had a good point, but then I looked it up, and apparently you cannot access localStorage in a web worker:

new Worker("data:text/javascript,console.log(localStorage);")

Even if you could, because it's a synchronous API, it would allow synchronous access to the main context, which is not allowed with web workers (all communication with the main context must be asynchronous).

Because localStorage cannot be accessed in a web worker, that makes my position even stronger: multi-threaded Wasm is just around the corner, and it uses web workers for parallelism.

So the fact that IndexedDB works with multi-threaded Rust and localStorage doesn't is a pretty big deal.


@c410-f3r I still do believe in the unified API and might publish it in a separated crate.

Sure, I think that'd be a good idea. And if it ends up being useful, we can reconsider putting it into Gloo.

You mean that the version feature of IndexedDB shouldn't be exposed to the user?

Right, the simplified API should expose very little to the user, making it as simple and convenient to use as localStorage.

one that has the version feature, like IndexedDbFull, and other that builds on top of it and hides the version feature, like IndexedDbBare or IndexedDb.

Yup, that was my thinking: have a full featured IndexedDB, and then also a simplified IndexedDB that can handle the use cases that are currently handled by localStorage.

The simplified API would do a lot more than just hide the version: the API would be simplified a lot, and it would have a hardcoded key/value schema, so the user doesn't need to deal with that either.

@derekdreery
Copy link
Contributor

fwiw I would like the API for local/session storage to look like https://github.com/derekdreery/localstorage-rs, or asynchronized if using indexeddb :).

The thing about async is it makes some things harder - like it's harder to know if you've finished iterating through the items in localstorage or not. You could possibly have some synchronization in rust to handle this, maybe, like some atomic ref count which disallows iterating when refs are out and vice verca?

@derekdreery
Copy link
Contributor

Also, I had a play at implementing indexeddb in rust here, but I got stuck understanding all the callback stuff somewhere.

@Pauan
Copy link
Contributor

Pauan commented Apr 2, 2019

@derekdreery like it's harder to know if you've finished iterating through the items in localstorage or not. You could possibly have some synchronization in rust to handle this, maybe, like some atomic ref count which disallows iterating when refs are out and vice verca?

Could you explain more? IndexedDB is transactional and atomic, so it shouldn't be possible to modify the db while a different transaction is iterating over it.

@derekdreery
Copy link
Contributor

Could you explain more? IndexedDB is transactional and atomic, so it shouldn't be possible to modify the db while a different transaction is iterating over it.

I think in that case this isn't an issue. :) It would be if you used window.localStorage

@fitzgen
Copy link
Member

fitzgen commented Apr 2, 2019

We've been trying to front-load more design discussion (as opposed to doing design discussion while looking at an implementation, and potentially having to rework whole PRs due to realizing we want a completely different API).

This design-first process is starting to get formalized in #65.

Looking at the discussion going on here, and the design discussion issue thread for storage-related stuff, it seems pretty clear to me that we don't have a collective understanding/consensus of exactly what we want to implement yet, and there is more design work that needs to happen before we continue with implementation and PRs. Therefore, I think we should close this PR until we have reached broader consensus on the APIs we want to commit to and their design.

Thanks @c410-f3r for writing this PR up. I think the best way to push this forward (either by you or some other motivated person) is to write a design proposal that sketches out the APIs in this PR (or different ones), and their motivation/rationale compared to alternative designs.

@Pauan
Copy link
Contributor

Pauan commented Apr 2, 2019

Therefore, I think we should close this PR until we have reached broader consensus on the APIs we want to commit to and their design.

I agree, as such I'll be closing this.

But I'm excited to see the design work, since I think data storage is really important.

@Pauan Pauan closed this Apr 2, 2019
@c410-f3r c410-f3r mentioned this pull request Apr 3, 2019
@fitzgen
Copy link
Member

fitzgen commented Apr 3, 2019

But I'm excited to see the design work, since I think data storage is really important.

Agreed!

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

Successfully merging this pull request may close these issues.

None yet

6 participants