Cascading parameters #1545

SteveSandersonMS · 2018-10-09T15:30:36Z

This PR includes a long list of changes, but hopefully should be reasonably understandable if read one commit at a time. Many of the commits are (simple-ish) refactorings that were necessary ahead of subsequent work, e.g., to generalize things or make more info available to the system. I've prefixed each such commit message with Refactor: so it should be clear which commits introduce new functionality and which don't.

Since this PR has become overwhelmingly long already, I propose to merge this once the code review is completed based on the functionality here. Then I'll do remaining work in a further PR. Specifically:

To do in a follow-up PR

Some indicator for whether each incoming parameter is a cascading parameter or not
Option to declare a <CascadingValue> as "static" (or maybe call it FixedValue to avoid clash of terminology)
E2E tests

RemiBou · 2018-10-10T07:12:17Z

Is this linked to issue #1009 ?

SteveSandersonMS · 2018-10-10T08:00:05Z

@RemiBou No, it's independent of that.

stavroskasidis · 2018-10-10T13:30:01Z

I saw the demo for this in the ASP.NET Community Standup and I have to say, good job.

I think this will help me remove some hacks/workarounds I have used in my context menu component.

conficient · 2018-10-11T10:45:24Z

Would tree parameters support multiple instances?

In your ASP.NET community standup example you demo'd both a Theme parameter and a Tab parameter. Would it be possible to have more than one, so the theme controls the styling (at a layout level) and the tabset controls the tab (from the parent control). I guess it would be by the type of the parameter as there was no naming being implemented.

conficient · 2018-10-11T10:47:39Z

Oh, and presumably you can override a provider with a second instance. e.g. the Theme Provider on the layout is overridden on a specific page which uses a different Theme Provider?

SteveSandersonMS · 2018-10-11T10:54:30Z

Yes to both.

src/Microsoft.AspNetCore.Blazor/Components/Provider.cs

src/Microsoft.AspNetCore.Blazor/Components/ParameterAttribute.cs

rynowak · 2018-10-11T17:00:33Z

src/Microsoft.AspNetCore.Blazor/Components/Provider.cs

@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Blazor.Components
    /// <summary>
    /// A component that provides one or more parameter values to all descendant components.
    /// </summary>
-    public class Provider : IComponent
+    public class Provider<T> : ProviderBase, IComponent


What's the advantage of turning this into a generic? Is the idea to make it based on declared type rather than runtime type?

My only concern is about cases that don't work super well as generics - like delegate conversions

Is the idea to make it based on declared type rather than runtime type?

Yes. The key scenario I have in mind is if you're doing <Provider Value="someObject">, but someObject is sometimes null. If we did it based on runtime type, then the "overriding upstream values" logic would be very strange: it would override if it was non-null, but would not override if it was null. In fact there would be no possible way to override a non-null value with a null one. Given the way we identify the Provider just once per tree parameter usage, super bizarre things would happen whereby if the initial value was null, you wouldn't be able to observe it become non-null.

BTW it was really impressive how making the component generic just worked in every way I could think of (type inference, etc.).

src/Microsoft.AspNetCore.Blazor/Components/ParameterCollectionExtensions.cs

src/Microsoft.AspNetCore.Blazor/Components/Provider.cs

src/Microsoft.AspNetCore.Blazor/Components/ProviderBase.cs

src/Microsoft.AspNetCore.Blazor/Components/TreeParameterState.cs

rynowak · 2018-10-11T17:35:40Z

test/Microsoft.AspNetCore.Blazor.Test/TreeParameterStateTest.cs

+        }
+
+        [Fact]
+        public void FindTreeParameters_ComponentRequestsImplementedInterface_ReturnsMatches()


I think there's a decision point here about the combination of is based on the declared type of T for some Provider<T>. Generally you do one of these things or the other.

The pairings I expect to see are:

declared type + typeof(T) == ParameterType => Type is used as a 'key' or identity, rather then representing a set of optional policies/protocols

runtime type + ParameterType.IsAssignableFrom(Value.GetType()) => Value can implement a set of protocols and binding is flexible

I expect the reason why it is like it is that you didn't have the value available in all cases in CanSupplyValue OR because you wanted the binding between to ancestor and parent to stay stable when the same set of components renders?

Another consideration that I can see you thought of is typed null. Maybe what's here is great, I just want to talk a little more about the rationale.

OR because you wanted the binding between to ancestor and parent to stay stable when the same set of components renders?

Yes, this is an invariant I'm relying on to make the updates cheap. If this were not the case, then whenever any Provider value changes, we have to reconsider every possible descendant consumer to see whether the new runtime type matches, not just the ones that are currently matched to its value. In other words, if some consumer is asking for interface ISomething, then it has to subscribe to N ancestor providers instead of just one, because all the intermediate ones between itself and its original match might change their value to some new object that implements ISomething. The "typed null" thing is important too (commented about scenarios with that above).

Do you have examples of any scenarios where doing it based on declared type would be problematic/inconvenient for the developer?

Yes, this is an invariant I'm relying on to make the updates cheap. If this were not the case, then whenever any Provider value changes, we have to reconsider every possible descendant consumer to see whether the new runtime type matches, not just the ones that are currently matched to its value.

OK then it makes sense that we need a declared type. We'll just have to call this out clearly. My experience with similar features in model binding is that developers generally expect any system they use to have the maximum theoretical flexibility - but I think we have good reasons to make this based on declared typed. Ultimately we provide value by sometimes designing systems that limit flexibility where it's important.

Do you have examples of any scenarios where doing it based on declared type would be problematic/inconvenient for the developer?

No, I don't think it's problematic, since the binding to a type is static.

I was trying to rationalize this based on systems like WPF's static resources, or IPropertyOwner from VS's editor APIs where the type is used as a key.

The only case that would become problematic is if you wanted to bind values to a common framework type like IEquatable or IConvertable. I think our guidance would be "don't do that". For instance with this system, it would be possible to define a cascading parameter of type IComponent and then bind to the nearest ancestor that is a component. These things are of somewhat minimal utility, and somewhat average level of wierdness. None of it feels broken.

What this does allow you to do is treat a value you provide as a feature collection, but exporting a bunch of common interfaces. You could image defining a control system based on capability, and exporting commands. Consider something similar to UIA control patterns - now you have bubbling commands 😁

It seems really powerful as it is defined.

test/Microsoft.AspNetCore.Blazor.Test/TreeParameterStateTest.cs

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeFrame.cs

src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs

src/Microsoft.AspNetCore.Blazor/Components/Parameter.cs

src/Microsoft.AspNetCore.Blazor/Components/ParameterCollectionExtensions.cs

dlr1 · 2018-10-12T13:25:47Z

Would the provider value be an object that gets passed down as a whole or can we say we are interested in only a part of it.
Can this provider value be updated in descendants? If so, can we say I want the original(parent value, not the updated value) in the descendants further down.

SteveSandersonMS · 2018-10-12T13:33:43Z

Would the provider value be an object that gets passed down as a whole or can we say we are interested in only a part of it.

It's a whole object. If you want multiple independent things, use multiple independent providers.

Can this provider value be updated in descendants?

The flow is downwards. The expected pattern is you'd provide a callback that descendants can call if they want their ancestor to do something (such as change the value being provided).

dlr1 · 2018-10-12T13:47:25Z

The flow is downwards. The expected pattern is you'd provide a callback that descendants can call if they want their ancestor to do something (such as change the value being provided).

I was asking about a provider at parent provides 'foo' as value, immediate child (child1) updates the value to 'bar'. I want child of child1 to be able to say I want the value provided to it be from root parent not the immediate parent, In WPF we can say I want to bind to an ancestor of specific type.

rynowak · 2018-10-14T00:20:48Z

src/Microsoft.AspNetCore.Blazor/Components/ChangeDetection.cs

+                var newValueType = newValue.GetType();
+                if (oldValueType != newValueType            // Definitely different
+                    || !IsKnownImmutableType(oldValueType)  // Maybe different
+                    || !oldValue.Equals(newValue))          // Somebody says they are different


Is there really any value in this check about known immutable types? Is this here to avoid boxing?

It's because conceptually, the values we're comparing aren't really oldValue and newValue. The values we want to compare are the state reachable transitively from oldValue at some unknown point in the past versus the state reachable transitively from newValue now. The purpose of the check is to determine whether there's any chance that the deeply-reachable state is different now versus before.

If this was public API, we'd have to be much clearer about the semantics of this check and do some naming review to try to communicate this better. We could still change the name, but since it's internal, it's not as urgent.

With these semantics in mind, the only case where we can return true is for types known to be immutable. For, say, object, it's not enough for oldValue.Equals(newValue), because in most cases oldValue and newValue will be handles to the same object instance but that doesn't tell us anything about whether it's mutated.

It's unfortunate that .NET doesn't have any native concept of (im)mutability. That limits us to hard-coding a set of commonly-used known-immutable types.

src/Microsoft.AspNetCore.Blazor/Components/ChangeDetection.cs

rynowak · 2018-10-14T00:23:32Z

src/Microsoft.AspNetCore.Blazor/Components/ChangeDetection.cs

+        {
+            var oldIsNotNull = oldValue != null;
+            var newIsNotNull = newValue != null;
+            if (oldIsNotNull != newIsNotNull)


Seeing code like this really makes me wish we had constexpr utilities like IsValueType(T1) that could be optimized statically.

rynowak · 2018-10-14T00:24:45Z

src/Microsoft.AspNetCore.Blazor/Components/ChangeDetection.cs

+{
+    internal class ChangeDetection
+    {
+        public static bool MayHaveChanged<T1, T2>(T1 oldValue, T2 newValue)


Seeing that the only usage of this in this commit is for boxed values, I'm curious to see how this will get used.

Yes, I considered doing this without the generics, but TBH I didn't have any reason not to use them. Even if we can't benefit from having the types statically right now, maybe we will later. AFAICT it's not going to make any difference either way right now.

I know it means JITting different versions of the method for each combination of value types, but that's a limited fixed cost overall for any given app so I didn't think it was a drawback to worry about.

Let me know if you have concerns as I'm happy to change this. For now will proceed as-is.

rynowak · 2018-10-14T00:25:27Z

src/Microsoft.AspNetCore.Blazor/Components/Provider.cs

 using Microsoft.AspNetCore.Blazor.RenderTree;
 using System;
+using System.Collections.Generic;


System first?

rynowak · 2018-10-14T00:31:07Z

src/Microsoft.AspNetCore.Blazor/Components/ParameterCollection.cs

+            {
+                builder.Append(_frames, _ownerIndex + 1, numEntries);
+            }
+        }


I know I'm not super familiar with this code, but I'm totally lost looking at this.... what's this doing and why?

Yeah, I'm not super thrilled with this, but it's creating a clone of the parameter collection data that matches the shape of what ParameterCollectionEnumerator knows how to step through.

I previously tried making a new data structure but that forced either having two versions of ParameterCollectionEnumerator that understands each of them, or actually copying the parameter data to a new buffer even in the non-cascading case. Both seemed worse than this relatively contained bit of copying here.

rynowak

Alright, I think I've now read through everything. It looks great! I just had a few small comments/questions on the more recent bits.

SteveSandersonMS · 2018-10-15T10:53:14Z

@dotnet-bot test ci please

* Add Provider component * Implement discovery and matching rules for tree parameters * Remove artificial component hierarchy unit tests now they are redundant * Refactor: Have RenderTreeFrame point to the ComponentState instead of IComponent ... so we can more quickly find associated tree param state without having to do lookups based on the componentId. Also rename AssignComponentId to AttachAndInitComponent to be more descriptive. * Refactor: Add shared code path for updating parameters so there's only one place to attach tree parameters Now framework code should no longer call IComponent.SetParameters directly, except if it knows it's definitely dealing with a root component. * Refactor: Simplify Parameter by making it hold the name/value directly This will be necessary for tree parameters, which don't correspond to any RenderTreeFrame * Refactor: Wrap ParameterEnumerator logic in extra level of iterator so we can also add one for iterating tree params * Extend ParameterEnumerator to list tree parameters too * Include tree parameters in SetParameters calls * Refactor: Move parameter change detection logic into separate utility class ... so we include dotnet/jsinterop#3 * Refactor: Move tree parameter tests from RendererTest.cs their own file * Have Provider re-render consumers when value changes. Unit tests in next commit. * Components that accept tree parameters need to snapshot their direct params for later replay * Empty commit to reawaken CI * CR: Make name matching case-insensitive * Refactor: Rename Provider/TreeParameter to CascadingValue/CascadingParameter * Add dedicated [CascadingParameter] attribute. Remove FromTree flag. * CR: CascadingParameterState cleanups * CR: Extra unit test * CR: arguments/parameters * CR: Enumerator improvements * Fix test

SteveSandersonMS added this to the 0.7.0 milestone Oct 9, 2018

SteveSandersonMS changed the base branch from master to stevesa/track-ancestor-hierarchy-in-componentstate October 9, 2018 15:32

SteveSandersonMS force-pushed the stevesa/provider-component branch from 8d6271a to a0508bc Compare October 9, 2018 15:33