salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
496 stars 24 forks source link

communication between modules seems wrong to me #21

Closed ms88privat closed 8 years ago

ms88privat commented 8 years ago

I like some of the ideas behind (redux-)elm. At the moment I'm using redux with redux saga in a real world application, which is working fine but I'm still looking for a new and better way to do things.

After reading the docs twice I got really scared by the custom matcher chapter. The way this example handles the SUM of the two counters feels absolutely wrong and also complicated to me.

In this example I have to copy the business logic of increment on the parent component. But what happens when the child counter also has some methods like double or reset ? There is no way I can get the SUM of these counters anymore, even if I try to copy all these methods. Also this pattern goes against the trend to normalize things?

The normal Redux way of writing a selector (e.g. SUM = state.a + state.b) feels much safer and easier.

What are your opinions about this? Maybe with the right component structure, we can avoid such use cases... I have to test it for real.

tomkis commented 8 years ago

It should have been better explained in docs but Custom Matcher implementation is definitely not meant to be used to solve any inter-component communication.

Event Log is Source of truth, not application state

Speaking in ES/CQRS terms Event Log is source of truth and therefore technically you don't need to duplicate business logic, since you only worry about the fact that something has been incremented, you don't care about underlying projection (which in our case is App State).

Saga can be used for communication between bounded contexts

It's quite common that Saga should be responsible for translating business events into more complex business side effects (where the side effect can be an action) and therefore can be used to communicate between bounded contexts.

Your specific example would be:

function* saga() {
  yield takeEvery(['Increment', 'Double', 'Reset'], function*() {
    const value = yield select();
    yield put({ type: 'CounterChanged', value });
  });
}

const initialModel = 0;
export default new Updater(initialModel, saga)
  .case('Increment', model => model + 1)
  .case('Double', model => model + 2)
  .case('Reset', model => 0)
  .toUpdater();

EndsWithMatcher is used to translate locally scoped Events into globally scoped Events, which is exactly what @slorber proposed here.

Anyway, I understand that your main concern is probably about data fetched from API which should be stored in normalized way and I do agree that it would make sense to keep normalized entity repository in one place and I am working on thin library which allows this. Once it's open you could check the code to get some inspiration, but it's quite easy to implement with redux-elm :-)

ms88privat commented 8 years ago

@tomkis1 thanks for your reply. Could you please help my understanding further?

Your specific example would be

Please clarify, what this example shows. It does show some implementation detail of the counter component itself, right? The "CounterChanged" action is then to be used by my parent component to calculate the SUM? (with the help of the EndsWithMatcher)

My goal for this example would be to SUM over all Counter-Values within the App. Every Counter can be independent incremented, doubled or resetted. Especially the "reset" part is hard to manage, because resetting one Counter only affects partly the SUM.

To get the reset part work with your above example, I would need to get the old value of this counter (before the reset) to get the actual change in value, which I could then add (/subtract) to my SUM. What is the best way to do this? Here is my solution, but I'm not sure if I could handle all the different cases this way or if there is a better way todo something like this?

But I can see the beauty in dispatching some "public api actions" of the components (e.g. CounterChanged)

function* saga() {
  // pseudo code
  yield takeEvery(['prepareReset'], function*() {
    const value = yield select();
    const subValue = value * -1;
    yield put({ type: 'CounterChanged', subValue });
    yield put({ type: 'Reset' });
  });
  yield takeEvery(['Increment', 'Double'], function*() {
    const value = yield select();
    yield put({ type: 'CounterChanged', value });
  });
}

const initialModel = 0;
export default new Updater(initialModel, saga)
  .case('Increment', model => model + 1)
  .case('Double', model => model + 2)
  .case('Reset', model => 0)
  .toUpdater();

your main concern is probably about data fetched from API

Not quite yet, but looking forward to your solution. 👍 If I understand how todo the SUM example correctly, I'm going to give this library a try for my new App :)

Event Log is Source of truth, not application state

I thought about that too, maybe it is. But I'm not sure what do do with it. It's like "if you know all of your App (reducer) and everything what happened (actions), then you know the state of your App. E.g. if you know everything, you know everything. But it doesn't give you anything useful.

tomkis commented 8 years ago

I thought about that too, maybe it is. But I'm not sure what do do with it. It's like "if you know all of your App (reducer) and everything what happened (actions), then you know the state of your App. E.g. if you know everything, you know everything. But it doesn't give you anything useful.

You can reconstruct the State anytime you want / need since reducers are pure functions, the state information lies in the sequence of actions and that's the only thing that matters. This is especially very useful with memoized reducers (something like https://github.com/slorber/rereduce) since you can query the State anytime, without even keeping the reference to the State, you can simply re-construct it on demand without performance penalty.

Please clarify, what this example shows. It does show some implementation detail of the counter component itself, right? The "CounterChanged" action is then to be used by my parent component to calculate the SUM? (with the help of the EndsWithMatcher)

With your example you are on really good path of understanding communication between bounded contexts. However, your example seems a bit more complicated than is actually needed.

IMO suming is implementation detail of the Parent component and therefore we can keep the logic there.

const initialModel = {
  counterValues: {}
  //...
};

// action.wrap contains ID of the component
export default new Updater(intialModel)
  .case('CounterChanged', (model, action) => ({
    ...model,
    counterValues: {
      ...model.counterValues,
      [action.wrap]: action.value
    }
  }), endsWithMatcher);

// This can be re-select selector
const countersSumSelector = model =>
  model
    .counterValues
    .reduce((memo, counterValue) => memo + counterValue, 0);

See ? We have effectively split the responsibility between Counter and Counters Aggregator.

ms88privat commented 8 years ago

Thats it! Nice example and idea 💯

I was thinking all the time of how I can access the state of every Counter to calculate my SUM. But you showed me now, that I can build up a new state in my CountersAggregator! Very neat.

Bringing up "action.wrap" == ID in my mind again helps also a lot. Didn't thought about that after only reading and theorizing about this library.

I will soon start my new project with this and see how it goes in a real world scenario :)

tomkis commented 8 years ago

@ms88privat forgot to mention that but there's an excellent article https://msdn.microsoft.com/en-us/library/jj591569.aspx which covers the problem.

ghola commented 8 years ago

Sorry to resurrect this but this solution seems to be an overly complicated way to obtain something simple like a diff. Let me elaborate.

Assume we are on the server side, we're applying DDD, we have WarehouseItem a aggregate (which holds an inventory count for that item and the item dimensions) and we also have a Warehouse aggregate (which holds the occupied warehouse space ... and let's assume space can be computed if we know the dimension of each item in the warehouse and it's count). It's an oversimplified example, but the main idea is that given an operation on a WarehouseItem like addInventory(numberOfItems) i would like to be able to also adjust the occupied space in the Warehouse. The way to do that is for the addInventory domain operation to produce an event, say WarehouseItemInventoryAdjusted, which contains the item dimensions and the adjustment (or if you will, the before and after item counts). Some event listener grabs that event and applies it to the Warehouse aggregate (message passing can be done via a bus, event can be converted to command before being send to a command handler that does triggers the actual domain operation on Warehouse ... unimportant details).

The point is that domain events usually carry the information needed by it's listeners, and if not, listeners have ways of fetching the additional information they need.

Coming back to the solution proposed in this thread and applying it to my example it would mean that the Warehouse aggregate would maintain a list of inventory counts for all the items in the system ... and that's madness. You can have millions of items. There is no reason for the Warehouse aggregate to keep all that data if all it needs is an adjustment (a diff of the data) and that adjustment is easily obtained (contained in the event).

So is there no simple way (@ms88privat did show a way but it's a lot of effort put into it) to publish events/actions with the before and after of the model? The way i would imagine it would be for the saga to be able to provide me with the before and after models, but that requires support from the redux-elm library.

ghola commented 8 years ago

I just got an idea, but it's still too verbose. What if the model of the child component would actually be made of two models, before and after? Like this:

const initialModel = {before: 0, after: 0};
export default new Updater(initialModel, saga)
  .case('Increment', model => ({before: model.after, after: model.after + 1}))
  .case('Double', model => ({before: model.after, after: model.after + 2}))
  .case('Reset', model => ({before: model.after, after: 0}))
  .toUpdater();

Then in the saga it would be trivial to produce the diff.

Is there a way to remove the verbosity?

ghola commented 8 years ago

One way to do it would be encapsulate the before and after switching into a function like this:

const mutate = (model, mutationFn) => ({before: model.after, after: mutationFn(model.after)})

and then use it like this:

const initialModel = {before: 0, after: 0};
export default new Updater(initialModel, saga)
  .case('Increment', model => mutate(model, (m) => m + 1))
  .case('Double', model => mutate(model, (m) => m + 2))
  .case('Reset', model => mutate(model, (m) => 0))
  .toUpdater();

The mutate function is generic enough to be used with any model, as long as that model contains the before and after keys. What if the mutate function would be somehow part of a smarter .case that would detect if the model has before and after keys and would apply the before and after switching for us. This would lead to a code that looks very much like the original:

const initialModel = {before: 0, after: 0};
export default new Updater(initialModel, saga)
  .case('Increment', model => model + 1)
  .case('Double', model => model + 2)
  .case('Reset', model => 0)
  .toUpdater();

One could just extend the Updater class and wrap/decorate the .case method to provide this functionality.

tomkis commented 8 years ago

@ghola I am kinda lost here. So can we have a real example why would you need reference to previous and next model?

ghola commented 8 years ago

Sure, let's use the initial issue described by the OP. This is how the final code of the child would look like:

function* saga() {
  yield takeEvery(['Increment', 'Double', 'Reset'], function*() {
    const model= yield select();
    yield put({ type: 'CounterChanged', adjustment: model.after - model.before});
  });
}

const initialModel = {before: 0, after: 0};
export default new Updater(initialModel, saga)
  .case('Increment', model => model + 1)
  .case('Double', model => model + 2)
  .case('Reset', model => 0)
  .toUpdater();

The parent component would listen to CounterChanged which contains the value adjustment. So now the parent can compute the sum by simply adding the adjustment paylod to his current sum. If the child counter is incremented the adjustment is positive, if decremented is negative. Reset works because it would be a negative number equal to value of the counter before resetting.

This makes the code in the parent component very simple and the parent component does not need to maintain values for each of his children.

tomkis commented 8 years ago
function* saga() {
  let previousModel = yield select(...);

  yield takeEvery(['Increment', 'Double', 'Reset'], function* () {
    const model = yield select(...);
    yield put({ type: 'CounterChanged', adjustment: model - previousModel });

    previousModel = model;
  });
}

Would this somehow help?

ghola commented 8 years ago

Yup, your solution is much better than my attempts. It keeps everything isolated in the saga and does not pollute the actual model. So yeah, this would probably be the best solution to the OPs issue.

ms88privat commented 8 years ago

thinking outside the box marks the spot here .... 👍

(still didn't find the time to test this library in a real project :( )