Make PropertySetter a concrete type #25054

pranavkm · 2020-08-19T21:09:17Z

Use the pattern from PropertyHelpers to set property values
Tweaks to Blazor WebAssembly tests to allow running tests locally

captainsafia · 2020-08-21T00:46:20Z

src/Components/benchmarkapps/Wasm.Performance/Driver/Selenium.cs

-#else
-            false;
-#endif
+        static bool PoolForBrowserLogs = true;


Is this intentional?

Yup. We had it turned off because running the benchmarks would bury you logs from the GC. But that's all gone now and it actually helps to see the benchmark progress when you're running it in a container.

captainsafia · 2020-08-21T00:55:25Z

src/Components/Web/src/Forms/ValidationMessage.cs

        /// Specifies the field for which validation messages should be displayed.
        /// </summary>
-        [Parameter] public Expression<Func<TValue>>? For { get; set; }
+        [Parameter(Required = true)] public Expression<Func<TValue>> For { get; set; } = default!;


Should we set CurrentEditContext to be a required cascading parameter as well?

With cascading parameters, we're not going to be able any tooling hints. Right now, the component produces a much more contextual error message than the generic one you get if we apply the attribute.

javiercn · 2020-08-21T11:16:23Z

src/Components/Components/src/Routing/Router.cs

        /// Gets or sets the content to display when a match is found for the requested route.
        /// </summary>
-        [Parameter] public RenderFragment<RouteData> Found { get; set; }
+        [Parameter(Required = true)] public RenderFragment<RouteData> Found { get; set; }


These two seem a bit aggressive, don't we currently handle them when they are null?

Currently, we throw exceptions when they are not provided. I like using Required parameters to indicate that they are needed.

@pranavkm Javier's comment made me realize that we might be able to remove the exceptions we throw in the Router component when these values are not provided. Thoughts?

Yup, I could remove the checks from this type. They're about as generic as they come.

javiercn · 2020-08-21T11:20:02Z

src/Components/Components/test/ParameterViewTest.Assignment.cs

        }

+        [Fact]
+        public void RequiredParameter_NoneSpecified()


nit: Naming SetParameterProperties_ThrowsWhen_ARequiredParameterIsNotSpecified

With all the cases that are here it makes it hard to see at a glance you are covering everything.

javiercn

Looks great!

Some minor comments, but that's it.

ghost · 2020-08-21T19:25:34Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

SteveSandersonMS · 2020-08-21T20:53:20Z

src/Components/Components/src/Reflection/ComponentProperties.cs


            public string? CaptureUnmatchedValuesPropertyName { get; }

+            public IReadOnlyDictionary<string, IPropertySetter> Writers => _underlyingWriters;


This is a bit nitpicky, but I'd have a preference for not exposing the dictionary completely like this, because it makes it really easy for someone to make a mistake in the future and do lookups against Writers instead of going through TryGetValue which has the perf optimization.

Since this is only used during the ThrowRequiredParametersNotSet method, what would you think about changing this to something like:

Suggested change

public IReadOnlyDictionary<string, IPropertySetter> Writers => _underlyingWriters;

public IEnumerable<string> GetKnownPropertyNamesInOrder()

=> _underlyingWriters.Keys.OrderBy(kvp => kvp.Key, StringComparer.Ordinal);

... so as to make it really scenario-specific?

SteveSandersonMS · 2020-08-21T21:17:31Z

src/Components/Components/src/Reflection/ComponentProperties.cs

                _cachedWritersByType[targetType] = writers;
            }

+            var requiredParametersWritten = 0;


I definitely like counting as a way of making this cheap, and it's great to see the benchmarks showing this works out well!

However, the current implementation leaves open an issue whereby the "required" checks could be bypassed by the parent component, either accidentally or deliberately. If we're going to document that "required" checks are truly enforced, then we should look at closing this loophole. The issue is that the parent component isn't required to provide only a single value for each parameter. A ParameterView isn't a dictionary; it can contain repeated keys. So if parameters A and B were both required, the parent could provide two values for A and none for B, and bypass the check.

The approach I had in mind for locking this down is a bit elaborate but TBH I can't think of anything simpler that remains O(1) and don't do any string hashing. Maybe you can! Here's one possibility:

Imagine that IPropertySetter also had an int RequiredParameterIndex field, and when WritersForType built its dictionary, it gave an incrementing value to each required parameter (starting from 0 for each WritersForType instance). The ordering is completely arbitrary. WritersForType also keeps track of the total number of required parameters like you already do here.

Then, inside SetProperties, we stackalloc a uint[] of length ceil(numRequiredParameters/32). And each time we write a required parameters, we set to 1 the bit corresponding to its RequiredParameterIndex. For example, if the index was 58, then we'd want to set the 58th bit, which is the 58-32 = 26th bit in the 2nd uint in the array. Ignoring my possible off-by-one errors or similar, this would be something like doing a logical OR on thatArray[writer.RequiredParameterIndex / 32] with value 1 << (writer.RequiredParameterIndex & 31).

At the end of the SetProperties process, we would have to for(var i = 0; i < writer.RequiredParameterIndex / 32; i++) and check that each entry in thatArray has the expected value, i.e., it equals uint.MaxValue for all entries except the last, which equals whatever expected sequence of bits is left over. Strictly speaking that's not O(1) but it basically is because virtually no components have > 32 required parameters and even then the extra cost is negligible.

If we wanted, this could be simplified by having the rule that you're just not allowed to have > 32 required parameters, and then the IPropertySetter could hold the 1 << index value precomputed and various bits of the above algorithm could be simplified to work on a single uint instead of an array. Instead of storing "number of required parameters" we could store the precomputed "expected sum of the bits" and then just do a direct equality comparison at the end of SetProperties.

Of course, if you have a different idea that's great too! I just haven't thought of a way of checking all the incoming parameter names are distinct without using some kind of hashset/dictionary-like structure, which we'd want to avoid due to the cost of hashing.

Sidenote: I have no idea why IPropertySetter is actually defined as an interface. Since it's purely internal, it seems like it could be turned into a concrete type. Not a big deal and obviously orthogonal to this PR, so please don't feel pressured to do anything with it here. Just a thought in case you actually wanted to make a change in that area.

A ParameterView isn't a dictionary; it can contain repeated keys. So if parameters A and B were both required, the parent could provide two values for A and none for B, and bypass the check.

Yup. I'd based it on the observation that the compiler \ parser does not allow duplicate attributes. But if you think it's compelling we can definitely guard against it.

I know it could be argued as an edge case. If our docs say that "required" is just a hint that isn't really enforced, then we could skip the robustness. But I suspect we want to document that "required" means "we really enforce this for reals".

BTW I really do think the "you can't have > 32 required parameters" would be a very reasonable restriction if this simplifies things significantly.

pranavkm · 2020-08-22T03:37:45Z

Updated numbers. ComplexTable-from-blank appears to have bumped up (it hovered around that number in a few runs), but the rest appear to be about the same.

scenario	before	after
render-10-items	5.830942	5.827246375543916
render-1000-items	202.9025	201.2624
orgchart-3-3-org	796.895	787.284999
edit-orgchart-3-2	293.3725	289.2225
render-plaintable-from-blank	566.58	586.7700
render-plaintable-switch-pages	282.06	284.762
render-complextable-from-blank	3680.645	3757.37
render-complextable-switch-pages	849.325	852.04
render-fastgrid-from-blank	95.179	96.25
render-fastgrid-switch-pages	77.735	78.06

pranavkm · 2020-08-22T03:40:25Z

src/Components/Components/src/Reflection/IPropertySetter.cs

+
+            var setMethod = property.SetMethod;
+
+            var propertySetterAsAction =


This is identical to what PropertyHelper does: https://github.com/dotnet/aspnetcore/blob/master/src/Shared/PropertyHelper/PropertyHelper.cs#L312-L321. I wasn't sure if we intentionally avoided using that type in here for some reason, so I copied it over. It avoids a Activator.CreateInstance call that we previously had per property which would be an improvement in my books

pranavkm · 2020-08-22T03:41:22Z

src/Components/Components/src/Routing/Router.cs

        /// Gets or sets the content to display when a match is found for the requested route.
        /// </summary>
-        [Parameter] public RenderFragment<RouteData> Found { get; set; }
+        [Parameter(Required = true)] public RenderFragment<RouteData> Found { get; set; }


Yup, I could remove the checks from this type. They're about as generic as they come.

* Use the pattern from PropertyHelpers to set property values * Tweaks to Blazor WebAssembly tests to allow running tests locally

pranavkm · 2020-08-24T18:57:29Z

@SteveSandersonMS I yanked out all the Required parameters support. I would still like to take the change to use a concrete PropertySetter type and the other benchmark infrastructure changes

SteveSandersonMS

Cool

ghost · 2020-08-24T21:06:38Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

src/Components/Components/src/Reflection/IPropertySetter.cs

captainsafia

Changes look legit.

ghost added the area-blazor Includes: Blazor, Razor Components label Aug 19, 2020

pranavkm marked this pull request as ready for review August 20, 2020 16:55

pranavkm requested review from a team and SteveSandersonMS as code owners August 20, 2020 16:55

captainsafia reviewed Aug 21, 2020

View reviewed changes

captainsafia approved these changes Aug 21, 2020

View reviewed changes

javiercn reviewed Aug 21, 2020

View reviewed changes

javiercn approved these changes Aug 21, 2020

View reviewed changes

pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 21, 2020

pranavkm added this to the 5.0.0-rc1 milestone Aug 21, 2020

SteveSandersonMS reviewed Aug 21, 2020

View reviewed changes

pranavkm commented Aug 22, 2020

View reviewed changes

pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 24, 2020

pranavkm mentioned this pull request Aug 24, 2020

Required parameters to blazor components #11815

Closed

pranavkm force-pushed the prkrishn/required-component-parameters branch 2 times, most recently from 56d3bbb to 7d5aa19 Compare August 24, 2020 18:55

Make PropertySetter a concrete type

3eb9546

* Use the pattern from PropertyHelpers to set property values * Tweaks to Blazor WebAssembly tests to allow running tests locally

pranavkm force-pushed the prkrishn/required-component-parameters branch from 7d5aa19 to 3eb9546 Compare August 24, 2020 18:56

pranavkm changed the title ~~Add support for required component parameters~~ Make PropertySetter a concrete type Aug 24, 2020

SteveSandersonMS approved these changes Aug 24, 2020

View reviewed changes

pranavkm added the auto-merge label Aug 24, 2020

davidfowl reviewed Aug 24, 2020

View reviewed changes

src/Components/Components/src/Reflection/IPropertySetter.cs Outdated Show resolved Hide resolved

Update src/Components/Components/src/Reflection/IPropertySetter.cs

d019b25

captainsafia self-requested a review August 25, 2020 01:43

captainsafia approved these changes Aug 25, 2020

View reviewed changes

ghost merged commit 29cbf6e into release/5.0 Aug 25, 2020

ghost deleted the prkrishn/required-component-parameters branch August 25, 2020 01:54

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

This pull request was closed.


		public string? CaptureUnmatchedValuesPropertyName { get; }

		public IReadOnlyDictionary<string, IPropertySetter> Writers => _underlyingWriters;

	public IReadOnlyDictionary<string, IPropertySetter> Writers => _underlyingWriters;
	public IEnumerable<string> GetKnownPropertyNamesInOrder()
	=> _underlyingWriters.Keys.OrderBy(kvp => kvp.Key, StringComparer.Ordinal);


		var setMethod = property.SetMethod;

		var propertySetterAsAction =

Make PropertySetter a concrete type #25054

Make PropertySetter a concrete type #25054

Uh oh!

Conversation

pranavkm commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Aug 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Aug 24, 2020

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 24, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pranavkm commented Aug 19, 2020 •

edited

Loading

SteveSandersonMS Aug 21, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.