From 94ad26a4797d1ae724b9679d8c567a89a3a2b9b0 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 26 Jan 2018 11:58:43 -0800 Subject: [PATCH] Update properties on retained child components --- .../RenderTree/RenderTreeDiffComputer.cs | 147 +++++++++++++++++- .../Rendering/ComponentState.cs | 5 + .../Rendering/Renderer.cs | 35 ----- .../RenderTreeDiffComputerTest.cs | 32 ++++ .../RendererTest.cs | 54 +++++++ 5 files changed, 230 insertions(+), 43 deletions(-) diff --git a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs b/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs index 963a436b96..7479658696 100644 --- a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs +++ b/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 - // rather than instantiating a new one - newTree[newNodeIndex].SetChildComponentInstance( - oldTree[oldNodeIndex].ComponentId, - oldTree[oldNodeIndex].Component); - - // TODO: Compare attributes and notify the existing child component - // instance of any changes + UpdateRetainedChildComponent( + oldTree, oldNodeIndex, + newTree, newNodeIndex); 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); + } } } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs index 6d38a6ea95..d565f4aebc 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs +++ b/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); } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs index 7dfdc43153..e7f344bc2a 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs +++ b/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); - } - } } } diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs index 5f791a9127..f172ef03ac 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs +++ b/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(12); + oldTree.AddAttribute(13, nameof(FakeComponent.StringProperty), "String will change"); + oldTree.AddAttribute(14, nameof(FakeComponent.ObjectProperty), objectWillNotChange); + oldTree.CloseElement(); + newTree.OpenComponentElement(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) diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs index 738348d426..191444dc09 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs +++ b/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(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; }