Skip to content

Default values for component parameters get overwritten #6864

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
danroth27 opened this issue Jan 19, 2019 · 10 comments
Closed

Default values for component parameters get overwritten #6864

danroth27 opened this issue Jan 19, 2019 · 10 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@danroth27
Copy link
Member

  • Create a new Razor Components app

  • Add a parameter to the Counter component with a default value of 1

    [Parameter] int IncrementAmount { get; set; } = 1;
    void IncrementCount()
    {
        currentCount+=IncrementAmount;
    }
  • Run the app and try the Counter on the Counter page

Expected result:

  • Counter increments

Actual result:

  • Counter doesn't increment - stays at zero
  • The IncrementAmount parameter was overwritten to zero
@danroth27 danroth27 added bug This issue describes a behavior which is not expected - a bug. 1 - Ready labels Jan 19, 2019
@danroth27 danroth27 added this to the 3.0.0-preview2 milestone Jan 19, 2019
@pranavkm pranavkm added discussion and removed 1 - Ready bug This issue describes a behavior which is not expected - a bug. labels Jan 19, 2019
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 19, 2019
@pranavkm
Copy link
Contributor

@danroth27, I tried this out with a blazor application, and I see the same outcome. Moving this out to preview3 so we can figure out what the right behavior needs to be.

@SteveSandersonMS, thoughts on what the behavior should be here? ParameterCollection goes out of it's way to ensure all parameters get initialized with a value. Is there a reason we don't rely rely on the language to initialize the type to the right value?

@danroth27
Copy link
Member Author

danroth27 commented Jan 20, 2019

@pranavkm This works in Blazor 0.7.0 and has worked from pretty much the first release of Blazor. This is one of the first scenarios that Blazor users try out when getting started with Blazor. If this was regressed in the latest update to Blazor it needs to be addressed.

@RemiBou
Copy link
Contributor

RemiBou commented Jan 21, 2019

@pranavkm the code you are seing was made to handle when navigating in the same page : on a page with this route "/index/{id}" when going from "/index/123" to "/index" the parameter "id" should be set to its default value, just like it would when accessing directly to "/index"

Maybe we could apply this only to parameters from url, but there is no way, yet, to make a difference between route parameter and attribute parameter.

@SteveSandersonMS
Copy link
Member

As @RemiBou points out, this is an unfortunate loss of functionality due to the change to make routing work better.

I'm not certain how we can reconcile the two goals of "resetting non-supplied parameters to default" with "obtain default from constructor", given that we can't re-run the constructor. We probably need to accept the loss of one of those two bits of functionality, and I'm not sure which one yet.

For the 0.8.0 release, I think we're stuck with the current behavior (i.e., ignores constructor defaults).

@SteveSandersonMS
Copy link
Member

Actually @danroth27, what's your view on the importance of not regressing this, and the practicality of changing it at this stage in the release? It would be a fix inside Microsoft.AspNetCore.Components (in shared framework), and the fix would be to undo the change that restores defaults for non-supplied route values.

@RemiBou
Copy link
Contributor

RemiBou commented Jan 21, 2019

@SteveSandersonMS maybe splitting Parameter in two concept : the parameter from routing and the parameter from attribute ? I think it'll be easier to understand as it unusual to have both of them behind the same concept (angular uses ActivatedRoute and @input() for instance, react uses props and match). So we could ignore the setting of default parameter for the attribute.

This would mean splitting the property initializaition in two part but it would improve the performance as well because the attribute parameter doesn't need to be updated on route change and route parameter doesn't need to handle cascading. I hope I'm clear enough.

@SteveSandersonMS
Copy link
Member

@RemiBou I'm not sure there's really any way to reset a parameter to its "default" value if they are allowed to be initialized using a constructor, no matter how we define the "parameter" concept. Consider the following:

[Parameter] SomeObject MyParam { get; set; } = ObtainDefaultValue(); // calls some static to initialize

There's no way the framework can know about the existence of ObtainDefaultValue or that you would want to call it to reset MyParam to its default state.

@RemiBou
Copy link
Contributor

RemiBou commented Jan 21, 2019

Yes indeed it difficult (There is some odd solution on SO https://stackoverflow.com/a/48190076/277067 but I don't think it's a good idea).

But the code that reset the parameter to the default value (the code @pranavkm found) would not be called for attribute parameters, only for route parameters. For route we can use the DefaultValueAttribute if someone want to set a different value than default().

@pranavkm
Copy link
Contributor

For route we can use the DefaultValueAttribute if someone want to set a different value than default().

We could use this approach and use the analyzer to flag initialization of parameters.

@danroth27
Copy link
Member Author

@SteveSandersonMS and I discussed this and we agreed to revert to the old behavior for default parameter values for preview2. We'll need to revisit the client routing scenario in preview3.

@danroth27 danroth27 added the bug This issue describes a behavior which is not expected - a bug. label Feb 6, 2019
@danroth27 danroth27 added the area-blazor Includes: Blazor, Razor Components label Feb 13, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

6 participants