Improve flexibility of message sending API #999
Conversation
captain-yossarian
changed the title
captain-yossarian
force-pushed
the
issue/932
branch
from
d7dc7d4
to
d4dd3f6
Compare
captain-yossarian
changed the title
src/html/scope.rs
Outdated
| self.update(ComponentUpdate::MessageBatch(messages)); | ||
| pub fn send_message_batch<T>(&self, messages: T) | ||
| where | ||
| T: Into<Vec<COMP::Message>>, |
There was a problem hiding this comment.
Check out this link: https://rust-lang.github.io/api-guidelines/flexibility.html#functions-minimize-assumptions-about-parameters-by-using-generics-c-generic
Let's use I: IntoIterator<Item = COMP::Message> for the batch methods
There was a problem hiding this comment.
@jstarry Thanks for the help!
|
In Instead of onclick=self.link.callback(|_| WsAction::Connect.into())>do onclick=self.link.callback(|_| WsAction::Connect)> |
|
in ///# let callback = link.callback(|response: Response<Result<String, anyhow::Error>>| unimplemented!());You can fix that by adding a return type to the closure like this: ///# let callback = link.callback(|response: Response<Result<String, anyhow::Error>>| -> Msg { unimplemented!() });You can run the doc tests like this: |
|
@jstarry Thank you very much, I'm appreciate it! |
|
@jstarry Tests are passed. Could you please check the types? |
src/html/scope.rs
Outdated
| @@ -133,9 +139,10 @@ impl<COMP: Component> Scope<COMP> { | |||
|
|
|||
| /// This method creates a `Callback` which will send a batch of messages back to the linked | |||
| /// component's update method when called. | |||
| pub fn batch_callback<F, IN>(&self, function: F) -> Callback<IN> | |||
| pub fn batch_callback<F, IN, M: Into<Vec<COMP::Message>>>(&self, function: F) -> Callback<IN> | |||
There was a problem hiding this comment.
Same, here two trait bounds, only need one
src/html/scope.rs
Outdated
| /// Send a batch of messages to the component | ||
| pub fn send_message_batch(&self, messages: Vec<COMP::Message>) { | ||
| self.update(ComponentUpdate::MessageBatch(messages)); | ||
| pub fn send_message_batch<I>(&self, messages: I) |
There was a problem hiding this comment.
On second thought, I don't think this change helps the API. It actually goes against the guidelines: https://rust-lang.github.io/api-guidelines/flexibility.html#caller-decides-where-to-copy-and-place-data-c-caller-control
Since we require ownership over the Vec<COMP::Message> we should have that be the method argument, otherwise we may unnecessarily allocate.
Please revert these changes and the changes to callback_batch. Sorry!
There was a problem hiding this comment.
So, should I keep IntoIterator + Into?
If no, could you please specify more precisely?
There was a problem hiding this comment.
I would like all changes in this PR reverted for send_message_batch and callback_batch
captain-yossarian
force-pushed
the
issue/932
branch
from
af246c6
to
7b35846
Compare
Related to #932