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

utility crate: local and session storage #3

Closed
fitzgen opened this issue Feb 26, 2019 · 12 comments
Closed

utility crate: local and session storage #3

fitzgen opened this issue Feb 26, 2019 · 12 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented Feb 26, 2019

We should have a utility crate for working with window.sessionStorage and window.localStorage!

@c410-f3r
Copy link

I will work on it

@Pauan
Copy link
Contributor

Pauan commented Mar 20, 2019

Just FYI, the local storage APIs are heavily discouraged, because they are synchronous, which means they fully block page loading. This causes significant performance issues.

Instead, the recommendation is to use IndexedDB, or the new experimental kv-storage API (not stabilized, and Chrome only for now).

@fitzgen
Copy link
Member Author

fitzgen commented Mar 20, 2019

I think it makes sense to have what we've dubbed "mid-level" APIs for local storage, even if IndexedDB is preferred. At minimum, it will make writing spec-compliant TodoMVC for various frameworks easier 😆

I don't think we should build libraries for anything that is still experimental / not cross-browser supported and part of the open Web yet (like the kv-storage API).

But yeah, would also love to get the ball rolling on Indexed DB too!

@Pauan
Copy link
Contributor

Pauan commented Mar 20, 2019

I think it makes sense to have what we've dubbed "mid-level" APIs for local storage, even if IndexedDB is preferred.

I'm not sure if we should be encouraging bad APIs. Especially APIs with really bad performance (Rust is supposed to be fast!), especially when there's a better alternative.

Instead, I think we should just make the IndexedDB API really good and convenient, so nobody feels the need for local storage.

Using web_sys directly is always an option if people really need local storage.

Also, what would a "mid-level" API for local storage even look like? Here's how it looks like using raw web_sys:

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

store.set_item("foo", "bar").unwrap();

Aside from all the .unwrap()s, there isn't too much to improve upon, since local storage is such a simple API. So what did you have in mind for how to add it into Gloo?

I don't think we should build libraries for anything that is still experimental / not cross-browser supported and part of the open Web yet (like the kv-storage API).

Agreed, I just brought it up because it will (eventually) be replacing local storage, and it also showcases why local storage is bad.

@fitzgen
Copy link
Member Author

fitzgen commented Mar 22, 2019

Instead, I think we should just make the IndexedDB API really good and convenient, so nobody feels the need for local storage.

Indeed! This is a great vision!

Aside from all the .unwrap()s, there isn't too much to improve upon, since local storage is such a simple API. So what did you have in mind for how to add it into Gloo?

Hm, yeah I guess there isn't really anything for a mid-level API to improve upon. A higher-level API could allow transparent serialization and deserialization to and from storage.

Overall, I agree that effort is better probably spent on Indexed DB (although if @c410-f3r is only interested in tackling local/session storage, I think it is worth accepting PRs for it and not rejecting them).

@yoshuawuyts
Copy link
Collaborator

The nanoidb crate might be a good reference for an IndexedDB design.

In particular one gotcha to watch out for when working with IndexedDB is that transactions / batched changes are automatically persisted to the database at the end of the current event loop tick. If not careful this means that you could be trying to persist data to the database after the current transaction has completed, creating some awkward situations.

The way we designed around that was by requiring a .flush() call to be called at the end of the tick, or else we'd throw a runtime error on the next tick (src). Rust doesn't have linear types so we can't enforce a .final() call statically, but I think any design should at least be able to implement the same runtime checks.

@Pauan
Copy link
Contributor

Pauan commented Mar 23, 2019

Rust doesn't have linear types so we can't enforce a .final() call statically, but I think any design should at least be able to implement the same runtime checks.

It's possible to emulate linear types using closures:

db.transaction(|tx| {
    tx.push(...);
    // At the end of the closure the tx is committed
})

Since tx is a reference (not an owned value), it's statically guaranteed that it won't outlive the closure.

(If needed, we can provide an escape hatch that allows for the usage of a transaction outside of a closure, but that should have a lot of warnings in the docs).

@yoshuawuyts
Copy link
Collaborator

@Pauan ohhhh, TIL! -- this is very cool; thanks for sharing!

@Pauan
Copy link
Contributor

Pauan commented Mar 23, 2019

@yoshuawuyts Rust used to have a thread::scoped API which used RAII to allow for using references between threads.

However, this API caused undefined behavior because it was not guaranteed that the thread guard would be dropped.

So it was removed and instead a new API was created to access references across threads.

This new API is safe because it uses a closure to statically restrict the lifetime of the variables. So it's another example of using closures to emulate linear types.

I've found that many issues involving lifetimes in Rust are solved with closures.

@c410-f3r
Copy link

Thanks for this informative conversation that will certainly improve the API design.

@fitzgen
Copy link
Member Author

fitzgen commented Mar 25, 2019

Note that the closure-based api is necessary because the scoped thread cleanup must happen for memory safety. When memory safety is not being enforced, then normal Drop implementations can be used, eg std::fs::File closes the file on drop, and doesn't use a closure-based API because if the file fails to close it doesn't break memory safety.

@ranile
Copy link
Collaborator

ranile commented Aug 26, 2021

Fixed in #125

@ranile ranile closed this as completed Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants