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

[mahdi739]

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]

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]

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]

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]

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.

[benwis]

Overall I like it, although I'd definitely want @gbj to review it as this is definitely his domain.

The error message itself should probably match whatever we show in the browser console, which my github-fu has somehow failed to find. reactive_graph::log_warning has this TODO on it // TODO remove this, it's just useful while developing which may or may not be indicative of its eventual fate.

I appreciate you taking the time to make the PR!

[mahdi739]

Thanks. Yes, especially now that there's a conflict with PR #3618.

[mahdi739]

I was thinking about a new implementation that uses a concrete type to address the limitations of trait specialization. This way, the numerous macro rules currently written could be replaced with an implementation that returns the concrete type. I don’t yet have a clear path to write it, but I think it’s similar to what’s already done in bind.rs

[gbj]

A couple comments:

1. Using log_warning is fine -- while it does have the // TODO comment to remove it, it's also what I use for the other signal-related warnings so it won't actually be removed.
2. This should probably be targeted against the leptos_0.8 branch as it includes breaking changes. I'm not sure why cargo-semver-checks is okay with it
3. It looks like in a number of places this adds a bunch of logic, where the previous version just delegated out to the implementation of the same method for move || signal.get() I have not reviewed it all but I have some concerns about the possible effects on binary size, and also on the maintainability here as it makes the code base larger.

It seems clear that you've encountered an issue with signals panicking in the renderer in some scenario but I am not clear on what that is, and I wonder whether addressing the root cause of that issue may be productive. Could you provide an example that currently panics when it shouldn't?

[mahdi739]

A couple comments:

1. Using log_warning is fine -- while it does have the // TODO comment to remove it, it's also what I use for the other signal-related warnings so it won't actually be removed.
2. This should probably be targeted against the leptos_0.8 branch as it includes breaking changes. I'm not sure why cargo-semver-checks is okay with it
3. It looks like in a number of places this adds a bunch of logic, where the previous version just delegated out to the implementation of the same method for move || signal.get() I have not reviewed it all but I have some concerns about the possible effects on binary size, and also on the maintainability here as it makes the code base larger.

It seems clear that you've encountered an issue with signals panicking in the renderer in some scenario but I am not clear on what that is, and I wonder whether addressing the root cause of that issue may be productive. Could you provide an example that currently panics when it shouldn't?

Thanks for checking.
There’s not actually an unexpected panic here. The issue comes up when a signal is used in a situation where it might already be disposed. In those cases, as a user, we need to use try_get() instead of get() to gracefully handle the possibility of it being disposed. The downside is that this forces us to write a closure like move || signal.try_get() everywhere we’d normally just pass the signal directly, which kinda defeats the convenience of doing that.

What I’m aiming for is a way to handle this internally by using try_get() under the hood, so it’s seamless for the user. You might say this is something the user should manage themselves, and yes it make sense. But right now, in a lot of places (like rendering) it already supports optional values, so we could treat a disposed signal as None without much trouble. That said, there are a few spots where I’ve had to add a default value, like for IntoClass for (&'static str, $sig<bool>) I treated disposed signal as false, to make it work.

I’ll admit the current implementation feels a bit verbose. I tried to reproduce something like Bind. I implemented the InnerHtmlValue like this:

impl<T, V> InnerHtmlValue for T
where
    T: Into<Signal<V>> + Send,
    V: InnerHtmlValue + Clone + Send + Sync + 'static,
    V::State: 'static,

But the compiler said there is conflict with other implementations which I guess you've experienced it before.
Generally I think there is more simple design that unifies some repetitive implementations.

And about branching, sure if you generally agree with current direction I’ll try rewriting it in the 0.8 branch.

[gbj]

The issue comes up when a signal is used in a situation where it might already be disposed. In those cases, as a user, we need to use try_get() instead of get() to gracefully handle the possibility of it being disposed. The downside is that this forces us to write a closure like move || signal.try_get() everywhere we’d normally just pass the signal directly, which kinda defeats the convenience of doing that.

Hm... But I think my question continues to be: Do you have an example where the application is written correctly but you need to do this? I think that's what I'm looking for in order to understand it.

I can understand occasional edge cases where you might want to dispose of a signal early and need to use .try_get() if you've done that already. I certainly wouldn't say that users should write a closure like move || signal.try_get() everywhere they'd normally just pass the signal directly — that, to me, looks like a way of papering over incorrectly-written code.

[mahdi739]

Hm... But I think my question continues to be: Do you have an example where the application is written correctly but you still need to do this? I think that’s what I’m looking for to understand it better.

I can understand occasional edge cases where you might want to dispose of a signal early and need to use .try_get() if you’ve done that already. I certainly wouldn’t say that users should write a closure like move || signal.try_get() everywhere they’d normally just pass the signal directly—that, to me, looks like a way of papering over incorrectly written code.

To clarify, remember that thread in Discord where we discussed this, and you shared a working example? Here’s the link:
https://discord.com/channels/1031524867910148188/1333525437992669357/1334916806057656401

I tweaked it a bit. I created a separate struct for session items and then used the selected session’s fields reactively in a <strong> tag at the bottom. That’s where I ran into the signal-being-disposed error. Here’s the code:

<strong>
  "Selected Item: "
  {move || {
    selected_session
      .try_get()
      .flatten()
      .map(|item| {
        view! { {move || item.field1().get()} }
      })
  }}
</strong>

So, creating a separate RwSignal might work for accessing selected_session itself, but when it comes to accessing and passing its subfields, the error still pops up.
You can check out the full code here:
https://github.com/mahdi739/arc_field_test/blob/main/src/app.rs

[gbj]

As I understand it, the issue in the example you linked comes when you select and then delete an item, right?

I think this is because you remove the item from the list before the clear the selected item, so there's a moment when the item no longer exists but the "Selected Item: " display still expects it to be there.

          // after this, the session no longer exists
          state.sessions().write().remove(removing_index);
          // and we un-select it
          if is_same_session {
            selected_session.set(None);
          }

I tried simply inverting the order of these, so it cleared the "selected item" before deleting the item, and it no longer panics:

            if is_same_session {
              selected_session.set(None);
            }
            state.sessions().write().remove(removing_index);

Editing to add: this one below does not need to be a .try_get(), because this signal will only be disposed when App is unmounted:

        {move || {
          selected_session
            .get()
            .map(|item| {
              view! { {move || item.field1().get()} }
            })
        }}

[mahdi739]

As I understand it, the issue in the example you linked comes when you select and then delete an item, right?

I think this is because you remove the item from the list before the clear the selected item, so there's a moment when the item no longer exists but the "Selected Item: " display still expects it to be there.

Thanks. My mistake. I made the changes.
But in my real project I actually set the selected_session to the first item, not None (for now let's assume the previous selected session wasn't the first one).

          if is_same_session {
            selected_session.set(state.sessions().into_iter().next().map(Into::into));
          }

And I have a class for each item, which reactively changes depending on whether it's selected.

class:selected=move || {
          selected_session
            .read()
            .is_some_and(|item| { *item.field0().read() == *session.field0().read() })
        }

With this changes the error appears again because it uses the selected_session in the class attribute.
(I also updated the repo)

[gbj]

I checked out the PR branch and your updated example in the PR still panics, though, with the exact same panic message as on main, so I'm not sure I'm understanding how it's relevant?

I have to log off for the next nine hours or so.

I just want to make sure it's clear: I appreciate the work you've done, and I'm not trying to dismiss it, I'm just trying to understand if there are specific cases this PR addresses where 1) the code is written correctly/can't be rewritten to work, and 2) the PR fixes the issue.

[mahdi739]

I checked out the PR branch and your updated example in the PR still panics, though, with the exact same panic message as on main, so I'm not sure I'm understanding how it's relevant?

I have to log off for the next nine hours or so.

I just want to make sure it's clear: I appreciate the work you've done, and I'm not trying to dismiss it, I'm just trying to understand if there are specific cases this PR addresses where 1) the code is written correctly/can't be rewritten to work, and 2) the PR fixes the issue.

Oh, my mistake again! I forgot that I used read() directly in that closure, and the panic isn’t related to this PR at all. With this change, it should work correctly:

        class:selected=move || {
          selected_session
            .read()
            .is_some_and(|item| {
              item.field0().try_read().as_deref().as_ref() // here 
                == session.field0().try_read().as_deref().as_ref() // here
            })
        }

And no errors!
So yeah, I think you were right about the root cause of the problem. I’m fine with any decision you make on this PR, and I can always open a new PR or issue if something else comes up (If you don't mind). Thanks again for taking the time to look at this!