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

ComponentLink: Allow send_message and send_message_batch through shared references #931

Merged
merged 1 commit into from
Feb 10, 2020
Merged

ComponentLink: Allow send_message and send_message_batch through shared references #931

merged 1 commit into from
Feb 10, 2020

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Feb 9, 2020

I don't know why these functions had &mut self parameters before, but the implementation doesn't need it and it can be worked around by using link.callback(|_| message).emit(()) instead of link.send_message(message) (or by cloning) so it seems very unlikely this is intended.

Example:

let _key_listener_handle = KeyboardService::register_key_press(
    &document(),
    (move |e: KeyPressEvent| {
        if e.key().as_str() == "s" {
            link2.callback(|_| Msg::FocusInput).emit(());
        }
    })
    .into(),
);

@jstarry
Copy link
Member

jstarry commented Feb 10, 2020

Ah yikes, this is tricky. Thanks for bringing this up. The workarounds you mentioned were unintentionally allowed recently. The intention is that it should be impossible to send messages inside Component::view because it could create an infinite update loop.

These two changes improved ergonomics #749 and #780 but introduced the workarounds :/

I think we can close this PR and introduce an issue describing the potential for infinite loops. We will need a way to prevent that.

@jplatte
Copy link
Contributor Author

jplatte commented Feb 10, 2020

I think that kind of infinite loop should be caught at runtime, if at all possible. I'm pretty sure removing Clone from ComponentLink will break lots of code without a clear way to fix it.

@jstarry
Copy link
Member

jstarry commented Feb 10, 2020

Yeah, I think the changes I mentioned have improved things a lot so I wouldn't recommend reverting. Maybe documentation and noticing infinite loops in the runtime is enough.

Anyways, does this PR fix an actual problem or did you open it for consistency with the other API methods?

@jplatte
Copy link
Contributor Author

jplatte commented Feb 10, 2020

It's not really a problem because there are the workarounds, but if they would be reverted I wouldn't know how to implement what I did, which is conditionally send a message to the component in an event handler.

@jstarry
Copy link
Member

jstarry commented Feb 10, 2020

It's not really a problem because there are the workarounds, but if they would be reverted I wouldn't know how to implement what I did, which is conditionally send a message to the component in an event handler.

Ah cool, can you add a code snippet to the PR description that shows why you needed this change? It helps if we ever look back at this PR to understand why the change was made. Thanks!

@jstarry jstarry merged commit 1ede167 into yewstack:master Feb 10, 2020
@jstarry
Copy link
Member

jstarry commented Feb 10, 2020

Created this issue: #933 where we can discuss how to handle the infinite update loop issue

@jplatte jplatte deleted the send_message_shared_ref branch February 10, 2020 10:12
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.

2 participants