Updating the local storage interface #386

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.

Sign up for GitHub

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

Jump to bottom

Closed

itsgreggreg wants to merge 1 commit into seed-rs:master from itsgreggreg:remove_panics_from_localstorage

Contributor

itsgreggreg commented Mar 10, 2020 •

edited

Loading

Does not expose panics
Enables usage of both session and local storage
Changes name to key in function parameters
Adds a convenience methods for getting all keys, deleting a single key, and clearing all storage
Adds more documentation

Hey guys this was something I ran into when writing an application that used local storage and I figured I'd take a stab at fixing it and cleaning up / extending the API a little. This does change the API so examples should probably be updated.

Anyway I figured I'd post what I've got here and let y'all comment or close this PR as you see fit. Happy to get it working up to the correct standards if you point me in the right direction!

itsgreggreg force-pushed the remove_panics_from_localstorage branch 2 times, most recently from e8b2694 to fa0de25 Compare

March 10, 2020 20:37

MartinKavik reviewed

View reviewed changes

Member

MartinKavik left a comment

The first code review round complete :)

src/browser/service/storage.rs Outdated

-                      .local_storage()
-                      .ok()
-                      .flatten()
+                  web_sys::window()?.local_storage().ok().flatten()

Member

MartinKavik Mar 10, 2020

you can use Seed's window():

seed/src/browser/util.rs

Lines 27 to 29 in 4552e30

    
           pub fn window() -> web_sys::Window { 
        
               web_sys::window().expect("Can't find the global Window") 
        
           }

It should be included in the Seed's prelude.

src/browser/service/storage.rs Outdated

               }
-              /// Create a new store, from a serializable data structure.
-              pub fn store_data<T>(storage: &Storage, name: &str, data: &T)
+              /// Clear all data in local storage

Member

MartinKavik Mar 10, 2020

Do we want to rename the file and methods + types to local_storage / LocalStorage where it makes sense?
It looks like this module only handle local storage, but Storage is general interface for both local storage and session storage. So it will become mess once we add module for session storage and want to write abstraction into module storage...
Or feel free to resolve the "session storage - local storage - storage problem" in another way.

Member

MartinKavik Mar 10, 2020

If you decide to choose renaming storage.rs to local_storage.rs, include also the old storage.rs with deprecated members so we don't introduce breaking changes.

Contributor Author

itsgreggreg Mar 10, 2020

I'm 100% on the rename. Will do

src/browser/service/storage.rs

		@@ -12,33 +12,75 @@ pub type Storage = web_sys::Storage;

		#[allow(clippy::module_name_repetitions)]
		pub fn get_storage() -> Option<Storage> {

Member

MartinKavik Mar 10, 2020

When it fails? - Is Option ok or should we use Result or panic?

src/browser/service/storage.rs Outdated

-              pub fn store_data<T>(storage: &Storage, name: &str, data: &T)
+              /// Clear all data in local storage
+              pub fn clear(storage: &Storage) {
+                  let _ = storage.clear();

Member

MartinKavik Mar 10, 2020

please don't add silent fails - results or panics

src/browser/service/storage.rs Outdated

+              /// All of the keys in local storage
+              pub fn keys(storage: &Storage) -> Vec<String> {
+                  let mut keys = vec![];
+                  let length = storage.length().unwrap_or(0);

Member

MartinKavik Mar 10, 2020

does it make sense to continue when you can't get storage length? - Isn't unwrap_or(0) another silent fail? Let's fail fast. Or if it does make sense, we can use also .unwrap_or_default().

src/browser/service/storage.rs Outdated

+              }
+              /// All of the keys in local storage
+              pub fn keys(storage: &Storage) -> Vec<String> {

Member

MartinKavik Mar 10, 2020

Does it make sense to return Iterator instead?

src/browser/service/storage.rs Outdated

+                  let mut keys = vec![];
+                  let length = storage.length().unwrap_or(0);
+                  for index in 0..length {
+                      if let Ok(Some(key)) = storage.key(index) {

Member

MartinKavik Mar 10, 2020

Another silent fail?

src/browser/service/storage.rs Outdated

+              /// Things that can go wrong when trying to load data from local storage.
+              pub enum LoadError {
+                  /// Could not connect to local storage.
+                  CouldNotConnect,

Member

MartinKavik Mar 10, 2020

What about ConnectionError to make it consistent with DecodeError?

src/browser/service/storage.rs Outdated

+              /// Removes key and associated data from local storage
+              pub fn remove_item(storage: &Storage, key: &str) {
+                  let _ = storage.remove_item(key);

Member

MartinKavik Mar 10, 2020

Silent fail.

src/browser/service/storage.rs Outdated

+              pub enum SaveError {
+                  /// The browser denied saving to local storage. Usually because the storage is full.
+                  /// See: https://developer.mozilla.org/en-US/docs/Web/API/Storage/setItem#Exceptions
+                  CouldNotSave,

Member

MartinKavik Mar 10, 2020

SaveError / SetError? Also we can save also exception content - SetError(JsValue) (applicable to all errors).

Member

MartinKavik commented Mar 10, 2020

And please run cargo make start verify before push to format and lint your code. (It doesn't pass pipelines without it).

MartinKavik reviewed

View reviewed changes

src/browser/service/storage.rs Outdated

+              /// Things that can go wrong when trying to save data to local storage.
+              pub enum SaveError {
+                  /// The browser denied saving to local storage. Usually because the storage is full.
+                  /// See: https://developer.mozilla.org/en-US/docs/Web/API/Storage/setItem#Exceptions

Member

MartinKavik Mar 10, 2020

Change it to something like [MDN - Storage/setItem Exceptions](.. please

itsgreggreg force-pushed the remove_panics_from_localstorage branch from fa0de25 to 7ebd6a3 Compare

March 10, 2020 21:46

Contributor Author

itsgreggreg commented Mar 15, 2020

Thinking about this a bit more, it doesn't seem like there's much to offer on top of the existing web_sys implementation and perhaps simply deprecating the storage module in favor of directly using web_sys is the path forward here. Is there a guiding ux philosophy that seed has to follow to make this decision? Does seed want to wrap the implementation to ensure swapping out the jsbindings can happen without documentation changes?

Member

MartinKavik commented Mar 15, 2020 •

edited

Loading

I try to use web_sys directly as little as possible in Seed apps.

When I need to use web_sys I have to add it into app deps.
Then I have to enable required web_sys features (and there are MANY of them).
I have to choose compatible web_sys version with Seed and update it when needed.
It reflects weird Web API.
web_sys functions often returns only JsValue - you have no idea what it represents until you read documentation and manually cast it into the correct type.
Once there is official support for native API access from WASM, you'll have to rewrite your code.
The main reason - Developer Experience - I think we can write better API with better error handling, better Seed integration, better autocomplete support, etc. In ideal world, developer don't have to read docs or copy-paste example, just explore API with autocomplete and doc comments and use it.

Good example is Fetch API (#353) - it's de-facto only a slim wrapper but the difference for DX and reliability is significant. (And it's also hard to write it well - it's 3rd complete refactor).

Contributor Author

itsgreggreg commented Mar 15, 2020

Righteous, thanks for taking the time to explain that! All good reasons 👍🤙

itsgreggreg force-pushed the remove_panics_from_localstorage branch 5 times, most recently from 1b4f2b6 to ea86a7d Compare

March 22, 2020 15:15


          Updating the Web Storage interface

412236d

* remove panics
* support both local and session storage
* provide detailed error conditions
* provide a fuller interface

itsgreggreg force-pushed the remove_panics_from_localstorage branch from ea86a7d to 412236d Compare

March 22, 2020 18:54

Member

MartinKavik commented Mar 30, 2020

@itsgreggreg Is it ready for code review?

MartinKavik added the 0.7.0 label

Member

MartinKavik commented Apr 17, 2020

Closing in favor of #423.

MartinKavik closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet