From 84793f5a22a150e3966fda0059732b7acbc30cdf Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Fri, 19 Aug 2022 16:28:37 -0700 Subject: [PATCH] Hot reload: reinitialize component on param remove --- .../Components/src/ParameterView.cs | 55 ++++++ .../src/RenderTree/RenderTreeDiffBuilder.cs | 88 +++++----- .../Components/test/ParameterViewTest.cs | 166 ++++++++++++++++++ 3 files changed, 265 insertions(+), 44 deletions(-) diff --git a/src/Components/Components/src/ParameterView.cs b/src/Components/Components/src/ParameterView.cs index 3c2f3ad9dc3f..23b989e98507 100644 --- a/src/Components/Components/src/ParameterView.cs +++ b/src/Components/Components/src/ParameterView.cs @@ -197,6 +197,61 @@ internal bool DefinitelyEquals(ParameterView oldParameters) } } + internal bool HasRemovedDirectParameters(in ParameterView oldParameters) + { + var oldDirectParameterFrames = GetDirectParameterFrames(oldParameters); + if (oldDirectParameterFrames.Length == 0) + { + // Parameters could not have been removed if there were no old direct parameters. + return false; + } + + var newDirectParameterFrames = GetDirectParameterFrames(this); + if (newDirectParameterFrames.Length < oldDirectParameterFrames.Length) + { + // Parameters must have been removed if there are fewer new direct parameters than + // old direct parameters. + return true; + } + + // Fall back to comparing each set of direct parameters. + foreach (var oldFrame in oldDirectParameterFrames) + { + var found = false; + foreach (var newFrame in newDirectParameterFrames) + { + if (string.Equals(oldFrame.AttributeNameField, newFrame.AttributeNameField, StringComparison.Ordinal)) + { + found = true; + break; + } + } + + if (!found) + { + return true; + } + } + + return false; + + static Span GetDirectParameterFrames(in ParameterView parameterView) + { + var frames = parameterView._frames; + var ownerIndex = parameterView._ownerIndex; + var ownerDescendantsEndIndexExcl = ownerIndex + frames[ownerIndex].ElementSubtreeLength; + var attributeFramesStartIndex = ownerIndex + 1; + var attributeFramesEndIndexExcl = attributeFramesStartIndex; + + while (attributeFramesEndIndexExcl < ownerDescendantsEndIndexExcl && frames[attributeFramesEndIndexExcl].FrameType == RenderTreeFrameType.Attribute) + { + attributeFramesEndIndexExcl++; + } + + return frames.AsSpan(attributeFramesStartIndex..attributeFramesEndIndexExcl); + } + } + internal void CaptureSnapshot(ArrayBuilder builder) { builder.Clear(); diff --git a/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs b/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs index ff20902e3527..be07fb06a385 100644 --- a/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs +++ b/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs @@ -533,45 +533,6 @@ private static void AppendAttributeDiffEntriesForRangeSlow( diffContext.AttributeDiffSet.Clear(); } - private static void UpdateRetainedChildComponent( - ref DiffContext diffContext, - int oldComponentIndex, - int newComponentIndex) - { - var oldTree = diffContext.OldTree; - var newTree = diffContext.NewTree; - ref var oldComponentFrame = ref oldTree[oldComponentIndex]; - ref var newComponentFrame = ref newTree[newComponentIndex]; - var componentState = oldComponentFrame.ComponentStateField; - - // Preserve the actual componentInstance - newComponentFrame.ComponentStateField = componentState; - newComponentFrame.ComponentIdField = componentState.ComponentId; - - // As an important rendering optimization, we want to skip parameter update - // notifications if we know for sure they haven't changed/mutated. The - // "MayHaveChangedSince" logic is conservative, in that it returns true if - // any parameter is of a type we don't know is immutable. In this case - // we call SetParameters and it's up to the recipient to implement - // whatever change-detection logic they want. Currently we only supply the new - // set of parameters and assume the recipient has enough info to do whatever - // comparisons it wants with the old values. Later we could choose to pass the - // old parameter values if we wanted. By default, components always rerender - // after any SetParameters call, which is safe but now always optimal for perf. - - // When performing hot reload, we want to force all components to re-render. - // We do this using two mechanisms - we call SetParametersAsync even if the parameters - // are unchanged and we ignore ComponentBase.ShouldRender - - var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, oldTree, oldComponentIndex); - var newParametersLifetime = new ParameterViewLifetime(diffContext.BatchBuilder); - var newParameters = new ParameterView(newParametersLifetime, newTree, newComponentIndex); - if (!newParameters.DefinitelyEquals(oldParameters) || (HotReloadManager.Default.MetadataUpdateSupported && diffContext.Renderer.IsRenderingOnMetadataUpdate)) - { - componentState.SetDirectParameters(newParameters); - } - } - private static int NextSiblingIndex(in RenderTreeFrame frame, int frameIndex) { switch (frame.FrameTypeField) @@ -699,11 +660,50 @@ private static void AppendDiffEntriesForFramesWithSameSequence( { if (oldFrame.ComponentTypeField == newFrame.ComponentTypeField) { - UpdateRetainedChildComponent( - ref diffContext, - oldFrameIndex, - newFrameIndex); - diffContext.SiblingIndex++; + // As an important rendering optimization, we want to skip parameter update + // notifications if we know for sure they haven't changed/mutated. The + // "MayHaveChangedSince" logic is conservative, in that it returns true if + // any parameter is of a type we don't know is immutable. In this case + // we call SetParameters and it's up to the recipient to implement + // whatever change-detection logic they want. Currently we only supply the new + // set of parameters and assume the recipient has enough info to do whatever + // comparisons it wants with the old values. Later we could choose to pass the + // old parameter values if we wanted. By default, components always rerender + // after any SetParameters call, which is safe but now always optimal for perf. + + // When performing hot reload, we want to force all components to re-render. + // We do this using two mechanisms - we call SetParametersAsync even if the parameters + // are unchanged and we ignore ComponentBase.ShouldRender. + // Furthermore, when a hot reload edit removes component parameters, the component should be + // disposed and reinstantiated. This allows the component's construction logic to correctly + // re-initialize the removed parameter properties. + + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, oldTree, oldFrameIndex); + var newParametersLifetime = new ParameterViewLifetime(diffContext.BatchBuilder); + var newParameters = new ParameterView(newParametersLifetime, newTree, newFrameIndex); + var isHotReload = HotReloadManager.Default.MetadataUpdateSupported && diffContext.Renderer.IsRenderingOnMetadataUpdate; + + if (isHotReload && newParameters.HasRemovedDirectParameters(oldParameters)) + { + // Components with parameters removed during a hot reload edit should be disposed and reinstantiated + RemoveOldFrame(ref diffContext, oldFrameIndex); + InsertNewFrame(ref diffContext, newFrameIndex); + } + else + { + var componentState = oldFrame.ComponentStateField; + + // Preserve the actual componentInstance + newFrame.ComponentStateField = componentState; + newFrame.ComponentIdField = componentState.ComponentId; + + if (!newParameters.DefinitelyEquals(oldParameters) || isHotReload) + { + componentState.SetDirectParameters(newParameters); + } + + diffContext.SiblingIndex++; + } } else { diff --git a/src/Components/Components/test/ParameterViewTest.cs b/src/Components/Components/test/ParameterViewTest.cs index 584609b0dc09..740359e5cb0a 100644 --- a/src/Components/Components/test/ParameterViewTest.cs +++ b/src/Components/Components/test/ParameterViewTest.cs @@ -402,6 +402,172 @@ public void Clone_ParameterPreservesOrder() p => AssertParameter("attribute 3", attribute3Value, expectedIsCascading: false)); } + [Fact] + public void HasRemovedDirectParameters_BothEmpty() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(1), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(1), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.False(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_OldEmpty_NewNonEmpty() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(1), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.False(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_OldNonEmpty_NewEmpty() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(1), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.True(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_ParameterRemoved() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 2", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(3), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 3", "value 3"), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.True(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_ParameterReplaced() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 2", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 2", "value 1"), + RenderTreeFrame.Attribute(2, "attribute replaced", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.True(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_ParameterReplacedAndAdded() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 2", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(5), + RenderTreeFrame.Attribute(1, "attribute 2", "value 1"), + RenderTreeFrame.Attribute(2, "attribute replaced", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + RenderTreeFrame.Attribute(4, "attribute 4", "value 3"), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.True(hasRemovedDirectParameters); + } + + [Fact] + public void HasRemovedDirectParameters_ParametersSwapped() + { + // Arrange + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 2", "value 2"), + RenderTreeFrame.Attribute(3, "attribute 3", "value 3"), + }, 0); + var newParameters = new ParameterView(ParameterViewLifetime.Unbound, new[] + { + RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(4), + RenderTreeFrame.Attribute(1, "attribute 1", "value 1"), + RenderTreeFrame.Attribute(2, "attribute 3", "value 3"), + RenderTreeFrame.Attribute(3, "attribute 2", "value 2"), + }, 0); + + // Act + var hasRemovedDirectParameters = newParameters.HasRemovedDirectParameters(oldParameters); + + // Assert + Assert.False(hasRemovedDirectParameters); + } + private Action AssertParameter(string expectedName, object expectedValue, bool expectedIsCascading) { return parameter =>