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

[Bug]: Blazor component base classes do not trigger WhenActivated() #3413

Closed
rcdailey opened this issue Nov 15, 2022 · 5 comments
Closed

[Bug]: Blazor component base classes do not trigger WhenActivated() #3413

rcdailey opened this issue Nov 15, 2022 · 5 comments
Labels

Comments

@rcdailey
Copy link

rcdailey commented Nov 15, 2022

Describe the bug 🐞

When deriving my Page (view) from ReactiveInjectableComponentBase and providing a view model class that implements IActivatableViewModel, my callback provided to this.WhenActivated() is not invoked for me.

Step to reproduce

  1. Derive your page from the mentioned base class like so:

    @inherits ReactiveInjectableComponentBase<MonitorDeploymentsViewModel>
  2. Implement your view model like this:

    public class MonitorDeploymentsViewModel : ReactiveObject, IActivatableViewModel
  3. Provide a call to WhenActivated like so:

    this.WhenActivated(disposal =>
    {
        // Do something meaningful here
    });
  4. Compile and run the application. Be sure to visit the page we're talking about to trigger the issue.

Reproduction repository

This is a non-public repository; I have no MCVE.

Expected behavior

The ReactiveInjectableComponentBase class (as well as the other Blazor-specific component base classes) implement ICanActivate and already trigger the following:

  • When OnInitializedAsync() is invoked by the view, trigger Activated state.
  • When Dispose() happens (IDisposable), trigger Deactivated state.

However, by default, there are no observers for these states. This is because, in my opinion, in the setter for ReactiveInjectableComponentBase.ViewModel, code is missing to check if the supplied VM implements IActivatableViewModel and if so, attaches it to the appropriate Activated/Deactivated observables.

The workaround right now requires users to manually override OnInitializedAsync() simply to invoke ViewModel.Activator.Activate(), as well as the corresponding Dispose() override for the Deactivate(). This means that, ultimately, there's logic in the ComponentBase class that ends up not getting used at all because it's not tied in.

I think the addition necessary to the base ViewModel property is as follows:

            _viewModel = value;
            if (_viewModel is IActivatableViewModel avm)
            {
                Activated.Subscribe(_ => avm.Activator.Activate());
                Deactivated.Subscribe(_ => avm.Activator.Deactivate());
            }

Another solution I tested that seems to work:

protected override void OnInitialized()
{
  Activated.Subscribe(_ => ViewModel!.Activator.Activate());
  Deactivated.Subscribe(_ => ViewModel!.Activator.Deactivate());
  base.OnInitialized();
}

Screenshots 🖼️

No response

IDE

Rider Windows

Operating system

Windows

Version

10

Device

N/A

ReactiveUI Version

18.3.1

Additional information ℹ️

I created this issue at the request of @glennawatson, who discussed this with me in Slack.

@rcdailey rcdailey added the bug label Nov 15, 2022
@glennawatson
Copy link
Contributor

Researching, I'm happy with

protected override void OnInitialized()
{
  Activated.Subscribe(_ => ViewModel?.Activator.Activate());
  Deactivated.Subscribe(_ => ViewModel?.Activator.Deactivate());
  base.OnInitialized();
}

I would use the ? operator on the view model just in case its null at the time.

Other platforms allow you to hook into view's creation/deletion from a external events calls separate to the current view, but due to Blazors methodology this can't happen easily.

We'd likely have to make similar changes to the other Rx blazor overrides.

imundy7 added a commit to imundy7/ReactiveUI that referenced this issue Dec 5, 2022
@BekAllaev
Copy link

Hey, guys. Any updates on this issue? I can see that @imundy7 have made changes in the fork.

@imundy7, any plans to merge this into main branch?

ChrisPulman added a commit that referenced this issue May 14, 2024
glennawatson pushed a commit that referenced this issue May 14, 2024
<!-- Please be sure to read the
[Contribute](https://github.com/reactiveui/reactiveui#contribute)
section of the README -->

**What kind of change does this PR introduce?**
<!-- Bug fix, feature, docs update, ... -->

Fix for #3413

**What is the current behavior?**
<!-- You can also link to an open issue here. -->

IActivatableViewModel does not activate

**What is the new behavior?**
<!-- If this is a feature change -->

adds ability to hook to IActivatableViewModel Activate and Deactivate

**What might this PR break?**

None expected

**Please check if the PR fulfills these requirements**
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)

**Other information**:
@ChrisPulman
Copy link
Member

This should be updated with the next Release, thank you all for your input with this.

@BekAllaev
Copy link

Thanks for the response @ChrisPulman. If you need hands for this issue I can assist. As far as I understand there is no big job to do. Just implement what was said in this comment:

Researching, I'm happy with

protected override void OnInitialized()
{
  Activated.Subscribe(_ => ViewModel?.Activator.Activate());
  Deactivated.Subscribe(_ => ViewModel?.Activator.Deactivate());
  base.OnInitialized();
}

I would use the ? operator on the view model just in case its null at the time.

Other platforms allow you to hook into view's creation/deletion from a external events calls separate to the current view, but due to Blazors methodology this can't happen easily.

We'd likely have to make similar changes to the other Rx blazor overrides.

@ChrisPulman
Copy link
Member

@BekAllaev I used ViewModel is IActivatableViewModel this covers a null condition and ensures its Activatable.
Thank you
I have a few PR'S to complete before I get the next Release out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants