Conditially require serialize

[mkawalec]

There is still a single Agent trait, but Reach is now what constraints the Input and Output types. This way we can guarantee Agent signatures to be the same between sync and async agents, with only constraints on the input and output types changing.

One downside of this approach is that we hit this compiler bug, which requires us to write the constraints longform (<Self::Reach as Discoverer>::Output) instead of the standard way (Self::Reach::Output) as there is no ambiguity, despite what the compiler reports.

ToDo:

• Replace the constrained (async) Input and Output with a type alias that captures these constraints to make each repetition less boilerplaty
• Move the code around this file and bring all the Reach definitions closer together (and add some comments) to make it easier to understand how Reaches are defined
• Update other code dependent on this

Fixes: #1212

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #1212: Conditionally require Serialize on Agent's input/output

---

[jstarry]

@mkawalec this is a clever solution!

With these changes, a worker agent implementation would look like this:

impl Agent for Worker {
    type Reach = Public<Request, Response>;
    type Message = Msg;

    // ...

What do you think about keeping the Input and Output associated types? I think we could achieve this by changing the Discoverer associated types from Input and Output to simply Agent and then add serialization trait bounds on Agent::Input and Agent::Output for worker reach agents:

impl Agent for Worker {
    type Reach = Public<Self>;
    type Message = Msg;
    type Input = Request;
    type Output = Response;

    // ...

[jstarry]

Move the code around this file and bring all the Reach definitions closer together (and add some comments) to make it easier to understand how Reaches are defined

Also, I went ahead and addressed this ^ for you here: #1202

[jstarry]

Replace the constrained (async) Input and Output with a type alias that captures these constraints to make each repetition less boilerplaty

Curious what you come up with for this! I wouldn't consider it a blocker though :)

[mkawalec]

Also, I went ahead and addressed this ^ for you here: #1202

That will be a fun merge :P

[jstarry]

That will be a fun merge :P

😅 yeah, didn't think about that. How about I revert that and we get this in first?

RE: trait aliases

It looks much cleaner but I'd like to keep Yew on stable Rust. It will be a lot of ugly copy pasting trait bounds everywhere but it's internal so I think we can live with it.

[mkawalec]

My investigation without running the code seems to suggest that local agents will respond sync (and they did that anyways, even without this change), but now the needless Serialize/Deserialize is not required, so this should also fix #1127

[mkawalec]

I'm getting the most bizzare of errors on 1.40.0, but not on stable, namely

error[E0367]: The requirement `for<'de> <AGN as agent::Agent>::Output: agent::_IMPL_DESERIALIZE_FOR_HandlerId::_serde::Deserialize<'de>` is added only by the Drop impl.
   --> yew/src/agent/worker/public.rs:188:1
    |
188 | / impl<AGN> Drop for PublicBridge<AGN>
189 | | where AGN: Agent,
190 | |       <AGN as Agent>::Input: fmt::Debug + Serialize + for<'de> Deserialize<'de>,
191 | |       <AGN as Agent>::Output: Serialize + for<'de> Deserialize<'de>
...   |
226 | |     }
227 | | }
    | |_^
    |
note: The same requirement must be part of the struct/enum definition
   --> yew/src/agent/worker/public.rs:121:1
    |
121 | / pub struct PublicBridge<AGN>
122 | | where AGN: Agent,
123 | |       <AGN as Agent>::Input: fmt::Debug + Serialize + for<'de> Deserialize<'de>,
124 | |       <AGN as Agent>::Output: Serialize + for<'de> Deserialize<'de>
...   |
131 | |     _agent: PhantomData<AGN>,
132 | | }
    | |_^

looks like a compiler bug fixed in the meantime?

[mkawalec]

Yep, here's the compiler bug rust-lang/rust#34426. Do we want to update the compiler versions we target maybe?

[jstarry]

Do we want to update the compiler versions we target maybe?

@mkawalec yes, feel free!

[jstarry]

My investigation without running the code seems to suggest that local agents will respond sync (and they did that anyways, even without this change), but now the needless Serialize/Deserialize is not required, so this should also fix #1127

This is true but I need to think on #1127 some more. I think the issue there is more about getting info back from the initial bridge call so that a component doesn't need to handle the default case with option wrapping and the like

[jstarry]

Any particular reason you chose to add a trait bounds to Input for Debug?

[mkawalec]

There was something requiring this constraint, I'll make sure it is actually still the case.

[mkawalec]

Also updated the docs in yewstack/docs#89

[jstarry]

Very cool how minimal the example changes were, great work!

[mkawalec]

Thanks for a good review :)

[mkawalec]

Implements #1212

Pull request has been modified.