mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.
MIT License
1.23k stars 139 forks source link

Out of the box support for IAsyncDisposable descendant components #325

Closed rainerllera closed 1 year ago

rainerllera commented 2 years ago

This PR:

rainerllera commented 2 years ago

hmm actually, this would break existing code that overrides Dispose(disposing), since it wouldn't be called at all by the renderer's disposing code (it chooses the async version over the non async once). I wonder if it would be ok to just call Dispose(disposing) from DisposeAsync(disposing). In any case, the component's disposal (the fluxor part of it at least) is guaranteed to happen just once by using the Disposed flag.

mrpmorris commented 2 years ago

Here is the official documentation... https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync

It seems to me that adding IAsyncDisposable to a base class would require all descendant classes to also implement the pattern.

Looks to me like the recommended pattern would break code that only overrides Dispose.

The docs also say a properly written Dispose method should be callable multiple times, but then goes on to say you shouldn't call Dispose multiple times because it's also acceptable to throw an ObjectDisposedException in subsequent calls.

https://docs.microsoft.com/en-us/visualstudio/code-quality/ca2202?view=vs-2022

So it looks to me like the recommended approach will certainly break code, and the potential fix of having DisposeAsync call Dispose should work, but should not be used.

Where this leaves us, I don't know :)

mrpmorris commented 2 years ago

I think this means it'll have to be a breaking change and implement IAsyncDisposable instead of IDisposable. I'll have to give it some thought, but it would be a V6 release as it breaks current code.

rainerllera commented 2 years ago

Yep, I agree, not much to do for now :( Cheers!

mrpmorris commented 1 year ago

I've implemented this in v6 by dropping IDisposable in the Blazor libraries and going only with IAsyncDisposable, as I know for certain this will be called.

Thank you!

dmitry-pavlov commented 4 months ago

When are you going to release NuGet for v6 with IAsyncDisposable @mrpmorris ?

mrpmorris commented 4 months ago

I have it ready, but I am still unsure about it.

Anyone who needs it could implement it themselves, and then call Dispose.

dmitry-pavlov commented 4 months ago

No doubt, all those who need something can do everything themselves any time 😄 But when we (developers) consider using libraries - we expect them to be flexible enough and not require much intervention in internal implementation. So... I assume that would still make sense to comply with the modern trend to making everything async.

What about the following approach then? Instead of doing it this way:

public abstract class FluxorComponent : ComponentBase, IDisposable, IAsyncDisposable { ... }

Just add one more version of Fluxor component like this:

public abstract class FluxorComponentAsync : FluxorComponent, IAsyncDisposable { ... }

This way Fluxor library could fulfill the needs by providing alternative components.

mrpmorris commented 4 months ago

If you wanted to async dispose something in your code and had to implement IAsyncDisposable, what would the code look like?

dmitry-pavlov commented 4 months ago

Keeping in mind the note below:

Components shouldn't need to implement IDisposable and [IAsyncDisposable] simultaneously. If both are implemented, the framework only executes the asynchronous overload.

and if we inherit this way FluxorComponentAsync : FluxorComponent, IAsyncDisposable { ... } I guess SomeCutomComponent inherited from FluxorComponentAsync should have something like:

protected override async ValueTask OnDisposeAsync() {
    if (someAsyncDisposableResource is not null)
    {
        await someAsyncDisposableResource.DisposeAsync();
    }
    await base.OnDisposeAsync();
}

FluxorComponentAsync base class should take care of calling FluxorComponent.Dispose (according the note above - the framework only executes the asynchronous overload if both are implemented). So if we had sync Dispose also overriden in SomeCutomComponent - this will be handled as well this way.