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

Replace try_get() with get() in several trait implementations for signals #3569

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

mahdi739
Copy link
Contributor

@mahdi739 mahdi739 commented Feb 9, 2025

This PR replaces the use of get() with try_get() for retrieving signal values in these trait implementations:

  • IntoProperty
  • IntoClass
  • IntoStyle
  • Render
  • RenderHtml
  • AttributeValue
  • InnerHtmlValue
    This change prevents potential panics when the signal is disposed.
    As a result, I've replaced most if let blocks in RenderEffects with match statements to handle different variations of the previous state and the signal value better.
    There are some match arms that don't make much sense to me, which usually happens when a signal is disposed, so I just returned None or the previous state if it was possible.

@benwis
Copy link
Contributor

benwis commented Feb 14, 2025

Copied from the other one you've opened, so apologies if you read that one first. Feel free to reply to either one

Overall I think using .try_get() is a decent solution here for panic prevention, but I am worried that instead of getting a panic it seems like this implementation just eats the error. I imagine it would have to emit an error somewhere user visible, whether that's the console in the browser or the server log on the server when the Signal is disposed. I'm not sure returning the previous value is a good choice there either

@mahdi739
Copy link
Contributor Author

mahdi739 commented Feb 14, 2025

Sorry for the duplicated commit history. I wrote (#3582) based on the changes here. Although this PR includes some newer commits not present in the later PR. Generally, I think if you merge this one first and then the later one, commits with the same hashes will be unified.

Most of the time, disposed signals occur when a reactive item is deleted. (For example, deleting an AtKeyed which was copied from within a For component.) I couldn't come up with a scenario where not panicking for disposed signals would cause a problem in this context.
I can return None instead of returning the previous item in some cases. And logging a warning seems like a good idea instead of panicking.
Would you agree to log a warning instead of panicking?

@benwis
Copy link
Contributor

benwis commented Feb 14, 2025

Sorry for the duplicated commit history. I wrote (#3582) based on the changes here. Although this PR includes some newer commits not present in the later PR. Generally, I think if you merge this one first and then the later one, commits with the same hashes will be unified.

Most of the time, disposed signals occur when a reactive item is deleted. (For example, deleting an AtKeyed which was copied from within a For component.) I couldn't come up with a scenario where not panicking for disposed signals would cause a problem in this context. I can return None instead of returning the previous item in some cases. And logging a warning seems like a good idea instead of panicking. Would you agree to log a warning instead of panicking?

Yeah, that sounds reasonable to me

@mahdi739
Copy link
Contributor Author

I added a macro called dispose_warn that uses the reactive_graph::log_warning method to show warnings.

I'm unsure about:

  • The name dispose_warn
  • The warning message
  • The implementation itself

I've pushed the changes for now to get some help here. In bind.rs, I've added an example message, but I need help to make it more precise for different situations, especially when there's more info about the element's name and attribute.

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