Update properties on retained child components

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

@@ -2,6 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Reflection;
+using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.Rendering;
 
 namespace Microsoft.AspNetCore.Blazor.RenderTree
@@ -164,6 +166,126 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree
             }
         }
 
+        private void UpdateRetainedChildComponent(
+            RenderTreeNode[] oldTree, int oldComponentIndex,
+            RenderTreeNode[] newTree, int newComponentIndex)
+        {
+            // The algorithm here is the same as in AppendDiffEntriesForRange, except that
+            // here we don't optimise for loops - we assume that both sequences are forward-only.
+            // That's because this is true for all currently supported scenarios, and it means
+            // fewer steps here.
+
+            ref var oldComponentNode = ref oldTree[oldComponentIndex];
+            ref var newComponentNode = ref newTree[newComponentIndex];
+            var componentId = oldComponentNode.ComponentId;
+            var componentInstance = oldComponentNode.Component;
+
+            // Preserve the actual componentInstance
+            newComponentNode.SetChildComponentInstance(componentId, componentInstance);
+
+            // Now locate any added/changed/removed properties
+            var oldStartIndex = oldComponentIndex + 1;
+            var newStartIndex = newComponentIndex + 1;
+            var oldEndIndexIncl = oldComponentNode.ElementDescendantsEndIndex;
+            var newEndIndexIncl = newComponentNode.ElementDescendantsEndIndex;
+            var hasMoreOld = oldEndIndexIncl >= oldStartIndex;
+            var hasMoreNew = newEndIndexIncl >= newStartIndex;
+            while (hasMoreOld || hasMoreNew)
+            {
+                var oldSeq = hasMoreOld ? oldTree[oldStartIndex].Sequence : int.MaxValue;
+                var newSeq = hasMoreNew ? newTree[newStartIndex].Sequence : int.MaxValue;
+
+                if (oldSeq == newSeq)
+                {
+                    var oldName = oldTree[oldStartIndex].AttributeName;
+                    var newName = newTree[newStartIndex].AttributeName;
+                    var newPropertyValue = newTree[newStartIndex].AttributeValue;
+                    if (string.Equals(oldName, newName, StringComparison.Ordinal))
+                    {
+                        // Using Equals to account for string comparisons, nulls, etc.
+                        var oldPropertyValue = oldTree[oldStartIndex].AttributeValue;
+                        if (!Equals(oldPropertyValue, newPropertyValue))
+                        {
+                            SetChildComponentProperty(componentInstance, newName, newPropertyValue);
+                        }
+                    }
+                    else
+                    {
+                        // Since this code path is never reachable for Razor components (because you
+                        // can't have two different attribute names from the same source sequence), we
+                        // could consider removing the 'name equality' check entirely for perf
+                        SetChildComponentProperty(componentInstance, newName, newPropertyValue);
+                        RemoveChildComponentProperty(componentInstance, oldName);
+                    }
+
+                    oldStartIndex++;
+                    newStartIndex++;
+                    hasMoreOld = oldEndIndexIncl >= oldStartIndex;
+                    hasMoreNew = newEndIndexIncl >= newStartIndex;
+                }
+                else
+                {
+                    // Both sequences are proceeding through the same loop block, so do a simple
+                    // preordered merge join (picking from whichever side brings us closer to being
+                    // back in sync)
+                    var treatAsInsert = newSeq < oldSeq;
+
+                    if (treatAsInsert)
+                    {
+                        SetChildComponentProperty(componentInstance,
+                            newTree[newStartIndex].AttributeName,
+                            newTree[newStartIndex].AttributeValue);
+                        newStartIndex++;
+                        hasMoreNew = newEndIndexIncl >= newStartIndex;
+                    }
+                    else
+                    {
+                        RemoveChildComponentProperty(componentInstance,
+                            oldTree[oldStartIndex].AttributeName);
+                        oldStartIndex++;
+                        hasMoreOld = oldEndIndexIncl >= oldStartIndex;
+                    }
+                }
+            }
+        }
+
+        private static void RemoveChildComponentProperty(IComponent component, string componentPropertyName)
+        {
+            var propertyInfo = GetChildComponentPropertyInfo(component.GetType(), componentPropertyName);
+            var defaultValue = propertyInfo.PropertyType.IsValueType
+                ? Activator.CreateInstance(propertyInfo.PropertyType)
+                : null;
+            SetChildComponentProperty(component, componentPropertyName, defaultValue);
+        }
+
+        private static void SetChildComponentProperty(IComponent component, string componentPropertyName, object newPropertyValue)
+        {
+            var propertyInfo = GetChildComponentPropertyInfo(component.GetType(), componentPropertyName);
+            try
+            {
+                propertyInfo.SetValue(component, newPropertyValue);
+            }
+            catch (Exception ex)
+            {
+                throw new InvalidOperationException(
+                    $"Unable to set property '{componentPropertyName}' on component of " +
+                    $"type '{component.GetType().FullName}'. The error was: {ex.Message}", ex);
+            }
+        }
+
+        private static PropertyInfo GetChildComponentPropertyInfo(Type componentType, string componentPropertyName)
+        {
+            var property = componentType.GetProperty(componentPropertyName);
+            if (property == null)
+            {
+                throw new InvalidOperationException(
+                    $"Component of type '{componentType.FullName}' does not have a property " +
+                    $"matching the name '{componentPropertyName}'.");
+            }
+
+            return property;
+        }
+
         private static int NextSiblingIndex(RenderTreeNode[] tree, int nodeIndex)
         {
             var descendantsEndIndex = tree[nodeIndex].ElementDescendantsEndIndex;
@@ -175,6 +297,9 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree
             RenderTreeNode[] newTree, int newNodeIndex,
             ref int siblingIndex)
         {
+            // TODO: Refactor to use a "ref var oldNode/newNode" everywhere in this method
+            // Same in other methods in this class that do a lot of indexing into RenderTreeNode[]
+
             // We can assume that the old and new nodes are of the same type, because they correspond
             // to the same sequence number (and if not, the behaviour is undefined).
             switch (newTree[newNodeIndex].NodeType)
@@ -244,14 +369,9 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree
                         var newComponentType = newTree[newNodeIndex].ComponentType;
                         if (oldComponentType == newComponentType)
                         {
-                            // Since it's the same child component type, we'll preserve the instance
+                            UpdateRetainedChildComponent(
-                            // rather than instantiating a new one
+                                oldTree, oldNodeIndex,
-                            newTree[newNodeIndex].SetChildComponentInstance(
+                                newTree, newNodeIndex);
-                                oldTree[oldNodeIndex].ComponentId,
-                                oldTree[oldNodeIndex].Component);
-
-                            // TODO: Compare attributes and notify the existing child component
-                            // instance of any changes
 
                             siblingIndex++;
                         }
@@ -340,6 +460,17 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree
                     }
 
                     _renderer.InstantiateChildComponent(nodes, i);
+                    var childComponentInstance = nodes[i].Component;
+
+                    // All descendants of a component are its properties
+                    var componentDescendantsEndIndex = nodes[i].ElementDescendantsEndIndex;
+                    for (var attributeNodeIndex = i + 1; attributeNodeIndex <= componentDescendantsEndIndex; attributeNodeIndex++)
+                    {
+                        SetChildComponentProperty(
+                            childComponentInstance,
+                            nodes[attributeNodeIndex].AttributeName,
+                            nodes[attributeNodeIndex].AttributeValue);
+                    }
                 }
             }
         }

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

@@ -51,6 +51,11 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
             var diff = _diffComputer.ApplyNewRenderTreeVersion(
                 _renderTreeBuilderPrevious.GetNodes(),
                 _renderTreeBuilderCurrent.GetNodes());
+
+            // TODO: Instead of triggering the UpdateDisplay here, collect all the (componentId, diff)
+            // pairs from the whole render cycle, including descendant components, then send them
+            // in bulk to a single UpdateDisplay. Need to ensure that if the same component gets
+            // triggered multiple times that the diff reflects the entire chain.
             _renderer.UpdateDisplay(_componentId, diff);
         }
 

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

@@ -83,46 +83,11 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
             var newComponent = (IComponent)Activator.CreateInstance(node.ComponentType);
             var newComponentId = AssignComponentId(newComponent);
             node.SetChildComponentInstance(newComponentId, newComponent);
-            SetPropertiesOnComponent(nodes, componentNodeIndex);
         }
 
         private ComponentState GetRequiredComponentState(int componentId)
             => _componentStateById.TryGetValue(componentId, out var componentState)
                 ? componentState
                 : throw new ArgumentException($"The renderer does not have a component with ID {componentId}.");
-
-        private void SetPropertiesOnComponent(RenderTreeNode[] nodes, int componentNodeIndex)
-        {
-            ref var componentNode = ref nodes[componentNodeIndex];
-            var component = componentNode.Component;
-            var descendantsEndIndex = componentNode.ElementDescendantsEndIndex;
-            for (var attributeNodeIndex = componentNodeIndex + 1; attributeNodeIndex <= descendantsEndIndex; attributeNodeIndex++)
-            {
-                SetPropertyOnComponent(component, nodes[attributeNodeIndex]);
-            }
-        }
-
-        private void SetPropertyOnComponent(IComponent component, in RenderTreeNode attributeNode)
-        {
-            // TODO: Is it beneficial to cache the reflection?
-            var property = component.GetType().GetProperty(attributeNode.AttributeName);
-            if (property == null)
-            {
-                throw new InvalidOperationException(
-                    $"Component of type '{component.GetType().FullName}' does not have a property " +
-                    $"matching the name '{attributeNode.AttributeName}'.");
-            }
-
-            try
-            {
-                property.SetValue(component, attributeNode.AttributeValue);
-            }
-            catch (Exception ex)
-            {
-                throw new InvalidOperationException(
-                    $"Unable to set property '{attributeNode.AttributeName}' on component of " +
-                    $"type '{component.GetType().FullName}'. The error was: {ex.Message}", ex);
-            }
-        }
     }
 }

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

@@ -784,6 +784,38 @@ namespace Microsoft.AspNetCore.Blazor.Test
             Assert.Same(originalFakeComponent2Instance, newNode2.Component);
         }
 
+        [Fact]
+        public void UpdatesChangedPropertiesOnRetainedChildComponents()
+        {
+            // Arrange
+            var renderer = new FakeRenderer();
+            var oldTree = new RenderTreeBuilder(renderer);
+            var newTree = new RenderTreeBuilder(renderer);
+            var diff = new RenderTreeDiffComputer(renderer);
+            var objectWillNotChange = new object();
+            oldTree.OpenComponentElement<FakeComponent>(12);
+            oldTree.AddAttribute(13, nameof(FakeComponent.StringProperty), "String will change");
+            oldTree.AddAttribute(14, nameof(FakeComponent.ObjectProperty), objectWillNotChange);
+            oldTree.CloseElement();
+            newTree.OpenComponentElement<FakeComponent>(12);
+            newTree.AddAttribute(13, nameof(FakeComponent.StringProperty), "String did change");
+            newTree.AddAttribute(14, nameof(FakeComponent.ObjectProperty), objectWillNotChange);
+            newTree.CloseElement();
+
+            diff.ApplyNewRenderTreeVersion(new RenderTreeBuilder(renderer).GetNodes(), oldTree.GetNodes());
+            var originalComponentInstance = (FakeComponent)oldTree.GetNodes().Array[0].Component;
+            originalComponentInstance.ObjectProperty = null; // So we can see it doesn't get reassigned 
+
+            // Act
+            var result = diff.ApplyNewRenderTreeVersion(oldTree.GetNodes(), newTree.GetNodes());
+            var newComponentInstance = (FakeComponent)oldTree.GetNodes().Array[0].Component;
+
+            // Assert
+            Assert.Same(originalComponentInstance, newComponentInstance);
+            Assert.Equal("String did change", newComponentInstance.StringProperty);
+            Assert.Null(newComponentInstance.ObjectProperty); // To observe that the property wasn't even written, we nulled it out on the original
+        }
+
         private class FakeRenderer : Renderer
         {
             internal protected override void UpdateDisplay(int componentId, RenderTreeDiff renderTreeDiff)

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

@@ -331,6 +331,49 @@ namespace Microsoft.AspNetCore.Blazor.Test
                 node => Assert.Same(nestedComponentInstance, node.Component));
         }
 
+        [Fact]
+        public void UpdatesPropertiesOnRetainedChildComponentInstances()
+        {
+            // Arrange: First render, capturing child component instance
+            var renderer = new TestRenderer();
+            var objectThatWillNotChange = new object();
+            var firstRender = true;
+            var component = new TestComponent(builder =>
+            {
+                builder.OpenComponentElement<FakeComponent>(1);
+                builder.AddAttribute(2, nameof(FakeComponent.IntProperty), firstRender ? 123 : 256);
+                builder.AddAttribute(3, nameof(FakeComponent.ObjectProperty), objectThatWillNotChange);
+                builder.AddAttribute(4, nameof(FakeComponent.StringProperty), firstRender ? "String that will change" : "String that did change");
+                builder.CloseElement();
+            });
+
+            var rootComponentId = renderer.AssignComponentId(component);
+            renderer.RenderComponent(rootComponentId);
+
+            var originalComponentInstance = (FakeComponent)renderer.RenderTreesByComponentId[rootComponentId]
+                .Single(node => node.NodeType == RenderTreeNodeType.Component)
+                .Component;
+
+            // Assert 1: properties were assigned
+            Assert.Equal(123, originalComponentInstance.IntProperty);
+            Assert.Equal("String that will change", originalComponentInstance.StringProperty);
+            Assert.Same(objectThatWillNotChange, originalComponentInstance.ObjectProperty);
+
+            // Act: Second render
+            firstRender = false;
+            renderer.RenderComponent(rootComponentId);
+
+            var updatedComponentInstance = (FakeComponent)renderer.RenderTreesByComponentId[rootComponentId]
+                .Single(node => node.NodeType == RenderTreeNodeType.Component)
+                .Component;
+
+            // Assert
+            Assert.Same(originalComponentInstance, updatedComponentInstance);
+            Assert.Equal(256, updatedComponentInstance.IntProperty);
+            Assert.Equal("String that did change", updatedComponentInstance.StringProperty);
+            Assert.Same(objectThatWillNotChange, updatedComponentInstance.ObjectProperty);
+        }
+
         private class NoOpRenderer : Renderer
         {
             public new int AssignComponentId(IComponent component)
@@ -387,6 +430,17 @@ namespace Microsoft.AspNetCore.Blazor.Test
             }
         }
 
+        private class FakeComponent : IComponent
+        {
+            public int IntProperty { get; set; }
+            public string StringProperty { get; set; }
+            public object ObjectProperty { get; set; }
+
+            public void BuildRenderTree(RenderTreeBuilder builder)
+            {
+            }
+        }
+
         private class EventComponent : IComponent
         {
             public UIEventHandler Handler { get; set; }