Define the stores (state-management) interaction mechanism

[lordrip]

Context

While working on multiple flows support, I noticed that we're using a few different ways to interact with the stores and that there's no clear (at least from my very friendly-and-humble-point-of-view 😄 ).

Way 1: (https://github.com/pmndrs/zustand#fetching-everything)

From the components, we bind the store completely by using:

const convenienceStore = useSevenElevenStore();

While this might be convenient as we get the state updated and also have access to the store API, it has the drawback of re-rendering the component even when unrelated properties change. Sometimes this is not a big problem by itself, it's very noticeable for instance in the Code Editor by the flickering that it generates.

Way 2: (https://github.com/pmndrs/zustand#selecting-multiple-state-slices)

const { fruitDepartment, electronicsDepartment } = useSevenElevenStore(
    ({ fruitDepartment, electronicsDepartment }) => ({ fruitDepartment, electronicsDepartment }),
);

This is a little bit better since now the component will be rendered only when said properties change but we should also combine it with the shallow function from zustand:

import { shallow } from 'zustand/shallow'

const { fruitDepartment, electronicsDepartment } = useSevenElevenStore(
    ({ fruitDepartment, electronicsDepartment }) => ({ fruitDepartment, electronicsDepartment }),
    shallow,
);

Way 3:

Interacting with the store through a service

class MyShoppingAdapter {
    getMeAMango(): Mango {
        return useSevenElevenStore.getState().getMango('aisle', 'mango');
    }
}

While this work, it doesn't seem like a common approach to handle store interactions in React, since usually this is done by leveraging hooks

Proposal

1. Select a mechanism to handle stores interactions**
2. Expose the store API publicly to be accessed from components directly
3. Bind only the needed properties in each component
4. Complex logic could be moved into dedicated services, only used by the store itself. This also might be beneficial as will expose more testing-surface between the store and the services.

Example

Let's take a look into the VisualizationStep.tsx component and the mechanism to append a new step

  // ...
  const onMiniCatalogClickAppend = (selectedStep: IStepProps) => {
    stepsService.handleAppendStep(data.step, selectedStep);
  };

  return (
            <AppendStepButton
              handleAddBranch={handleAddBranch}
              handleSelectStep={onMiniCatalogClickAppend}
              position={plusIconPosition}
              step={data.step}
              showStepsTab={showStepsTab}
            />
  );
 // ...

Notice how the onMiniCatalogClickAppend handler uses the stepService to add a new step. In turn, the stepService leverage the useFlowsStore to execute the action.

useFlowsStore.getState().insertStep(integrationId, newStep, { mode: 'append' });

Using the previously described proposal, this should look like

  // ...
  const insertStep = useFlowsStore((state) => state.insertStep);
  const onMiniCatalogClickAppend = (selectedStep: IStepProps) => {
    insertStep('integrationId', selectedStep);
  });

  return (
            <AppendStepButton
              handleAddBranch={handleAddBranch}
              handleSelectStep={onMiniCatalogClickAppend}
              position={plusIconPosition}
              step={data.step}
              showStepsTab={showStepsTab}
            />
  );
 // ...

Then, in the insertStep function from the store, we would call all the related method to calculate where exactly this step should be included and under which conditions.

export const useFlowsStore = create<IFlowsStore>()(
    (set) => ({
      insertStep: (integrationId, newStep, options) => {
        stepsService.calculateId(newStep);
        stepsService.getParentPath(newStep);
        // ... more logic if needed
      },
// ...

[lordrip]

cc @kahboom @igarashitm @mmelko

[igarashitm]

Great point 👍 if there's potential performance impact, we should get away from Way 1. When I tried to do Way 1 the point was to reduce business logic from components as much as possible, but it shouldn't compromise the performance. And we have to be consistent on this contract.
For me the Way 2 looks like a bit too deep details are leaking from store into components, but it might be coming from an another point:
The Proposal #4, separation of data and functions. Although I don't yet completely understand all the stores in Kaoto UI, my first impression when I looked at the stores code was that it includes business logic in spite of the name which says it's a "store". I don't have a clear picture, but I felt like we'd be better to isolate the logic from stores more, and coordinate between other services... for example, split the logics from stores into each *StoreService, and/or integrate with existing services such as StepsService and VisualizationService. Or could be even possible to maintain 2 layers, store services and other services (assuming it corresponds to UI actions, action services?)

Just a thought from one joined later without background/frontend-architecture knowledge 😅

[lordrip]

Thanks for sharing @igarashitm, from my side, considering any given store as the owner of the data, I would see two interactions:

UI Components <---> Store <---> Supporting services

but in turn, we have the following:

UI Components |---> Store ---> Supporting services
              |---> Supporting services ---> Store
              |---> Supporting services ---> Store ---> Supporting services

I think that what you did in terms of moving the business logic to dedicated services was the right call, now it should be a matter of deciding how to move forward and go there step by step.

From the API point of view, I think that again considering any Store as a central place to hold data, the smaller the API that exposes to the component, the better, it's also true that apart from exposing data it also should expose how to update that data, then here's is where the actions come into play. IMHO, components should communicate with the store by subscribing to the data and by dispatching actions to update said data, in that sense, using the Store in the components should in theory be better.

That being said, it shouldn't be any problem in having business logic outside the store in dedicated supporting services, for instance, to calculate the step's IDs, this logic could be outside the store 😃

[igarashitm]

Store as a central place to hold data
Exactly this point, and I think the data structure should be the least changed part in the code. The business logic on the other hand - how to see/change the data - changes more frequently as we add more new features. The theory is that the clear separation between those increases maintainability. So I think the operations directly on the store are better to stay primitive so it won't change frequently, then delegate any other operations to the services.
Having said that how much we'll follow the theory is up to us, just one point to consider I guess 😁

[kahboom]

While this might be convenient as we get the state updated and also have access to the store API, it has the drawback of re-rendering the component even when unrelated properties change.
useFlowsStore.getState().insertStep(integrationId, newStep, { mode: 'append' });

🤔 TIL I learned that useFlowsStore exists.. seems to have come from this commit: 8c916e7 but one thing I will say is that I haven't seen that we are having any performance issue, though I suppose creating and reviewing benchmarks could be a good place to start. The bigger problem we were facing was the mixing of business logic with the presentation layer as the requirements grew rather quickly. Of course, we don't have this fully implemented, and there are indeed places where we are not following this philosophy, but that was a matter of not finishing.

The issue with the re-rendering I think is more related to how we are referencing stores directly from the components, where we probably shouldn't be.

UI Components <---> Store <---> Supporting services
Expose the store API publicly to be accessed from components directly

It could just be I'm old-school but to me this pattern doesn't make sense. IMHO, the store should not have any knowledge of the business logic or call services. This will become even more evident if we ever decide to support external stores and we will end up with a lot of duplicated business logic.

The longer term goal we had was to make the UI components completely dumb and as thin as possible, with no business logic in them, leaving that to the services we call. If we had another abstraction for something like a more clearly defined domain model, then I see no problem to move hooks or whatever you'd prefer there instead. We can have business-logic aware components separate from presentational components, perhaps that could be a compromise.

Having said that I can see where we might not be so strict where there are performance concerns, for example, maybe with the code editor or other places, and re-assess the situation if that becomes a problem in other places too. But maybe I have missed some other key affected areas that should be looked at.

[lordrip]

I will say is that I haven't seen that we are having any performance issue

maybe performance wasn't the right word, sorry about that, I was referring to the issue around the step's details tab that was rerendered many times causing flickering, and that wouldn't be the only place.

It could just be I'm old-school but to me this pattern doesn't make sense.

which pattern 😃 ?

IMHO, the store should not have any knowledge of the business logic

That's fine, that logic will live in Services

or call services

This is also fine, instead of calling a service to make calculations, we could call them from a container component and pass it to the store there, that wouldn't be an issue either. The point is mostly to convey that currently, we have components calling a store, then the store calls a service, then this service calls another store or sometimes even the same one. Other times, a component calls both the store and the service and during these calls, more communication between the store and the service happens as well and that doesn't seem about right 🤔

Having said that I can see where we might not be so strict where there are performance concerns, for example, maybe with the code editor or other places, and re-assess the situation if that becomes a problem in other places too.

Yes, absolutely.

But maybe I have missed some other key affected areas that should be looked at.

The most affected part I would say is everything related to steps, being Visualization, VisualizationStep, VisualizationStepsDetails, and the ones that add more steps, there is where I can see more duplication or have cross calls between services and stores.

[igarashitm]

So do we agree that we want to

1. Isolate business logic from components. Make React components dumb
2. Restrict who talks with what, for example components only talk to services or introduce special intermediate component. Correct the current mixed situation like components are talking with both of stores and services
3. Isolate business logic from stores. Focus on data structure. Operations directly implemented in stores should stay primitive to the data structure

[kahboom]

@igarashitm Yep exactly, at least for me I think that makes sense

The point is mostly to convey that currently, we have components calling a store, then the store calls a service, then this service calls another store or sometimes even the same one. Other times, a component calls both the store and the service and during these calls, more communication between the store and the service happens as well and that doesn't seem about right 🤔

Yes that is indeed not right. 😂 Definitely not intentional.

instead of calling a service to make calculations, we could call them from a container component and pass it to the store there

@lordrip Yep. It's a suggestion but it doesn't have to be exactly that unless you agree. I think we are on the same page just some nuances that we can work through. As long as we keep working towards separating concerns I'm happy with your suggestion. 🙂

[lordrip]

Yes, I agree with that @igarashitm @kahboom.

In addition to your comment @igarashitm, I would like to mention that it's very likely that the communication between components and the store, cannot be avoided, as the data binding happens when calling the store hook, i.e. useTargetStore((state) => state.aisle);, without this, no component will be rerendered when the date changes.

It's also true that there are other mechanisms to "react" (bada bum 🥁) to data changes, like subscribing to the store imperatively with useTargetStore.subscribe(() => { do something here });, but that is not necessary in most of the cases.

Personally, I wouldn't attempt to change everything immediately but start with the new code and little by little whenever we're working nearby.

Thanks, both of you for taking the time to follow the discussion 🙇

[lordrip]

Closing the discussion as an agreement was reached ✌️

Summary by @igarashitm :

1. Isolate business logic from components. Make React components dumb
2. Restrict who talks with what, for example components only talk to services or introduce special intermediate component. Correct the current mixed situation like components are talking with both of stores and services
3. Isolate business logic from stores. Focus on data structure. Operations directly implemented in stores should stay primitive to the data structure