Skip to content

Conversation

Craig-Macomber
Copy link
Contributor

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:

  1. Cleanup this PR (mostly improve documentation) for merging roughly as is. This may or may not be followed up a second change to decouple this from serde.
  2. Continue attempts to decouple this from serde before trying to merge this PR (In this case I'd like suggestions for what approach to take as my initial attempt didn't go very well).

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).

@ealmloff
Copy link
Member

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 S: StorageBacking is only ever used in a PhantomData, so it shouldn't require the Clone bound. I wonder if this would be simpler if we removed StorageBacking and LayeredStorage and just used the new traits directly?

For error handling, it might be easier to handle some of the errors in the StorageEncoder rather than every time you read or write to the signal. If we can make it easy enough to implement StorageEncoder in userland and we allow state on the StorageEncoder, it could potentially handle fallback for deserialization failures and error reporting

@Craig-Macomber
Copy link
Contributor Author

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 wonder if this would be simpler if we removed StorageBacking and LayeredStorage and just used the new traits directly?

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.

@Craig-Macomber
Copy link
Contributor Author

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;
Copy link
Contributor Author

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:?}");
Copy link
Contributor Author

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.

@Craig-Macomber Craig-Macomber marked this pull request as ready for review August 30, 2025 23:54
@Craig-Macomber
Copy link
Contributor Author

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.

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.

2 participants