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

Introduces the FromQuery and IntoQuery traits. #364

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

xfbs
Copy link
Contributor

@xfbs xfbs commented Aug 8, 2023

Introduces the FromQuery and IntoQuery traits to allow for customizing how query strings are encoded and decoded. Introduces the

Because this PR introduces quite a few new things, allow me to walk through it an explain what is happening here.

Query Encoding

Previous Behaviour

The History trait has some methods that allow specifying a query parameter. This query parameter must implement Serialize. It is encoded using serde_urlencoded. These return a HistoryResult<()>, which expands to a Result<(), HistoryError>.

impl History {
    fn push_with_query<'a, Q>(&self, route: impl Into<Cow<'a, str>>, query: Q) -> HistoryResult<()>
        where Q: Serialize;
    fn replace_with_query<'a, Q>(&self, route: impl Into<Cow<'a, str>>, query: Q) -> HistoryResult<()>
        where Q: Serialize;
    ...
}

New Behaviour

We introduce a helper trait, IntoQuery, to help us encode any arbitrary type that implements this into a query string. It has a configurable error (because different strategies might fail in different ways) and a method to encode that takes &self.

pub trait IntoQuery {
    type Error;
    fn encode(&self) -> Result<Cow<'_, str>, Self::Error>;
}

We also add a blanket implementation of this type for any type that implements Serialize with the Error set to HistoryError.

impl<T: Serialize> IntoQuery for T {
    type Error = HistoryError;

    fn encode(&self) -> Result<Cow<'_, str>, Self::Error> {
        serde_urlencoded::to_string(self)
            .map(Into::into)
            .map_err(Into::into)
    }
}

Now, we change the methods in the History trait to constrain on anything that implements IntoQuery instead of Serialize. Since we have a blanket implementation of IntoQuery for anything that implements Serialize, these two constraints are identical at this point. These now return a IntoQuery::Error as an error instead.

impl History {
    fn push_with_query<'a, Q>(&self, route: impl Into<Cow<'a, str>>, query: Q) -> HistoryResult<(),Q::Error>
        where Q: IntoQuery;
    fn replace_with_query<'a, Q>(&self, route: impl Into<Cow<'a, str>>, query: Q) -> HistoryResult<(), Q::Error>
        where Q: IntoQuery;
    ...
}

Before, it was only possible to call these methods with Q: Serialize and it returned a Result<(), HistoryError>. Now, calling it with any type that is Q: Serialize is still possible, and returns the same value. However, it is now possible to implement IntoQuery manually to override an encoding scheme. This is done with the Qs type and the Raw type.

Breaking Changes

Because the new History trait API is a superset of the previous API, this does not constitute a breaking change for consumers of this trait. It does however change the trait, which means that anyone who manually implements the History trait for a custom type (I don't know why anyone would do that, but you could) needs to adjust their implementation slightly. It might be worth looking into sealing the History trait for future-proofing it, otherwise any change, even addition of a method, is a potential (hypothetical) breaking change.

Query Decoding

Previous Behaviour

Query decoding happens in the Location struct, which has a query method.

impl Location {
        pub fn query<T>(&self) -> HistoryResult<T> where T: DeserializeOwned;
}

New Behaviour

We introduce a trait to capture the idea of decoding a query string. This has a Target type parameter, so that a MyCustomStrategy can decode into a String if it wanted to, and an Error parameter, as well as a decode() method.

pub trait FromQuery {
    type Target;
    type Error;
    fn decode(query: &str) -> Result<Self::Target, Self::Error>;
}

Again, we add a blanket implementation for any T that implements DeserializeOwned.

impl<T: DeserializeOwned> FromQuery for T {
    type Target = T;
    type Error = HistoryError;

    fn decode(query: &str) -> Result<Self::Target, Self::Error> {
        serde_urlencoded::from_str(query).map_err(Into::into)
    }
}

With this, we change the method to return a T::Target and an T::Error instead.

impl Location {
pub fn query<T>(&self) -> HistoryResult<T::Target, T::Error> where T: FromQuery;

With this change, all previous invocations still work the exact same way. However, we can now implement FromQuery for custom types to use a custom decoding strategy.

Breaking Changes

Since this is not a trait, and the new definition is a superset of the old, this does not constitute a breaking change.

Tests

Tests have been added for the traits. The test coverage exceeds 80%.

Documentation

Documentation has been added, however it could still be improved with more examples.

@futursolo
Copy link
Collaborator

futursolo commented Aug 8, 2023

It might be worth looking into sealing the History trait for future-proofing it, otherwise any change, even addition of a method, is a potential (hypothetical) breaking change.

I am not too concerned about sealing History as the ultimate goal is to have a way to provide AnyHistory with any type that is 'static + History. So users can implement their own History should they wish to link History to a different data store.

This is a major change (any non-trivial change to item signatures), which means the minor version needs to be bumped.

We have a pending minor release for the next pull request already, i.e.: bumping the MSRV.

Copy link
Collaborator

@futursolo futursolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update changelog.md, including your change?

crates/history/Cargo.toml Outdated Show resolved Hide resolved
@xfbs xfbs force-pushed the custom-query-encoding branch 2 times, most recently from c021711 to 73b26c6 Compare August 8, 2023 16:23
crates/history/src/query.rs Outdated Show resolved Hide resolved
crates/history/src/query.rs Outdated Show resolved Hide resolved
crates/history/src/query.rs Outdated Show resolved Hide resolved
crates/history/src/query.rs Outdated Show resolved Hide resolved
@xfbs xfbs force-pushed the custom-query-encoding branch 2 times, most recently from cc2faf0 to 100e725 Compare August 9, 2023 09:59
crates/history/src/any.rs Outdated Show resolved Hide resolved
crates/history/src/query.rs Show resolved Hide resolved
crates/history/tests/query.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ranile
Copy link
Collaborator

ranile commented Aug 9, 2023

I wonder if there is a way to use URLSearchParams with serde

@xfbs
Copy link
Contributor Author

xfbs commented Aug 9, 2023

I wonder if there is a way to use URLSearchParams with serde

Do you mean something like extracting key-value pairs from a URLSearchParams and feeding that into a serde deserializer?

@ranile
Copy link
Collaborator

ranile commented Aug 9, 2023

I wonder if there is a way to use URLSearchParams with serde

Do you mean something like extracting key-value pairs from a URLSearchParams and feeding that into a serde deserializer?

I mean the opposite of that: using serde to serialize into a URLSearchParams

@xfbs
Copy link
Contributor Author

xfbs commented Aug 10, 2023

That should be possible, actually. Although, you would probably not want to couple it so strongly. So ideally, you'd want something that serializes into a Vec<(String, String)>, and then you feed that into a URLSearchParams. Sounds like a fun weekend project.

@futursolo
Copy link
Collaborator

I think URLSearchParams might not be the best choice for gloo-history as it needs to work with server-side rendering, in which URLSearchParams is not available.

Copy link
Collaborator

@futursolo futursolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

I am approving this pull request as I do not have more comments with the current implementation and we can proceed with it after the change log is updated.

CHANGELOG.md Outdated Show resolved Hide resolved
These two traits are introduced to allow for customizing the query
encoding and decoding behaviour by implementing these traits for custom
types. The previous behaviour of using serde_urlencoded for anything
that implements Serialize and Deserialize is preserved. Customt wrapper
types are added which modify the encoding and decoding behaviour. An
optional feature named `query-qs` is introduced which allows for
optionally encoding and decoding query strings using serde_qs.
Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change!

@ranile ranile merged commit ce9779a into rustwasm:master Aug 11, 2023
17 checks passed
@ranile ranile linked an issue Aug 11, 2023 that may be closed by this pull request
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.

gloo-histroy Support custom query decoder / encoder
3 participants