MetaCell / geppetto-meta

Other
4 stars 4 forks source link

Ensure synched state between redux widgets state with flexLayout widget state #108

Open lrebscher opened 3 years ago

lrebscher commented 3 years ago
  1. Invalid states due to middleware and reducer, if middleware stops action but reducer carries on we get invalid widget states, need better error handling & clear understanding of what is allowed and what not

  2. Redux widgets state is not in synch with layout state, user actions on the layout is not consistently propagated to widget state, e.g. rename or maximize.

Maximizing a widget by double clicking on the tab doesn't change the redux widget state to MAXIMIZED.

We would have to hook into the maximize action of flexLayout and dispatch a redux action that changes the widget state.

Two cases:

We have the following two action patterns:

FlexLayout-Action are actions triggered by FlexLayout itself. We hook into these actions with the onAction callback.

I think we better avoid to dispatch the event in 2 that we use in 1 to trigger the FlexLayout action.

enicolasgomez commented 2 years ago

Going over it as it's related to the redux review planned to do during Jan/Feb/

filippomc commented 2 years ago

@ddelpiano this is exactly the problem you showed me yesterday. There are 2 missing pieces to fix:

  1. The reducer is ignoring the maximize/minimize and change of active tab actions https://github.com/MetaCell/geppetto-meta/blob/master/geppetto.js/geppetto-client/src/common/layout/reducer.ts#L84
  2. The handler in the LayoutManager is not dispatching the actions when they are coming from the layout actions made by user interactions (for instance, here

Something to be careful is that since that the LayoutManager is listening to the actions also as middleware we must be careful avoiding loops

filippomc commented 2 years ago

I think this is being addressed with #290 cc @ddelpiano

ddelpiano commented 2 years ago

I think this is being addressed with #290 cc @ddelpiano

@filippomc partially addressed, I fixed what I could implement and test in netpyne so there are still bits and pieces of that part of middleware to be fixed.

enicolasgomez commented 2 years ago

@filippomc @ddelpiano I've noticed this while working on NetPyNE's plots. If the getPlot function fails the middleware misses synchronization and tags the widget as initiated, when it failed.

IMHO, we don't need to fix specific cases but re-think the middlewares so they don't include business logic (e.g. opening a plot) in favor of any mechanism that would allow async state update into the reducer:

https://redux.js.org/tutorials/fundamentals/part-6-async-logic

filippomc commented 2 years ago

@enicolasgomez I don't think this specific issue has anything to do with the middlewares: what we need to do here is to connect the layout action handlers that are now missing synchronization with the widgets state.

About the Netpyne issue you @enicolasgomez mention I suspect it is unrelated as it's more about implementing the error handler when it's called, whathever is the pattern used.

As for the general middleware thoughts I think running the async calls in the middleware I still think it's a best practice and I don't see any drawback, other that the possible confusion of following the pattern the first times. Also the article you posted suggests to use middlewares. Thunk is a nice alternative for clarity for simple cases as it keeps the logic with the action's code, but let me point out that it's still syntactic sugar to run callback code specified in the actions inside a middleware. Although I don't see big advantages I'm happy to use thunk as it helps clarity, but let's open another specific refactoring issue.

enicolasgomez commented 2 years ago

@filippomc what Lucas mentioned here as "better error handling" is for my point of view an overhead needed by using this middleware centric approach, if you put tunk on top of the reducer with an async action, instead of having to handle the specifics (like propagating or communicating error or any other state), you just get an exception from the async call which the reducer would receive and update synchronously to it's state so any connected component will be notified off though redux framework.

Will prepare a minimalistic example so we can discuss over Thursday's meeting and make my point more clear.

filippomc commented 2 years ago

I don't think the first point is really valid as stated: "middleware stops action but reducer carries on" should not be possible by design, so probably there is something else that may happen that Lucas was referring to and we need to catch