Skip to content

Explicitly implement SetParametersAsync #12254

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

Closed
wants to merge 1 commit into from
Closed

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented Jul 16, 2019

This change converts SetParametersAsync to be an explicit interface
implmenentation, and adds lifecyle methods for running code before or
after the parameters are set.

This is a design prototype, and not ready to be merged. I'll add tests
if we like the general direction of this.

This change converts SetParametersAsync to be an explicit interface
implmenentation, and adds lifecyle methods for running code before or
after the parameters are set.

This is a design prototype, and not ready to be merged. I'll add tests
if we like the general direction of this.
@rynowak rynowak requested a review from SteveSandersonMS July 16, 2019 20:57
@@ -175,22 +190,22 @@ void IComponent.Configure(RenderHandle renderHandle)
/// Method invoked to apply initial or updated parameters to the component.
/// </summary>
/// <param name="parameters">The parameters to apply.</param>
public virtual Task SetParametersAsync(ParameterCollection parameters)
Task IComponent.SetParametersAsync(ParameterCollection parameters)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our idea of making this explicit with overrides is that you can't use the base keyword within in an explicit interface implementation. So it's not really possible to interact with by overriding or it would require a lot more design.

It seemed more natural to give you a before/after lifecycle model. The only use case I can really think of for before is to set default values, but it's otherwise SUPER painful to do.

{
parameters.SetParameterProperties(this);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was another issue I discovered here (or maybe it's intentional and I just missed it).

The way this is currently implemented, Init runs after parameters are set. This means that you can read parameter values in Init and it works. That seems really surprising and I wonder how often it's misused. I assumed that it's valuable to separate these things so that we can keep the exact timing of init being called an implementation detail.

@rynowak rynowak added the area-blazor Includes: Blazor, Razor Components label Jul 16, 2019
@rynowak
Copy link
Member Author

rynowak commented Jul 16, 2019

@SteveSandersonMS

@SteveSandersonMS
Copy link
Member

Can we do some further design discussion on this? I'm concerned that the drawbacks of going this way might outweigh the benefits, and starting to wonder if the existing arrangement of SetParameters being protected virtual might be preferable. Apologies for going back and forth on this.

At the very least, I think we would need to eliminate OnParametersSettingAsync and only have a sync version of that. Because otherwise, we'd have to snapshot the parameter collection for every single component on every render (or at least, if the developer returns an incomplete task) to avoid race conditions. Is there really a use case for an async version of this anyway? What would you do from it? Also I'd hope we don't need to call StateHasChanged from here - wouldn't that cause extra renders that don't serve any purpose?

More broadly, it seems like the primary problem solved by OnParametersSetting is setting default values for parameters. In turn, the primary scenario for setting default values for parameters is to work around this routing issue. Let's forget that routing issue exists for the purposes of this design, because we have to fix that separately anyway, for which I have a separate suggestion. The other common reason people set parameter defaults is in cases like [Parameter] public int IncrementAmount { get; set; } = 1 (i.e., for if the consumer didn't pass any value), which also wouldn't work with this model anyway. So how important is setting default parameter values in non-routing, non-inline-parameter-initializer cases? It seems like a pretty advanced use case for which overriding SetParameters and calling base is a very reasonable fit.

There are also reasons why we'd want to expose the actual ParameterCollection to component authors, which this PR largely prevents:

  • What if a component wants to throw a descriptive error if a particular combination of parameters are used together, or of a particular parameter is omitted (but null is allowed)? It's pretty hard to know what the upstream component is supplying if you can only observe the resulting data in your [Parameter] properties, some of which might already have been set by an earlier set of parameters.
  • What if a component wants to snapshot its previous set of parameters so that, on the next ShouldRender, it can compare the old against the new to decide whether to render? It's relatively easy today because you can do parameters.ToDictionary() to get a snapshot, but with this change developers would have to do left-right code to copy all the values from each of their [Parameter] properties.

Altogether I feel more comfortable with the general-purposeness of the override SetParameters approach, despite the drawbacks:

  • It does mean overriders have to remember to call base, but it's a somewhat advanced case and anyone who forgets to call base will immediately see it doesn't work. It's not a subtle problem.
  • It does mean that a bad implementation might try to hold on to the ParameterCollection instance without cloning it, but that's something we could address in the future non-breakingly by using the techniques we discussed yesterday (either ref struct or having ParameterCollection throw if you try to read from it after the backing collection has changed).

@rynowak
Copy link
Member Author

rynowak commented Jul 17, 2019

Can we do some further design discussion on this? I'm concerned that the drawbacks of going this way might outweigh the benefits, and starting to wonder if the existing arrangement of SetParameters being protected virtual might be preferable. Apologies for going back and forth on this.

Yes, totally fine. There's a reason why I said this was a design prototype.

@rynowak rynowak closed this Jul 18, 2019
@rynowak
Copy link
Member Author

rynowak commented Jul 18, 2019

Discussed and rejected!

@dougbu dougbu deleted the rynowak/set-parameters branch May 18, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants