-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
de3506f
Add SupplyParameterFromForm attribute
javiercn 3935340
Detect [SupplyParameterFromFormAttribute] as a cascading value
javiercn 4b7b634
Define form binder interface and basic implementation
javiercn c419f21
Cleanup
javiercn 91429eb
Fix tests
javiercn 454a2fa
Cleanups
javiercn 6e7c52c
Fix XML doc comment
javiercn 3e2e0bf
Cleanups and tests
javiercn dbbee36
Add E2E tests
javiercn d04febe
Undo the IsFixed change
javiercn 91abdda
Move SupplyParameterFromFormAttribute to Microsoft.AspNetCore.Compone…
javiercn 3ff60a4
Register default implementations on other platforms
javiercn 3308969
Update src/Components/Components/src/Binding/IFormValueSupplier.cs
javiercn b1c47e3
Address feedback
javiercn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
29 changes: 29 additions & 0 deletions
29
src/Components/Components/src/Binding/IFormValueSupplier.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace Microsoft.AspNetCore.Components.Binding; | ||
|
||
/// <summary> | ||
/// Binds form data valuesto a model. | ||
/// </summary> | ||
public interface IFormValueSupplier | ||
javiercn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
/// <summary> | ||
/// Determines whether the specified value type can be bound. | ||
/// </summary> | ||
/// <param name="formName">The form name to bind data from.</param> | ||
/// <param name="valueType">The <see cref="Type"/> for the value to bind.</param> | ||
/// <returns><c>true</c> if the value type can be bound; otherwise, <c>false</c>.</returns> | ||
bool CanBind(string formName, Type valueType); | ||
|
||
/// <summary> | ||
/// Tries to bind the form with the specified name to a value of the specified type. | ||
/// </summary> | ||
/// <param name="formName">The form name to bind data from.</param> | ||
/// <param name="valueType">The <see cref="Type"/> for the value to bind.</param> | ||
/// <param name="boundValue">The bound value if succeeded.</param> | ||
/// <returns><c>true</c> if the form was bound successfully; otherwise, <c>false</c>.</returns> | ||
bool TryBind(string formName, Type valueType, [NotNullWhen(true)] out object? boundValue); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 11 additions & 0 deletions
11
src/Components/Components/src/IHostEnvironmentCascadingParameter.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace Microsoft.AspNetCore.Components; | ||
|
||
// Marks a cascading parameter that can be offered via an attribute that is not | ||
// directly defined in the Components assembly. For example [SupplyParameterFromForm]. | ||
internal interface IHostEnvironmentCascadingParameter | ||
{ | ||
public string? Name { get; set; } | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.FormValueSupplier.CanBind
only returnstrue
forstring
. 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.The two main mitigations I could think of are:
[SupplyParameterFromForm] public FormData Form { get; set; }
(possibly alsoFormData<T>
for model binding other types, which also gives a way to trigger binding procedurally and possibly collect validation errors)ICascadingValueComponent
interface to have another overload ofCanSupplyValue
which also accepts the attribute on the parameter (i.e., theCascadingParameterAttribute
or derived attribute type), and then the logic here can be prefixed withif (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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.