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

Add a method to emit multiple messages in one callback #660

Merged
merged 5 commits into from
Sep 27, 2019

Conversation

stkevintan
Copy link
Contributor

@stkevintan stkevintan commented Sep 25, 2019

Currently, the send_back method can only trigger one message as the callback return. but there are many common case that need trigger multiple messages, like:
I fetch a post list from server using FetchService, but I cannot fetch all the posts at once, the common way is using a pagination , so suppose I have following two messages:

pub enum Msg {
   SetPosts(Vec<Post>),
   SetPagination(u32,u32,u32), // page, per_page, last_page
   // ...
}

So it's necessary to trigger the two Message when the request is done. send_back cannot do this because Callback only allow to return one Message, so I create the create_effect method.

fn update(&mut self, msg: self::Message) -> ShouldRender {
   match msg {
      Msg::FetchPosts => {
          let callback = self.link.create_effect(
              move |response: Response<Json<Result<PostResult, Error>>>, dispatch| {
                  let (header, Json(body)) = response.into_parts();
                 if  let PostResult(posts, page, per_page, last_page) = body {
                      dispatch(Msg::SetPosts(posts));
                      dispatch(Msg::setPagination(page, per_page, last_page));
                 }
              } 
          )
         self.task = Some(self.fetch_service.fetch(request, callback));
      },
     Msg::SetPosts(posts) => {},
     Msg::SetPagination(page, per_page, last_page) => {}
      // ...
   }
}

There is a work example in my blog source: kirk

This solution is much like redux-thunk

Besides , I am not sure to determine wether to update the send_back method directly or create a new one. maybe adding a new method is the wise choice which will not cause a break change.

@stkevintan stkevintan changed the title add an API add a method to emit multiple messages in one callback Sep 25, 2019
@stkevintan stkevintan changed the title add a method to emit multiple messages in one callback add a useful method to emit multiple messages in one callback Sep 25, 2019
@stkevintan stkevintan changed the title add a useful method to emit multiple messages in one callback Add a method to emit multiple messages in one callback Sep 25, 2019
@hgzimmerman
Copy link
Member

For reference, the existing pattern to accomplish multiple message dispatch per received message is:

pub enum Msg {
    SetPosts(Vec<Post>),
    SetPagination(u32,u32,u32), 
    HandleResponse(Response<Json<Result<PostResult, Error>>>),
    FetchPosts
}
// ...
fn update(&mut self, msg: self::Message) -> ShouldRender {
    match msg {
        Msg::HandleResponse(response) => {
            if let PostResult(posts, page, per_page, last_page) = body {
                self.update(Msg::SetPosts(posts));
                self.update(Msg::SetPagination(page, per_page, last_page));
                true
            } else {
                false
            }
        }
        Msg::SetPosts(posts) => {},
        Msg::SetPagination(page, per_page, last_page) => {}
        Msg::FetchPosts => {
            let request = unimplemented!();
            let callback = self.link.send_back(HandleResponse);
            self.task = Some(self.fetch_service.fetch(request, callback));
            false
        }
    }
}

@stkevintan
Copy link
Contributor Author

@hgzimmerman it's a really cool solution!

However, in my opinion, It is unnecessary to create a new Message (HandleResponse) .
I prefer to keep the code in same place if they are related ,not to seperate them. I think it will help me code be more readable and higher coupling

@jstarry jstarry self-requested a review September 25, 2019 15:58
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

@stkevintan looks good to me, the dispatched messages will be processed sequentially by the scheduler. Only thing is that you could possibly render for each dispatch call when you would probably prefer to only render once, which is bad for performance.

I think we can add a new method send_messages to Scope for sending multiple messages at once. ComponentUpdate::Message can be changed to a list of messages. Then only need to render once and the user does not need to worry. What do you think?

@jstarry jstarry added the feature-request A feature request label Sep 25, 2019
@hgzimmerman
Copy link
Member

hgzimmerman commented Sep 25, 2019

I like @jstarry's proposed solution. It keeps terminal-message specification close to the site of where the callback is created, and keeps additional rerendering to a minimum.

To quickly rewrite the initial proposed code with jstarry's suggestions in mind:

let callback = self.link.send_back_many(|response: Response<Json<Result<PostResult, Error>>>| {
    let (header, Json(body)) = response.into_parts();
    if let PostResult(posts, page, per_page, last_page) = body {
        vec![Msg::SetPosts(posts), Msg::SetPagination((page, per_page, last_page)]
    } else {
        vec![Msg::GetPostsFailed]
    }
} 

If we want to prematurely over-optimize this a little bit, then I would suggest defining:

enum SingleOrMany<T> {
    Single(T),
    Many(Vec<T>)
}

to use for the internal representation in ComponentUpdate::Message. This way we could avoid allocating for the single message case, which I think would still be the dominant variety of sent messages.

@stkevintan
Copy link
Contributor Author

@jstarry @hgzimmerman Sorry for delay.
I just add a send_bunch_back method in my last commit, using the SingleOrMany way, any idea?

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Looking much cleaner!! Just some naming changes and then good to go

src/html/mod.rs Outdated Show resolved Hide resolved
src/html/mod.rs Outdated Show resolved Hide resolved
src/html/mod.rs Outdated Show resolved Hide resolved
src/html/scope.rs Outdated Show resolved Hide resolved
src/html/scope.rs Outdated Show resolved Hide resolved
src/html/scope.rs Outdated Show resolved Hide resolved
@jstarry
Copy link
Member

jstarry commented Sep 27, 2019

@stkevintan can you please run cargo fmt? It looks like indenting changed a lot in your last commit

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

谢谢!

@stkevintan
Copy link
Contributor Author

@jstarry You're welcome~ 🤣

@jstarry jstarry merged commit 9faefde into yewstack:master Sep 27, 2019
llebout pushed a commit to llebout/yew that referenced this pull request Jan 20, 2020
* feat: add create_effect method to help create effect callback

* style: fmt code with rustfmt

* feat: support bunch update in one render loop

* typo: change bunch to batch

* style: fmt with cargo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants