Skip to content

[Blazor] Initial support for [SupplyParameterFromForm] #48412

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

Merged
merged 14 commits into from
May 31, 2023

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented May 24, 2023

Adds support for binding strings via [SupplyParameterFromForm] any other type that requires conversion has been explicitly left out.

Contributes to #46688

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label May 24, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label May 24, 2023
@javiercn javiercn marked this pull request as ready for review May 24, 2023 19:49
@javiercn javiercn requested a review from a team as a code owner May 24, 2023 19:49
@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from 96f40a7 to 520ae3b Compare May 24, 2023 21:59
Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few questions.

@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from b9eddae to f025ae0 Compare May 26, 2023 10:24
@@ -136,4 +133,54 @@ void IDisposable.Dispose()
{
Navigation.LocationChanged -= HandleLocationChanged;
}

bool ICascadingValueComponent.CanSupplyValue(Type valueType, string? valueName)
Copy link
Member

@SteveSandersonMS SteveSandersonMS May 26, 2023

Choose a reason for hiding this comment

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

I'm trying to make sense of whether this can interfere with unrelated [CascadingParameter] scenarios. It looks like it might do.

  • Right now I know you've limited this so that FormValueSupplier.CanBind only returns true for string. So it looks like it would interfere with any case where someone has a parameter like [CascadingValue] public string Something { get; set; }. That's a pretty niche scenario, but still would be very weird for a form value to show up.
  • If we want to extend in the future also to binding arbitrary types to model-bind a form post, then maybe it would start interfering with everything as it would automatically want to supply values for all types. I'm not 100% sure about this but it seems possible.

The two main mitigations I could think of are:

  1. We could require these parameters to have a special type, e.g., [SupplyParameterFromForm] public FormData Form { get; set; } (possibly also FormData<T> for model binding other types, which also gives a way to trigger binding procedurally and possibly collect validation errors)
  2. Or, extend the ICascadingValueComponent interface to have another overload of CanSupplyValue which also accepts the attribute on the parameter (i.e., the CascadingParameterAttribute or derived attribute type), and then the logic here can be prefixed with if (thatAttribute is not SupplyParameterFromFormAttribute) return false and we then know for sure it's not interfering with anything else, and we're not incurring any perf cost of instantiating strings for every unrelated named [CascadingParameter] like on line 141.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SteveSandersonMS Mackinnon is working on that part. We'll very likely do 2. Which allows us to filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am scoping that part out of my PR.

Copy link
Member

Choose a reason for hiding this comment

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

I find it difficult to review this PR in isolation because it's adding things we know we don't want, so would have to combine it with a follow-up PR to make sense of whether the net result actually adds the right things. If you feel that's definitely the best way to proceed then OK, but my preference would be to split things up into small items which are each individually correct, for example first making the tweak to generalize cascading parameters, then as a follow-up building [SupplyParameterFromForm] on top of that.

@SteveSandersonMS
Copy link
Member

Is there a design description somewhere for how we're going to handle:

  • Binding to custom model types
  • Integrating with forms and validation

Do you have an example of a canonical SSR form that will show server-side validation messages on post?

@javiercn
Copy link
Member Author

@SteveSandersonMS sample form https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Samples/BlazorUnitedApp/Pages/Index.razor

I am trying to work out the prototype in small chunks, so there will be missing parts and pieces that will change as I dive deeper into specific implementations.

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS sample form https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Samples/BlazorUnitedApp/Pages/Index.razor

That looks good, thanks. How do the validation messages get into the edit context for the form? There doesn't appear to be any code in the app saying this specific form is meant to be associated with data from the form post.

@javiercn
Copy link
Member Author

That looks good, thanks. How do the validation messages get into the edit context for the form? There doesn't appear to be any code in the app saying this specific form is meant to be associated with data from the form post.

The values are bound within a binding context, the form is rendered within that binding context, so the errors are populated from it.

https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Components/src/Binding/BindingContext.cs

https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Forms/src/BindingContextValidator.cs

The BindingContextValidator and Antiforgery are automatically plugged in by EditForm

https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Web/src/Forms/EditForm.cs#L147-L150

Antiforgery flows transparently from the server to webassembly via PersistedComponentState https://github.com/dotnet/aspnetcore/blob/javiercn/blazor-named-forms/src/Components/Web/src/Forms/Antiforgery/AntiforgeryStateProvider.cs so your form renders the same in server and webassembly.

@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch 2 times, most recently from cfbeda2 to 66c3f89 Compare May 31, 2023 10:11
javiercn added 10 commits May 31, 2023 16:06

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from 50b3aa3 to 3ff60a4 Compare May 31, 2023 14:23
javiercn and others added 2 commits May 31, 2023 17:12
@javiercn javiercn merged commit 61286fd into main May 31, 2023
@javiercn javiercn deleted the javiercn/supply-from-form-parameter branch May 31, 2023 16:56
@ghost ghost added this to the 8.0-preview6 milestone May 31, 2023
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.

None yet

3 participants