Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

ViewModel's Dispose is not being invoked unless page is refreshed #57

Closed
deveshbahuguna opened this issue Nov 21, 2021 · 6 comments · Fixed by #66
Closed

ViewModel's Dispose is not being invoked unless page is refreshed #57

deveshbahuguna opened this issue Nov 21, 2021 · 6 comments · Fixed by #66

Comments

@deveshbahuguna
Copy link

deveshbahuguna commented Nov 21, 2021

@klemmchr You are right as per the documentation one should not call Dispose but if you look at the example provided by microsoft, the dispose method is called on each new request/refresh of the page but why would one refresh the page to call Dispose? and during my testing as well the Dispose method is called on Page Refresh.
"The debug console shows the following output after each refresh of the Index page:" ~Microsoft

In think there could be two way to solve this problem

  1. via Dispose
  2. Invoking an event say page closed in view model on dispose.

What do you think?

Originally posted by @deveshbahuguna in #55 (comment)

@klemmchr
Copy link
Owner

You are right as per the documentation one should not call Dispose but if you look at the example provided by microsoft, the dispose method is called on each new request/refresh of the page but why would one refresh the page to call Dispose?

The disposal of a resource is determined by the lifetime of a resource. Usually you would use OwningComponentBase to circumvent this issue. However, having this as a base class has some implications that are not feasible when trying to use this library. This is indeed an issue but this cannot be resolved by just implementing IDisposable. The DI needs to have control over this in order to keep track of the instances. Manually implementing the dispose pattern could result in disposing a singleton which leads to unpredictable behavior.

If you have a good solution for this problem, I'll gladly take it.

@deveshbahuguna
Copy link
Author

deveshbahuguna commented Nov 27, 2021

Well Implementing IDisposable in viewmodel(Service) with transient dependency would further cause memory leak as GC stores the reference for those objects with IDisposable type.

thus I think the possible solution could be

  1. Have a PageClosed event in ViewModelBase
  2. Call the PageClosed event when the BIndings Are Disposed.

Thus in this way one can do the clean up activity e.g Deregister events etc at the time of page close.

If you think this could solve the problem, I would happy to raise PR

@klemmchr
Copy link
Owner

That is the reason why OwningComponentBase would be the right choice. Implementing this behavior on top of the Blazor mechanics is an anti-pattern IMHO. Resolving the memory leak for transient services is out of scope of this library. The issue of transient services implementing IDisposable should be handled by some analyzer provided by Microsoft. I would rather provide an extension method that registers the view model in a proper way (meaning always scoped) to prevent this problem from happening.

@deveshbahuguna
Copy link
Author

How about having a Page Closed event in MvvmComponentBase which can be invoked when the page is Disposed thus one can do any cleanup activity on page close?
In this way we could avoid IDisposable in ViewModel

@deveshbahuguna
Copy link
Author

deveshbahuguna commented Dec 6, 2021

@klemmchr like you suggested we could have one more class "MvvmOwningComponentBase" which inherit from OwningComponentBase just like the way you had created MvvmComponentBase for blazor framework's ComponentBase, we could have this new class as well.

@klemmchr
Copy link
Owner

klemmchr commented Feb 6, 2022

Yes, this is the only proper solution to this problem. I will address this together with #52 where the ground work for this is already set. This will also come with a breaking change so I will take this opportunity to upgrade it to .NET 6 while at it.

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

Successfully merging a pull request may close this issue.

2 participants