Skip to content
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

[Blazor] Support for primitive types in [SupplyParameterFromForm] #48432

Merged
merged 9 commits into from
Jun 7, 2023

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented May 25, 2023

  • Adds support for binding IParsable values and Nullable values.

FormDataMapper is the entry point for mapping form values into a type, it receives a FormDataReader and FormDataMapperOptions. The options contain an internal collection of converters and factories that are used to define how to bind a given type.

For well-known types we prepopulate the list of converters.

  • FormDataConverter <-> ModelBinder.
  • FormDataConverterFactory <-> ModelBinderProvider

#46688 (comment)

Some basic benchmark data against MVC. Beware that this is with any of the main optimizations.

Method Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
ModelBinding_PrimitiveTypes_Mvc 43.08 μs 1.120 μs 3.141 μs 23,212.5 1.00 0.00 - - - 3 KB
ModelBinding_PrimitiveType_Components 25.11 μs 0.834 μs 2.367 μs 39,821.0 0.59 0.07 - - - 2 KB

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label May 25, 2023
@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from b9eddae to f025ae0 Compare May 26, 2023 10:24
@javiercn javiercn force-pushed the javiercn/supply-from-form-primitive-values branch from f9504c8 to 0ac09b7 Compare May 27, 2023 10:05
@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from cfbeda2 to 66c3f89 Compare May 31, 2023 10:11
@javiercn javiercn force-pushed the javiercn/supply-from-form-primitive-values branch from 470b2be to 23b534b Compare May 31, 2023 10:39
@javiercn javiercn force-pushed the javiercn/supply-from-form-parameter branch from 50b3aa3 to 3ff60a4 Compare May 31, 2023 14:23
Base automatically changed from javiercn/supply-from-form-parameter to main May 31, 2023 16:56
@javiercn javiercn force-pushed the javiercn/supply-from-form-primitive-values branch 2 times, most recently from 0745e8c to 11c7bd8 Compare May 31, 2023 17:08
@javiercn javiercn marked this pull request as ready for review May 31, 2023 18:06
@javiercn javiercn requested review from a team as code owners May 31, 2023 18:06
@javiercn javiercn requested review from MackinnonBuck and removed request for a team May 31, 2023 18:07
@javiercn javiercn force-pushed the javiercn/supply-from-form-primitive-values branch from 20a9175 to ee67e2a Compare June 1, 2023 11:47
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.

Just a couple questions but this looks great to me.

return result;
}

// We don't do error handling yet.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue tracking the remaining error handling work? I couldn't find one.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's covered as part of #46688


internal abstract class FormDataConverter<T> : FormDataConverter
{
internal abstract bool TryRead(ref FormDataReader context, Type type, FormDataMapperOptions options, out T? result, out bool found);
Copy link
Member

Choose a reason for hiding this comment

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

Is type ever going to be different from typeof(T)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not obvious yet in this PR, but it's passed in for a different reason. When you do typeof(T) the generated code needs to ask the type system at runtime (this is in IL) for the type instance, which involves a lookup. For that reason, many serializers cache and pass the type instance along.

If you look at System.Text.Json, they do something similar. See here

The other part is that we might still need this if in the future we want to support polymorphic binding or enable people to do so, so we can't just assume typeof(T) == type in general.

@javiercn javiercn merged commit 3863fdb into main Jun 7, 2023
@javiercn javiercn deleted the javiercn/supply-from-form-primitive-values branch June 7, 2023 07:38
@ghost ghost added this to the 8.0-preview6 milestone Jun 7, 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.

2 participants