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 Dispatchers #639

Merged
merged 21 commits into from
Sep 26, 2019
Merged

Add Dispatchers #639

merged 21 commits into from
Sep 26, 2019

Conversation

hgzimmerman
Copy link
Member

@hgzimmerman hgzimmerman commented Sep 14, 2019

Addresses #307

Dispatchers are similar to bridges, but they don't require a callback to instantiate.

Dispatchers solve the problem of having to create NoOp messages that handle responses from Agents for components that do not care about them. This is a relatively common occurrence, and this change provides a significant ergonomic improvement over handling these NoOp cases.

Dispatchers, when created, will not hook into the typical Component <-> Agent lifecycle. This is because they do not have HandlerIds. So connected and disconnected will not be called when Agent::dispatcher() is called. You can send messages through them, which still reach the handle method on the Agent.

Because of the lack of HandlerIds in dispatchers, a breaking change was required for Agent::handle which now takes an Option<HandlerId> instead of just a HandlerId.

Because it would be pointless to have dispatchers to Job and Private discoverers, its usage is constrained to just Context and Public discoverers.

Copy link
Member Author

@hgzimmerman hgzimmerman left a comment

Choose a reason for hiding this comment

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

Not exactly sure what I'm doing w/ SINGLETON_ID here, and I don't replicate it for Private discoverers.
It should be neither or both.

src/agent.rs Outdated Show resolved Hide resolved
src/agent.rs Outdated Show resolved Hide resolved
@hgzimmerman
Copy link
Member Author

hgzimmerman commented Sep 14, 2019

@jstarry It looks to me like the CI is acting funky (although because of the breaking change, I still probably broke another example, but its failing for what appears to be unrelated reasons)

@jstarry jstarry changed the title Add Dispatchers [Breaking Change] Add Dispatchers Sep 23, 2019
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.

Definitely understand the motivation for this, seems like a nice ergonomic improvement to me. I would like to avoid making a breaking change for the optional handler id, let me know your thoughts! I also made a change to remove some code reuse here: hgzimmerman#1

src/agent.rs Outdated Show resolved Hide resolved
src/agent.rs Outdated Show resolved Hide resolved
examples/routing/src/router_button.rs Show resolved Hide resolved
examples/routing/src/lib.rs Outdated Show resolved Hide resolved
@hgzimmerman
Copy link
Member Author

I still need to address the singleton id stuff, but I have merged your suggestions into this branch.
I added a new-type around Box<dyn Bridge> because I think the different behavior warrants a distinct type. Let me know what you think about that.

@hgzimmerman hgzimmerman requested a review from jstarry September 25, 2019 00:31
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.

Looks like 44aeb47 isn't quite right, can you fix?

Also, I think I have a good solution for HandlerId:

  1. Always insert into slab, whether Callback is Some or None
  2. Change type of slab to accept Option<Callback>
  3. Change HandlerId to (usize, bool)
  4. Add is_respondable() method to HandlerId

src/agent.rs Outdated Show resolved Hide resolved
@hgzimmerman
Copy link
Member Author

Not sure if this is completely done, but I'm sure it gets us closer to the implementation we want. Let me know if there is anything else that needs changing.

@hgzimmerman hgzimmerman requested a review from jstarry September 25, 2019 13:54
src/agent.rs Outdated Show resolved Hide resolved
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.

Add specific warning message if Agent tries to send message to handler with no callback

@hgzimmerman hgzimmerman requested a review from jstarry September 25, 2019 17:55
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.

Looks great! There isn't a breaking change anymore, right?

@hgzimmerman
Copy link
Member Author

Yes, I no longer think that this breaks anything.

@jstarry jstarry merged commit 97f5aa3 into yewstack:master Sep 26, 2019
llebout pushed a commit to llebout/yew that referenced this pull request Jan 20, 2020
* Add Dispatchers, which act like bridges, but don't require a callback to create; updated router example

* cargo fmt

* improve comment

* Another approach

* add newtype around dispatcher bridges

* added debug impl, run cargo fmt

* fix example

* make button on routing example start on loading variant

* revert singleton_id changes

* actually revert singleton_id changes

* slabs own option<callback>

* cargo fmt

* remove dead lines

* address bad doc comment

* fix router example

* fix handler id initialization in local agent

* add appropriate error message when id is not associated with callback

* remove misleading comment

* use a type alias to the shared output slab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants