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

OnParametersSetAsync with reused component #18638

Closed
poke opened this issue Jan 28, 2020 · 3 comments
Closed

OnParametersSetAsync with reused component #18638

poke opened this issue Jan 28, 2020 · 3 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved

Comments

@poke
Copy link
Contributor

poke commented Jan 28, 2020

This is somewhat a follow-up from #18382. To understand where I am, let’s assume a component, similar to a tab control, where you can switch the selection and the selected item is passed to a control that should display details:

@foreach (var item in items)
{
    <button @onclick="() => selectedItem = item">@item</button></li>
}
</ul>
<DetailsComponent Item="@selectedItem" />

@code {
    private string[] items = new string[] { "Item 1", "Item 2", "Item 3", "Item 4" };
    private string selectedItem;
}

In this setup, the details component gets heavily reused, so when I want to load additional data, I will have to do that within the OnParametersSetAsync since the initialization would only run once for the very frist render.

If I now add a cascading parameter to this setup, then what I am seeing is that the OnParametersSetAsync of the DetailsComponent gets called twice. Looking at the source, this appears to be the case because direct parameters and cascading parameters are set separately. This also matches my attempts to see whether there are ever more than two calls; I couldn’t reproduce #18382 more than twice.

The problem is now that I am using OnParametersSetAsync to load data for the component to display:

<h3>Details for @Item (@Cascading)</h3>
<p>@Details</p>

@code {
    [CascadingParameter] public int Cascading { get; set; }
    [Parameter] public string Item { get; set; }
    public string Details { get; set; }

    protected override async Task OnParametersSetAsync()
    {
        Details = await LoadData(Item);
    }
}

This also matches what the sample does in the FetchData component:

protected override async Task OnInitializedAsync()
{
forecasts = await ForecastService.GetForecastAsync(DateTime.Now);
}

Since the method is now invoked twice when the selected item changes, this also means that the data is loaded twice. So I have to go out of my way to make sure that I don’t start two separate jobs here to retrieve the data.

While that is somewhat manageable, there is a follow-up problem with this: Assuming that the data loading does not complete fast enough, the user is abled to change the selection before the loading completes. This means that the data is still loading for a value of the component that is no longer relevant. And since this does not interrupt the data loading, this also means that there are parallel tasks that will eventually complete and set the shared Details property.

Even if I clear the Details value before loading (to avoid showing old data for new parameters), this can cause incorrect data to appear when the task completes and the Details gets set:

Delayed loading with reset before loading

If I now add the cascading value to this setup, then the behavior gets really odd. Occasionally, I am seeing the following where the data is not loaded for the correct item and instead only appears after the next change:

Delayed loading with erratic behavior due to cascading values

From looking at my logs, it appears that the call to OnParametersSetAsync from the cascading value gets called first which causes a data loading while there is still the old selected item value. That way, the data for Item 4 gets loaded although Item 1 was selected.


I don’t really know where to go from here. From the way the parameters are updated, it’s not surprising that this behavior can happen. Maybe changing the ComponentState to combine the parameters directly and only set the parameters once on the component. Alternatively, if there was a way to prevent reusing the component instances, the data loading could be done in the OnInitializeAsync which would not suffer from this problem.

Ultimately, I am wondering what the recommended setup for a data loading situation in OnParametersSetAsync is, since this can get really complicated really fast. I’m currently considering switching this to a synchronous loading to avoid this problem altogether, but of course that will heavily restrict what kind of data can even be loaded.

A repro project for this is over here.

@pranavkm pranavkm added the area-blazor Includes: Blazor, Razor Components label Jan 29, 2020
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Jan 29, 2020
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @poke.
@javiercn can you please review this? Thanks!

@javiercn
Copy link
Member

javiercn commented May 7, 2020

The behavior here is expected and is the result of not cancelling the loading work when the parameters change.

Essentially what happens is that you start loading the data, the parameters change and you don't cancel the previous async work (which is still going) and at that point you have two load operations running concurrently.

You will see different behavior based on the order they finish as the callback will try to set the state on the components.

The CascadingParameter component renders itself first (rendering the children) and then notifies subscribers of updates in case they have changed. The problem is, the cascading parameter can't know if the direct parameters changed from the last/previous time they were set and that causes it to require a rerender.

I'm not sure at this point if there is an actual way we could achieve a single re-render by combining the direct and cascading parameters into one call, but at the very least it would involve a lot of extra tracking and I'm not sure if it would be worth performance wise or given the added complexity we would add. There might also be other drawbacks/consequences that we can't foresee, so I don't think we'll likely change how it works today.

For your specific scenario, the recommendation is to use a cancellationtokensource to cancel the previous load and trigger a new one when the parameters are set. In the case where you have a cascading component I would capture the specific parameters used for your query and just re-trigger the query if any of them changed.

@javiercn javiercn added ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question and removed investigate labels May 7, 2020
@ghost ghost added the Status: Resolved label May 7, 2020
@ghost
Copy link

ghost commented May 8, 2020

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed May 8, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jun 7, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved
Projects
None yet
Development

No branches or pull requests

4 participants