fix: ReactiveComponentBase creating unnecessary subscriptions

[RLittlesII]

What kind of change does this PR introduce?

Relates To: #2377

What is the current behavior?

The current implementation of the ReactiveComponentBase is potentially creating unnecessary subscriptions. Currently, we are wiring up this.WhenAnyValue in the OnAfterRender outside the firstRender which causes additional subscriptions every render.

We currently have our logic checking for the first render, doing some subscriptions for state changes, and then doing others outside of that if block. That code is potentially leaking subscriptions. We also do not call that state change correctly in the observable pipeline.

What is the new behavior?

This change moves all subscription logic to inside the firstRender if statement. This will reduce the number of subscriptions generated and should reduce leaks.

What might this PR break?

ReactiveUI.Blazor

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:

[codecov]

Codecov Report

Merging #2391 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2391   +/-   ##
=======================================
  Coverage   54.69%   54.69%           
=======================================
  Files         114      114           
  Lines        4344     4344           
  Branches      663      663           
=======================================
  Hits         2376     2376           
  Misses       1801     1801           
  Partials      167      167           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e469f6...f10cb43. Read the comment docs.

[glennawatson]

We need to have the WhenAnyValue to be in OnAfterRender() due to javascript engines not being able to handle it earlier. Several users had bugs. Move both the WhenAnyValue check inside the if statement. Also we can probably use RefCount() on the shared observable.

Please update the description about OnAfterRender() above.

[glennawatson]

Changes as per comment.

[glennawatson]

We should probably remove the Skip(1), this makes sense in constructors where the value couldn't be any value but null but in this case it can be.

[b-straub]

I applied the changes for testing purpose to my project. Unfortunately the fix will break any rerendering of the component from a PropertyChangedEvent. You can verify the problem by using the ReactiveUI Blazor serverside sample and applying the following changes:

FetchDataViewModel.cs:

private readonly ObservableAsPropertyHelper<bool> _loading;
  public bool Loading => _loading.Value;
….
_loading = LoadForecasts.IsExecuting.ToProperty(this, x => x.Loading, scheduler: RxApp.MainThreadScheduler);

….
private async Task<WeatherForecast[]> LoadWeatherForecastsAsync()
{
   await Task.Delay(TimeSpan.FromSeconds(2));
   return await _weatherForecastService.GetForecastAsync(DateTime.Now);
}

FetchDataView.razor:

@if (ViewModel.Loading)
{
    <p><em>Loading...</em></p>
}
else if (ViewModel.Forecasts.Any())
{

After loading has finished the page will not update. This can be fixed by changing
.RefCount(2);

I'm not such a reactive expert that I can judge if this makes sense.

[RLittlesII]

@b-straub Thanks for the input. I am continuing conversation on #2377 as it relates to this change.

[b-straub]

As far I have tested this PR must be fixed, otherwise view updates neither for SPA nor for WASM will work anymore. Please see my comment here: #2377

[RLittlesII]

We are aware that this is breaking. No packages have been released. This will be resolved this week in a subsequent Pull Request. No need to comment further on this pull request as it is already merged.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.