Consider an analyzer to warn if a [Parameter] property isn't a simple auto property

[SteveSandersonMS]

If a [Parameter] property has setter logic, the setter could be used to cause side effects that create problems, such as infinite rendering loops. There have been reports of side-effects causing unexpected extra rendering and overwriting parameter changes at https://github.com/dotnet/aspnetcore/issues/24599#issuecomment-697588562

In general, a [Parameter] property is intended as a framework-managed communication channel between a parent and a child component. Developers shouldn't either (1) write to the parameter themselves, either from inside or outside the component, except when implementing their own SetParametersAsync logic, or (2) trigger any side-effects from the setter.

We should consider adding an analyzer that warns if a [Parameter] property isn't just a simple { get; set; } one. Of course, this could be disruptive for existing apps.

Additionally we should strengthen the documentation about parameters to advise developers that not only should they not overwrite the incoming data on a [Parameter] (because their changes can get lost next time the parent renders), but also they should not cause side-effects from the setter.

Examples of problems this would avoid: https://github.com/dotnet/aspnetcore/issues/27997, https://github.com/dotnet/aspnetcore/issues/28518

[ghost]

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[simonziegler]

Some follow-up commentary to https://github.com/dotnet/aspnetcore/issues/24599. We have been building Material.Blazor and really appreciate @SteveSandersonMS's careful explanation in that thread. We have followed this advice and are now using a two-way binding pattern described here and implemented here. This is in fact developed from a port of Steve's RazorComponents.MaterialDesign.

There's one observation we'd like to leave the ASP.NET with. To make our RCL work we have to cope with the fact that when a component is held inside a cascading value, there is effectively a "bounce" on incoming values. We made a couple of different attempts to code around this before Steve's explanation, usually involving a worrying 1ms debounce delay - we discovered the bounce by chance and then understood it subsequent to Steve's explaination. Before we dealt with this, we found that some components went into unending oscillation when values changed.

We'd therefore urge the ASP.NET team to address this if possible in .NET 6. What we encountered in Material.Blazor is likely to be the same for anybody attempting to work with established JS based component platforms, and we suspect that this will include the burgeoning world of web components.

[SteveSandersonMS]

In your repo, it looks like you have several CascadingValue components that don’t have IsFixed=true but should do. This is important for performance, and may well eliminate the difficulty you’ve been seeing.

Once you fix that, are there still problems?

[simonziegler]

Aha! Those cascading values lacking IsFixed="true" need it - thank you for finding ones that got away! However that's not the cause of the issue above.

Our issue is that Material.Blazor is an open source RCL (on nuget here) and so we need to cater for all cases for our package's consumers.

I'm not that clear on the exact circumstances of bounce and need to refer back to your advice any time I need to consider it in detail. However that there is bounce at all, in any circumstance lead us to some fiddly cached value checks in OnParametersSet/OnParametersSetAsync that we otherwise wouldn't need. We believe that this will lead to issues for other RCLs in the future and if there is any way to remove the bounce it seems to be a worthy goal.

[SteveSandersonMS]

Ok, got it.

If you’re able to create a minimal repro for the behaviour you think should change, then rather than posting here, could you please file a new issue with the minimal repro code? Thanks!

[simonziegler]

I'll try to do so. I don't promise to be fast due to pressure of business. Out of interest, do you have a time limit during which such a repo would ideally be done? If so I'll try to hold up my end of the bargain.

Cheers, Simon

[Youssef1313]

@SteveSandersonMS should the analyzer allow non-auto-property in case it behaves the same as an auto-property?

e.g:

private T _prop;

[Parameter]
public T Prop
{
    get => _prop;
    set => _prop = value;
}

[SteveSandersonMS]

@Youssef1313 Thanks for looking into this!

should the analyzer allow non-auto-property in case it behaves the same as an auto-property?

While it would be nice to do that, my guess is that it would be a lot harder to implement reliably. In the vast majority of cases people will be quite happy to use auto-properties, and in the rare cases where they aren't happy with that, they can suppress the warning (either on a per-property level, or even project-wide).

So I suggest sticking with only checking for true auto-properties, unless you have some clever mechanism in mind to detect equivalent non-auto-properties reliably.

[Dreamescaper]

What is the suggested approach if I do need to process provided value? E.g. I have Url parameter, and I create some UrlImageSource object based on this value (and I don't want to re-create it on each render)?

Would it be better in any way to handle that in OnParametersSetAsync method instead of custom setter?

[SteveSandersonMS]

Would it be better in any way to handle that in OnParametersSetAsync method instead of custom setter?

Yes, precisely.