mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.
MIT License
1.25k stars 144 forks source link

Discussion: How to avoid a mass rerendering of everything that depends on only unchanged parts of a State object? #220

Closed szalapski closed 2 years ago

szalapski commented 2 years ago

As a state object is immutable, anytime a reducer has a change to make, it creates a new state, replacing the old one. Any component that gets state properties and inherits from FluxorComponent will automatically rerender. Right?

So that means that any component that depends on one part of that state but not another part will still rerender regardless of what part of the state they need. In other words, if my component shows MyState.Value.A but not MyState.Value.B, and there is a change to MyState.Value.B, my component will rerender even though it doesn't need to.

Suppose I enable a rather complex domain object to be edited piecemeal. So the user might edit a CommentsPanel that internally uses CurrentOrderState.Value.OrderComments and a RushShippingPanel that internally uses CurrentOrderState.Value.IsRushShipping, along with dozens of other properties. So now any change anywhere in the state will result in dozens of components being rerendered when it is more desirable that only one part at a time needs to be rerendered. Is there any way to get more granular with rerendering of components that inherit from FluxorComponent? It strikes me that rerendering everything that depends on a state object when anything in that state object changes (because it's a whole new state each time) is a very blunt approach.

On another related note, for the whole app more broadly I was hoping perhaps to have one GlobalAppState class and have a tree of properties under it; that would make my state rather discoverable, as my teammates would call e.g. State.Value.CurrentCustomer.Name or State.Value.Customers.FirstOrDefault(c=>c.Name=="Acme Inc.") or State.Value.Ordering.CurrentOrder.IsProcessing, etc. That way we can use Intellisense to figure out exactly where in the State our desired property is.

But I believe such a "big state" approach is unwise, right, because a change anywhere in the state would result in a rerender of nearly everything everywhere?

So that means that having any degree of complexity in a State class is undesirable; I should keep each thing that might change independently in different state objects. Right?

So that means I will have dozens or even hundreds of state classes in a moderately sized project. How do you suggest organizing them?

mrpmorris commented 2 years ago

Hi

Discussions here are fine.

So that means that any component that depends on one part of that state but not another part will still rerender regardless of what part of the state they need.

Yes, that is correct. Note though that Blazor doesn't render the whole component to the page, it renders it to memory and then updates the differences in the page.

There is currently no way to react to the change of subsections. I don't think it would be difficult to write something.

I was hoping perhaps to have one GlobalAppState class

You can if you wish, but I would recommend against it. You can never know which parts of your state are in-use, therefore you can never clear any of it out, and you effectively have a memory leak.

I recommend having a state per UI use-case, this way you can clear them down as a response to navigation.

[ReducerMethod]
public static MyState ReduceGoAction(MyState state, GoAction action) =>
  action.newUri.StartsWith("/whatever") ? state : MyState.Empty;
mrpmorris commented 2 years ago

Regarding selectors, how about something like this? https://github.com/mrpmorris/Fluxor/issues/221

szalapski commented 2 years ago

OK, I understand that there hasn't been a more selective way to limit rerendering per property. I also understand (and agree) that multiple Features is probably best for state.

I still am thinking there should be a way to avoid rerendering for unchanged parts of the state, as it is inevitable that even the simplest properties on a state object will themselves have properties, many of which won't change. Maybe I'll look into it...I suppose it would take iterating through the whole tree of properties, not sure.

Note though that Blazor doesn't render the whole component to the page, it renders it to memory and then updates the differences in the page.

I see, I am using the term "rerender" differently. To me, "rerender" or "render" is the process by which .NET (in WebAssembly) runs the C# code to figure out the overall presentation state of each component in memory. After that, OnAfterRender is called, then manipulating the DOM is a subsequent process. I try to avoid using the term "render" to mean "manipulating the DOM", since it seems Microsoft uses it to mean running the C# code.

But for pages of significant scope, rerendering can be the source of much slowness. If I am showing a 30x10 cell table, and each cell has 1-3 components in it, it can be quite burdensome to rerender (run the C# code to determine what the DOM should become) hundreds of components. So it would be nice if there was a way to avoid rerendering a component if the parts of the state it depends on have not changed. I have written a small package to do this apart from state management, but I was hoping that changes due to state management wouldn't need such a thing.

Vue and React seem to automagically handle this (I think internally they compare previous state to new state on a per-property basis), but not so with Blazor (hence the need for StateHasChanged at all).

For what it's worth, I tried a parallel implementation using Blazor-State, and it suffers the same problem--a change to one property on a State object causes all components that read that state to rerender. :(

szalapski commented 2 years ago

Now looking at FluxorComponent.cs, I can't imagine an incremental way to implement more nuance here.

Basically, I want to choose whether or not to call StateHasChanged based on whether the particular properties consumed in the component have changed. I believe VueX and Redux in Javascript accomplish this by somehow rewriting each getter to effectively update the "cached" value in the state whenever the dependencies change--but I'm fuzzy on what the analog in Blazor/C# might be, if any.

Instead, thinking more in Blazor and C# paradigms, it seems to me that the StateSubscriber handler needs to know the subset of the state that has changed each time--this part seems doable--and then also the subset of the state that the child component is dependent on--this part seems tough. We could leave it up to the component to specify as I did in BlazorRerenderReducers, but that is really suboptimal design; it would be best if such could be figured out automatically, but I fear now we're looking at reflection of the component code, and perhaps difficult reflection at that.

uhfath commented 2 years ago

I have also stumbled upon this. But in my case the problem is not the performance but focus.

There is an EditForm which contains a few inputs with validations. But since we can't use 2-way binding (model is immutable) for this I had to override "onChange" events. Inside these events I dispatch an action to update the state. But after state is updated "StateHasChanged" is issued which interrupts focus change event, which in turn was the source for "onChange". As a result focus "flies away" to some other element. In my case after inputting some data in a text box and pressing "tab" next focused element is the body of the page and not next input.

It would be very handy to have and ability to control when "StateHasChanged" should be used. For instance in my case after text is being changed all I need is to update the state for this single field and not re-render the page.

szalapski commented 2 years ago

That is an interesting problem, uhfath. My thoughts from the outside looking in: whereas the display state of all the components on a page should depend on the data referenced by the component's code, the focus should largely be determined by the users prior actions.

I am a little surprised that StateHasChanged causes move of focus. Is the component in question removed from the DOM and readded when focus is lost? To test this, add a private Guid to the code and default it to Guid.NewGuid, then one-way bind this Guid to the id of the input in question. If you see the Guid change when focus is lost, then you know it is because of Blazor removing and readding the component... Which I would not expect even on StateHasChanged with new data. If that's the case, then I'd guess your code could be tweaked to avoid removing the element even on StateHasChanged. Could you share the .razor file here? It could be some other problem with how things are structured.

uhfath commented 2 years ago

@szalapski actually after your post I've created a clean repro of the case and it seemed to work just fine. The only difference with the real project is that I'm using some third-party components for my inputs and it seems they change focus themselves or do some other stuff and that doesn't play well with Fluxor updating the page almost at the same time. But still it would be nice to have some way to "filter" which actions should result in a "StateHasChanged" call and which should not. And that filter should be used on a per-case basis, e.g. per-page.

szalapski commented 2 years ago

@uhfath, I would argue that the goal is to automaticaly avoid unecessary re-rendering based on what data is changed, not what actions have happened. It is undesirable either for a state property you depend on has changed not to cause a rerender, or a state property you don't depend on has changed resulting in an unnecessary rerender. But somehow letting the component supress rerenderings based on its arbitrary logic? That shouldn't be a priority, I think.

mrpmorris commented 2 years ago

@uhfath I would advise against edit forms going into state. Edit a mutable object, and when the user clicks Save then update your state.

State is a record of truth. The changes aren't "true" until you save them.

mrpmorris commented 2 years ago

In the meantime, you can override ShouldRender on your component

public Person PreviousValue;

protected override bool ShouldRender()
{
  if (Object.ReferenceEquals(MyState.Person, PreviousValue))
    return false;
  PreviousValue = MyState.Person;
  return true;
}
uhfath commented 2 years ago

@uhfath I would advise against edit forms going into state. Edit a mutable object, and when the user clicks Save then update your state.

State is a record of truth. The changes aren't "true" until you save them.

Thanks. That was my previous version until I switched to Fluxor. It's just that when going this route it effectively forces me to create 3 exact copies of same entity: DTO, Immutable state and mutable state. Too much boilerplate for each form.

szalapski commented 2 years ago

In a previous project, we wanted to "save as we go", but we still updated a mutable object and called actions. There was no other way--the user might type an invalid value that the action/effect needs to reject, yet we still need a place to temporarily store the invalid value. The only way around that would be to reject it in the UI control which seems very inappropriate.

mrpmorris commented 2 years ago

@uhfath there is no need. Your effect can get your dto to your component without it going into state.

https://github.com/mrpmorris/Fluxor/tree/master/Tutorials/01-BasicConcepts/01E-ActionSubscriber

mrpmorris commented 2 years ago

@szalapski See https://github.com/mrpmorris/Fluxor/tree/master/Tutorials/01-BasicConcepts/01E-ActionSubscriber

uhfath commented 2 years ago

@uhfath there is no need. Your effect can get your dto to your component without it going into state.

https://github.com/mrpmorris/Fluxor/tree/master/Tutorials/01-BasicConcepts/01E-ActionSubscriber

Thanks, but this means there won't be any "state" which actually defeats the whole purpose. I guess using 3 entities is a kind of inevitable if we are about to make things right:

  1. one model goes for current "dirty" data before it's being validated
  2. one model goes for saving current clean state (in local storage, memory, etc.)
  3. one model goes for transferring this clean data to the API

If there is a change in business logic then any of these can be freely changed without affecting each other. Of course we can get away with one single mutable entity, shared across the stages, but that doesn't seem right.

So unless I'm missing something, to make things fast we go with single shareable entity. To make things good we go with boilerplate code.

The only issue I see here is that in (1) we have a separate state control inside UI component which I wanted to avoid and a mix of 2 states.

Imagine a SignIn page.

  1. The form is connected to a mutable state (SignInModel) which contains just the Username and Password properties
  2. A UI component (a page in this case) contains a second immutable state (SignInState with IsLoading and string[] Errors fields along with same Username and Password)
  3. When Username and Password are set a user clicks "Submit"
  4. In this click event we dispatch an action supplying these Username and Password from our SignInModel
  5. This action updates our SignInState, sets a loading spinner and clears errors
  6. An attached effect sends an API request to actually sign in a user
  7. If there are any errors this effect dispatches an action to update SignInState with them and hide a spinner
  8. This action has to clear the Password field in SignInState (as a good UX)
  9. And now we have to duplicate this part in our UI component to clear our SignInModel's Password field too which can be done in a subscribed callback

As a result our form is connected to 2 states (SignInModel and SignInState) and has a duplicate logic in a callback. Any ideas on how to deal with this?

Of course "SignIn" is just an example which could be done even without Flux pattern at all for it's simplicity, but these issues can be observed in other cases too.

uhfath commented 2 years ago

BTW, regarding action subscriptions. There is a case when I need to refresh some UI component when certain action occurs. But these third party components can be "refreshed" only using async methods whereas our actions are sync. How to overcome this? Didn't want to launch async code from sync callbacks since this seems to be a bad practice (exceptions handling, extra tasks, etc.).

mrpmorris commented 2 years ago

Thanks, but this means there won't be any "state" which actually defeats the whole purpose. I guess using 3 entities is a kind of inevitable if we are about to make things right:

There will probably be other state, but the DTO should not be part of it. So, for example, let's say you have a list of users that you can manage because you are an admin.

At the top-right of your page, you display the name of the user currently signed in - and let's say it is called CurrentUserState.

Now, when the user wants to fetch a user DTO from the server to edit, you'd do this

  1. Dispatch GetEditableUserAction (id of user)
  2. Reducer sets IsBusy = True in EditUserState
  3. Effect gets the DTO from the server
  4. Effect dispatches GetEditableUserActionResult(that DTO)
  5. Edit form has an ActionSubscriber, so grabs the DTO from the action and sets it to be the EditForm's Model property value.
  6. A reducer on CurrentUserState reduces this action if it is for the current user
[ReducerMethod]
public static CurrentUserState(CurrentUserState state, GetEditableUserActionResult action) =>
  action.UserDto.Id != state.UserId
  ? state
  : state with { GivenName = action.UserDto.GivenName, FamilyName = action.UserDto.FamilyName };

So, your other usecase (display current user's name) is aware that changes to a user's name can be known via the GetEditableUserActionResult action, and updates its own state only if it is relevant (i.e. it is the current user).

You would do the same later when you have a SaveCustomerActionResult.

  1. Dispatch SaveCustomerAction(that dto)
  2. Reducer sets IsBusy = true
  3. Effect sends the dto from the action to the server
  4. Server updates, and returns the new state from the server, and a success or fail flag.
  5. Effect dispatches a SaveCustomerActionResult(success, newDto)

The CurrentUserState can be reduced with the newDto value in the action, which either contains the same data that was just saved or, if another user has edited the data, it will contain the remote data.

So:

  1. The state is immutable.
  2. It is specific to the task at each "feature" needs.
  3. It remaps data in specific actions into its own representation that is needed for the feature.
  4. Some other state does the same thing.

So you may have a user's full name in 3 different states, held on properties of completely different classes (one about employees, one about users), but all are immutable and kept up-to-date with the last known "truth" about the data on the server.

Using this approach (rather than a monolith state) you are safe to clear down chunks of data when certain features are known not to be in use. For example, if you navigate to a URL that doesn't match the regex ^/admin/user.* then you can reduce your EditUserState to EditUserState.Empty

Don't store the user's password in state, it's a security risk (someone accessing their unlocked computer can get their password).

Is there anything about this that doesn't address what you need?

uhfath commented 2 years ago

@mrpmorris thanks for this info. Makes sense. However, I'm currently struggling with some deadlocks issues, probably because of the mix of sync/async parts (effects are async, action subscribers are sync and components are async). This makes things a bit tricky to implement correctly.

mrpmorris commented 2 years ago

@uhfath Deadlocks within Fluxor code?

uhfath commented 2 years ago

That's what I'm trying to figure out. Unfortunately Blazor wasm debugging is a bit limited and some third party components are involved, so I'm in a process of creating a clean repro.

mrpmorris commented 2 years ago

WASM is single threaded, do you have a .Wait() or .Result somewhere?

uhfath commented 2 years ago

Nope. Async all the way. I think I've found what the issue is in my case. But to keep this thread cleaner I've created a separate issue (#226)

mrpmorris commented 2 years ago

Closing in favour of #221