dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.56k stars 10.05k forks source link

[Blazor] Replacing EditForm Model causes component destruction #41621

Open uecasm opened 2 years ago

uecasm commented 2 years ago

Is there an existing issue for this?

Describe the bug

Reassigning the Model of an EditForm causes it to create a new EditContext, which in turn causes it to destroy and re-create all child components of the form.

Expected Behavior

Regardless of whether it creates a new EditContext or updates it in place, child components should not be destroyed and recreated, they should only have their parameters set and re-rendered.

Steps To Reproduce

Create a page with:

<InitReporter Name="top" />
<EditForm Model="@_Model">
    <InitReporter Name="form" />
</EditForm>

Also make a property for _Model (of any desired type) and a button that performs something similar to _Model = new ModelType();.

Create InitReporter:

public class InitReporter : ComponentBase, IDisposable
{
    [Parameter, EditorRequired] public string? Name { get; set; }

    protected override void OnInitialized()
    {
        Console.WriteLine($"{Name} init");

        base.OnInitialized();
    }

    public void Dispose()
    {
        Console.WriteLine($"{Name} dispose");
    }

    protected override void OnParametersSet()
    {
        Console.WriteLine($"{Name} set parameters");

        base.OnParametersSet();
    }
}
  1. Load the page.
  2. Observe that top+form init/set are logged as expected.
  3. Click the button to assign a different model instance.
  4. Observe that the form logger reports that it was destroyed and re-created, and the top logger does not.
  5. Further note that this component isn't even dependent on the model anyway, and yet this still happens -- although even input controls and other things that do depend on the EditContext really shouldn't be re-created either.

Exceptions (if any)

No response

.NET Version

6.0.201

Anything else?

.NET SDK (reflecting any global.json):
 Version:   6.0.201
 Commit:    ef40e6aa06

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19041
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.201\

Host (useful for support):
  Version: 6.0.3
  Commit:  c24d9a9c91

.NET SDKs installed:
  2.1.511 [C:\Program Files\dotnet\sdk]
  6.0.200 [C:\Program Files\dotnet\sdk]
  6.0.201 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download
SteveSandersonMS commented 2 years ago

As things stand, this is by design. It means that all descendant components don't have to subscribe for changes to the EditContext identity, which makes them much cheaper and enables scaling up to gigantic forms.

We could potentially change it, perhaps even requiring the developer to pass a special flag saying "I might change this model later" so we know to disable the optimization. Requiring the flag would also help avoid such a change from being a breaking change in edge cases.

Moving to backlog, as we'd need to collect more evidence of community demand in order to prioritize this above other more well-established feature requests.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

uecasm commented 2 years ago

There's no difficulty in "subscribing" for changes to the EditContext -- it's easily detectable in SetParameters, and in fact components like InputBase already do detect this and throw an exception (though I'm not sure why; it doesn't seem like it would be hard to unsubscribe events on the old context and resubscribe on the new one, since both are available). Even that would not be necessary if the EditContext were updated in place instead of being recreated, although there may be good reasons for that.

Another possible interim workaround might be a way for a specific component to opt-out of being recreated via an attribute or something, to indicate that it does support changing contexts?


Where I originally noticed this is in a component that does some JS Interop using the "import private module on first render" technique; it seems wasteful to constantly make it discard and reload the module, and there's also some other state that it can only gather after first render that causes it to flicker whenever this re-creation occurs (both due to some changed state and due to CSS transitions). It was even worse when I tried to use an external entirely-JS control that did a bunch of DOM manipulation to add/remove itself (and, as it turned out, didn't completely remove itself properly; not sure if that was a bug due to not commonly being needed, or due to an unintended interaction with Blazor itself).

Although what led me to replacing the model instance in the first place is because there didn't seem to be any easy way to deserialize incoming JSON from the server side into an existing instance (either directly, or by after-the-fact overwriting an existing instance from a new instance, including removals), and other than this re-creation effect everything rebinds correctly to new instances just fine. Perhaps making that scenario easier might reduce the need for this one?

SteveSandersonMS commented 2 years ago

There's no difficulty in "subscribing" for changes to the EditContext -- it's easily detectable in SetParameters, and in fact components like InputBase already do detect this and throw an exception

It's not about difficulty, it's about performance. The framework uses the IsFixed=true optimization on cascaded edit contexts, which it wouldn't be able to do if they could change without recreating the subtree.

Although what led me to replacing the model instance in the first place is because there didn't seem to be any easy way to deserialize incoming JSON from the server side into an existing instance ... Perhaps making that scenario easier might reduce the need for this one?

Yes, that might be a good route to take. The following idea might be a bit simplistic but let's just check: what if you have a container object that just holds your model instance? For example, if the thing you're editing is a Customer object:

public class CustomerEditState
{
    // This is the thing that gets JSON serialized/deserialized
    public Customer Customer { get; set; }
}
uecasm commented 2 years ago

The following idea might be a bit simplistic but let's just check: what if you have a container object that just holds your model instance? For example, if the thing you're editing is a Customer object:

That's an interesting idea, and it does indeed stop the re-creation of all components. However something a bit weirder now happens instead. The form doesn't just edit properties on Customer directly, there's a list of other data inside that, and rather than directly foreaching and binding to those objects it creates some viewmodel wrappers with some extra properties, and binds to those. On initial load and edit this is still working as expected, but then I have a button which replaces the top-level model when clicked (and also replaces the vm.Model property instance without replacing the viewmodel instance itself), and these are now updating to the wrong values when the button is clicked (blank/default values instead of the actual values); whereas it was working (other than the flicker) when it did re-create everything.

I strongly suspect what's going wrong here is that it's not updating the FieldIdentifier to refer to the new model instances, and getting confused as a result. Having said that, I did also try to rearrange things so it would bind to an object that didn't change instances (i.e. bind to vm.Prop instead of to vm.Model.Prop), and this didn't help, so I may be mistaken as to the cause. (But it would have been too fragile to consider a real solution anyway.)


Edit: hmm, it was working as expected when I tried distilling it down to an MCVE, so clearly there's some trigger for the weirdness that I haven't managed to extract out of it yet...

uecasm commented 2 years ago

Well, derp. The button that was causing problems was because it was set as <button type="reset"/> instead of a regular button. Interestingly, this was apparently resetting the form state before the onclick handler for the button ran, but then also ignoring the change to the model objects caused by that handler; and this didn't start happening until after I made the change above to not reset the EditContext.

Changing it to a regular non-reset button fixes all issues that I was having with this change (or at least I haven't noticed any new ones yet). (It does still look like the components don't update their FieldIdentifier, but at least so far this doesn't appear to be causing any problems that I can see. Though I'm not using validation in this particular form anyway.)

Perhaps this technique (making a wrapper model for the EditForm instead of directly switching instances) should just be documented, and then we could call this sufficiently "done"?

uecasm commented 2 years ago

Sadly, after further experimentation it appears that just adding a wrapper model doesn't really help when also using validation.

The problem appears to be that the FieldIdentifier captures actual object instances, so when these instances are changed under the hood without resetting the entire EditContext it gets confused and things are left thinking that they're valid when they're not, or not modified when they are, or other weirdness.

There doesn't appear to be any easy way to cascade object instance updates through an edit context while retaining messages (since most of it is marked internal). It would probably have worked ok if the field identifiers were property-path-based instead (which is how WPF and many other validation libraries work). But failing that it looks like the only solution is to update the object graph in-place.

So it would definitely be nice if System.Text.Json or elsewhere did have some way to overwrite existing objects rather than making a new tree (or, having already created a new tree, a method to overwrite the existing instances from that). Especially since it could use the source-generator metadata for that purpose to avoid reflection.

For the moment I've got it working as expected again by using AutoMapper to overwrite the existing instances, but it uses reflection (so I suspect I'll run into problems again once I start looking at AoT and assembly pruning) and very much feels like using a torque wrench on a nail. (And out of the box it doesn't do well with merging lists.)

gingters commented 1 year ago

I have a similar problem, but from a different approach. I am working on a (very complex) that should provide undo/redo functionality. The main issue is that it's currently not possible to read (persist) / and restore the field states.

Whenever a field changes, I can store a copy of the current model. I also need to keep track of which fields changed in order to being able to determine if a field was touched but not actually changes. This is required in order to recreate i.e. field required validation messages when a previous state is restored.

Also, when resetting to a previous state I have to re-recreate the model with a new copy of the previous state and create a new editcontext, which then re-creates the whole form, and and then I need to manually notify each of the stored fields as being changed to restore the validation for that specific step.

That also means, that in order to keep the state of complex forms (i.e. with expanders for parts of the form), I also need to store their state outside of the form to be able to restore them after each undo/redo operation.

I am fighting relatively hard with this feature to make it as generic as possible. Any suggestions on how that is supposed to work?

gingters commented 1 year ago

For the moment I've got it working as expected again by using AutoMapper to overwrite the existing instances, but it uses reflection (so I suspect I'll run into problems again once I start looking at AoT and assembly pruning) and very much feels like using a torque wrench on a nail. (And out of the box it doesn't do well with merging lists.)

As an idea, you could try to use Mapperly. This uses a source generator at compile time to generate the mapping code. So no reflection necessary and its very fast at runtime.

ghost commented 11 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

ThisIsPedroMoreira commented 7 months ago

I would need this to change because destroying a child content is not problematic until some of the components within an EditForm have some logic written on initialized (api call). That logic is unnecessarily executed twice.

UniMichael commented 4 months ago

We're also seeing this issue with our app. It seems Blazor might not be mature enough for our needs: an app that has complex data models and needs validation.

The fact that a use case this common doesn't seem to be supported is frankly very frustrating. This seems like the kind of thing most users of a SPA would run into, no?

nathanh2011 commented 4 months ago

Another group here would like to see this behaviour change as it is causes a lot of issues with our custom input components that interact with Javascript. Maybe a flag on the EditForm as suggested earlier would be the easiest option that way a person could "opt-in" on the change and take the performance hit of not re-creating all the components within.

Also, this issue took about 10 hours of developer time working the issue under the assumption the issue was with our components and not a behaviour with the EditForm's model. In a team of 5, no one automatically assumed simply setting the model to a new instance would cause such a cascading change which, in only my opinion, is an indication of a potential design failing.