dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

Add Name Attribute to EditForm EditContext cascading parameter. Doing it capable to implement components using current DataAnnotationsValidator when multiple CascadingValues are required. #45598

Closed smaicas closed 1 year ago

smaicas commented 1 year ago

Summary

Add Name Attribute to EditForm EditContext parameter. Doing it capable to implement components using current DataAnnotationsValidator when multiple CascadingValues are required.

Motivation and goals

In the scenary when we would like to make a component in the way that EditForm is being. We find that we are not capable to add multiple cascading parameters if we pretending to use current Validation (DataAnnotations) components.

Unless "Name" attribute is added to EditForm when the building tree is done. And also referenced by name in components related to EditForm: ValidationSummary, ValidationMessage and DataAnnotationsValidator.

By the fact that only named-referenced parameters are able to exist when we use multiple cascading parameters.

Simply adding Name attribute it would let us to implement custom components in the way that EditForm is being and use existing DataAnnotationValidator, ValidationSummary and ValidationMessage.

Risks / unknowns

Hardcoding the Name property when we use: [CascadingParameter(Name = "CurrentEditContext")] EditContext? CurrentEditContext { get; set; }

Examples

If we would like to have our own "CustomEditForm" but using capabilities of implemented Validation. We could do it like this. Always that inner components have referred by Name to "EditContext" cascadig parameter:

In my example I implemented a custom validator to validate viewmodel inner process in a MVVM pattern implementation. (User register)

protected override void BuildRenderTree(RenderTreeBuilder builder)
    {
        builder.OpenRegion(_editContext.GetHashCode());

        builder.OpenElement(0, "form");
        builder.AddMultipleAttributes(1, AdditionalAttributes);
        builder.AddAttribute(2, "onsubmit", _handleSubmitDelegate);
        builder.OpenComponent<CascadingValue<IDnjViewModel>>(3);
        builder.AddAttribute(4, "IsFixed", false);
        builder.AddAttribute(5, "Value", ViewModel);
        builder.AddAttribute(6, "Name", "ViewModel");
        builder.AddAttribute(7, "ChildContent", new RenderFragment((builder2) =>
        {
            builder2.OpenComponent<CascadingValue<EditContext>>(8);
            builder2.AddAttribute(9, "IsFixed", true);
            builder2.AddAttribute(10, "Value", _editContext);
            builder2.AddAttribute(11, "Name", "CurrentEditContext");
            builder2.AddAttribute(12, "ChildContent", ChildContent?.Invoke(_editContext));
            builder2.CloseComponent();
        }));

        builder.CloseComponent();
        builder.CloseElement();
        builder.CloseRegion();

    }

It give us more posibilities to implement custom validations that depends on CascadingParameters. For example in the way it is done in this part of code:

<CustomViewModelEditForm EditContext="@_editContext" ViewModel="@ViewModel" OnValidSubmit="async () => await ViewModel.OnValidSubmitAsync(_editContext ?? throw new InvalidOperationException())">

                    <DataAnnotationsValidator />
                    <CustomViewModelValidator />
                    <ValidationSummary />
                _** Notice the custom implementations_

Detailed design

I have done changes and PR where I add Name attribute in the building tree of EditForm and referenced it cascading parameter by name if affected components: DataAnnotationsValidator, ValidationSummary and ValidationMessage.

Thanks in advance and please give me feedback if you find it usefull or you find another way to resolve in this scenario that does not require to add explicit to pass through the CustomViewModelValidator. Also make me know if you feel more information of the scenario I purpose is required or larger code snippets.

smaicas commented 1 year ago

PR: https://github.com/dotnet/aspnetcore/pull/45599

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

smaicas commented 1 year ago

@javiercn @MackinnonBuck @mkArtakMSFT I saw you added it to the .NET 8 planning milestone.

Let me know if I can do work for the issue acomplishment. After all I think a change in the design that affects in the way I have purposed in the PR https://github.com/dotnet/aspnetcore/pull/45599

Or changes in the way that CascadingParameters are evaluated (I have not analysed this part).

Could bring improvements to this EditForm behavior.

If you think it convenient I can help after your decision making. All the best.

javiercn commented 1 year ago

@smaicas thanks for contacting us.

Adding it to the planning milestone is part of our process. It is not clear what you are trying to achieve, but we do not believe this change is the right way to accomplish it.

Giving a name to the cascading value will break all existing form components which is not something we want to support.

If you can provide an example scenario of what you are trying to achieve (with code) we might offer an alternative way of doing so.

ghost commented 1 year ago

Hi @smaicas. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

ghost commented 1 year ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

smaicas commented 1 year ago

@smaicas thanks for contacting us.

Adding it to the planning milestone is part of our process. It is not clear what you are trying to achieve, but we do not believe this change is the right way to accomplish it.

Giving a name to the cascading value will break all existing form components which is not something we want to support.

If you can provide an example scenario of what you are trying to achieve (with code) we might offer an alternative way of doing so.

The scenario is trying to implement a custom validation that derived in a custom EditForm or a compononent, that uses current DataAnnotationsValidator with the existing ValidationSummary and ValidationMessage but also adds other cascading parameters.

The fact is that when we using cascading parameters, we cannot have 2 of them if they are not referenced by name. Also if they have different type we cannot use two parameters if we use implicit type reference.

When I did PR I also thought it is not the best way to resolve. Because the constant. And because typed references are much more clean in my opinion. But still wanted to point it because if there exists a way to do that we can reference multiple implicit type referenced parameters one above other could be great for scenario I show here in the code I implemented:

In a project where I am using MVVM pattern I wanted to add a custom validation that uses existing Blazor ValidationSummary component to show messages in

I needed the ViewModel as parameter for my validation and my intention was to keep the part of the Form all clean as posible

With the code of the PR we can manage as it follows

<DnjViewModelEditForm ViewModel="@ViewModel" EditContext="@_editContext" OnValidSubmit="async () => await ViewModel.OnValidSubmitAsync(_editContext ?? throw new InvalidOperationException())">
                    <h2>@Localizer["Use a local account to log in."]</h2>
                    <DnjDataAnnotationsValidator/>
                    <DnjViewModelValidator/>
                    <DnjValidationSummary Model="ViewModel.LoginInput"/>
                    <div class="mb-3">
                        <label for="login-input-email" class="w-100">
                            @Localizer["Email"]
                            <InputText id="login-input-email" @bind-Value="ViewModel.LoginInput.Email" class="form-control"></InputText>
                            <DnjValidationMessage For="@(() => ViewModel.LoginInput.Email)"></DnjValidationMessage>
                        </label>

                        [...]

If you see I am adding "ViewModel" parameter to the EditForm (DnjViewModelEditForm) and like this I have not to add it parameter as another CascadingParameter being passed to the "DnjViewModelValidator" directly because the EditForm creates it yet. But in the scenario DnjDataAnnotationsValidator and DnjViewModelValidator will perform validation and both will "send" messages to the DnjValidationSummary. And the only point to do it like this is having multiple cascading parameters in a row.

Code above can be resolved in this way too (adding explicit cascading parameter) but I was trying to integrate the most with the framework:

  <EditForm EditContext="@_editContext" OnValidSubmit="async () => await ViewModel.OnValidSubmitAsync(_editContext ?? throw new InvalidOperationException())">
                    <h2>@Localizer["Use a local account to log in."]</h2>
                    <DataAnnotationsValidator/>
                    <CascadingParameter Name="ViewModel" Value="@ViewModel"
                        <DnjViewModelValidator/>
                    </CascadingParameter>
                    <ValidationSummary Model="ViewModel.LoginInput"/>
                    <div class="mb-3">
                        <label for="login-input-email" class="w-100">
                            @Localizer["Email"]
                            <InputText id="login-input-email" @bind-Value="ViewModel.LoginInput.Email" class="form-control"></InputText>
                            <ValidationMessage For="@(() => ViewModel.LoginInput.Email)"></DnjValidationMessage>
                        </label>

I've should investigate why implicit types references for cascading parameters can not be used if there are multiple in cascade and maybe we've could bring a better solution for this. Because you are right and the solution I provided is not a good solution at all.

Just I hope you have understand what I meant :)

javiercn commented 1 year ago

The scenario is trying to implement a custom validation that derived in a custom EditForm or a compononent, that uses current DataAnnotationsValidator with the existing ValidationSummary and ValidationMessage but also adds other cascading parameters.

The part that is not clear is why do you need other cascading parameters for.

It is a design constraint that you can only have one cascading parameter per type or name that applies in a given scope. This is not something that you can change at runtime since the cascading parameters are resolved at compile time (the name is a constant on the attribute).

So I do not believe the approach you are taking is a good fit.

Would creating your own EditForm component and rendering additional cascading values within it solve your problem? Note that you can create your own types for the cascading values and have your custom components consume those types, which should give you the discriminator you are looking for.

ghost commented 1 year ago

Hi @smaicas. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

smaicas commented 1 year ago

Maybe I mistake but what I known is we can only have 1 type parameter also if we using different types. Because the child component only gets cascading parameter (if we use implicit type) from the immediate parent. Example:

<CascadingParameter Value="@ValueOfTypeA">
    <CascadingParameter Value="@ValueOfTypeB">
        <MyComponent></MyComponent>
    </CascadingParameter>
</CascadingParameter>

What I think or experimented (I could be wrong) is MyComponent is not able to get both 2 parameters and he can only get ValueOfTypeB in this case. He could get both 2 if we were using named parameters instead of implicit types.

In my scenario I wanted another CascadingParameter used in my custom validator. Then I tried to create it in the Tree build of my custom EditForm. Having then 2 CascadingParameters: 1 for EditContext class (default) and one for my own behavior. In my case a TViewModel that represents a view model (I am using MVVM pattern)

It has multiple solutions that does not need to modify framework. Also the solution I gave does not fit with good practices I agree.

One solution is that simple as using explicit CascadingParameter that pass to my validator by name. Then using EditForm normally. I was just trying to go out of the box :)

<EditForm ....>

<DataAnnotationsValidator/>
<ValidationSummary/>
<CascadingParameter Name="MyNamedParameter" Value="MyNamedParameterValue">
    <MyCustomValidator>
</CascadingParameter>     

</EditForm>

Another solution could be to wrap both 2 needed parameters in a new one class/type.

I've got a busy month. I left my job :) but I ve got in mind to go deeper in this behavior as soon as I get some time. And try to see why we can have only 1 cascading parameter when we use implicit types. Then maybe an improvement can be done. Assuming that what I say is so. I'll have to check again

Thank you for your concern and for your time. If you consider to close this you can do it for my part, I guess. Since at least for now I think it does not lead anywhere and only brings you more work.

javiercn commented 1 year ago

Thanks for the additional details.

What I think or experimented (I could be wrong) is MyComponent is not able to get both 2 parameters and he can only get ValueOfTypeB in this case. He could get both 2 if we were using named parameters instead of implicit types.

I am not sure of the exact details regarding your component, but I can tell you that you can have multiple non named cascading parameters as long as they have different types, and your component can get both values (one on each property). You only need to discriminate with a name, when there can be multiple values in the hierarchy.