-
Notifications
You must be signed in to change notification settings - Fork 31
Custom storage encoding support and example #85
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for working on this! The bounds are somewhat infectious, but you can get it working without serde if you just follow the errors: main...ealmloff:dioxus-std:decouple-serde I think some of the bounds on the current implementation are unnecessary. For example, the For error handling, it might be easier to handle some of the errors in the |
I finally got around to debugging an issue and getting it working on desktop: the fact that the subscriptions send the unencoded data through the channels tripped me up and I had a bug where I was sending encoded data. Still a lot of cleanup to do, but at least its working. I will continue to make progress on this.
I kept the existing stuff in place to avoid making major API breaking changes, and to limit the scope of the change. I suppose nearly all users will just be using the storage hooks with the existing storage types, so adjusting the traits wouldn't be too disruptive. I'll consider such options as I iterate on this. Reworking this might make it easier to layer the encoding better to avoid issues like the StoragePersistence.store needing the unencoded value. |
I think everything is working as expected now, both on web and desktop. I still need to to do a self code review, add more documentation and add some unit tests. |
/// TODO: | ||
/// Documentation on the APIs implies this just needs to live across reloads, which for web would be session storage, but for desktop would require local storage. | ||
/// Since docs currently say "local storage" local storage is being used. | ||
type Storage = LocalStorage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the behavior of the APIs in this file confusing given their behavior and documentation did not match.
I changed them to use LocalStorage to match the documentation, but that behaves a bit odd since they don't sync.
I'm interested on opinions on what should be done here. If there isn't a clear answer, I can split this into its own PR so it can be sorted out independently.
deserialized.ok() | ||
} | ||
None => { | ||
warn!("Got None for key {key:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually hit this if you delete the local storage entry while the web page is open. That will go on to crash on an unwrap since the current API surface doesn't handle it.
I don't think this is a regression: the old code use to unwrap before it got to this point.
In the future, we should probably have some policy of using the initial value in both empty and error cases, but have an option for the user to provide a custom handler or something. That seems like an unrelated feature and I'm considering it out of scope for now. My changes do mage it easier to implement by plumbing the errors and missing cases up to this layer however.
I think this change is ready for initial review. I ended up pivoting a bit on the design, reworking the StorageBacking trait so its easier to implement and just serves the role of composing together the encoder and persistence. |
An implementation of the proposed feature from #82 .
Included is an example which stores pretty printed JSON in the local storage instead of the current compressed binary format (which ironically in this example takes up much more space.
I attempted to integrated the idea in #82 (comment) and decouple the API/format from serde, but I ran into complications which can be seen in Craig-Macomber@7bda9ab . The extra type parameter introduced to constrain the encodable types leaked out to a lot of places where it caused issues, like StorageBacking.
Due to those issues, this PR contains the addition of support for custom encoding format of serde compatible types, and does not make the coupling to serde opt in.
I'd like some guidance on the preferred way for me to proceed with this work. I'm thinking I should do one of:
Additionally I'd like clarity on how error handling should be done. For example if the storage is filled with some invalid value (possibly from a different version of the app, or collision with some other storage in a different format), I think the default behavior should probably not be to destroy that existing data, nor to panic, but instead produce an app facing error which the app can handle as desired (for example by asking the user if they want to overwrite the data or exit, or attempting to parse the data with a legacy format handler).